On 04/04/2012 10:07 AM, Marek Lindner wrote:
On Monday, April 02, 2012 14:48:15 Martin Hundebøll wrote:
@@ -265,6 +266,18 @@ static int vis_data_open(struct inode *inode, struct file *file) return single_open(file, vis_seq_print_text, net_dev); }
+static int bat_stats_open(struct inode *inode, struct file *file) +{
- struct net_device *net_dev = (struct net_device *)inode->i_private;
- return single_open(file, bat_stats_show, net_dev);
+}
Did you consider using bat_priv->stats or ethtool->get_ethtool_stats() ?
Wouldn't that require patching netdevice.h? And in that case, is there any chance to get batman-adv specific counters into the generel netdev stats structure?
+static int bat_stats_clear_open(struct inode *inode, struct file *file) +{
- struct net_device *net_dev = (struct net_device *)inode->i_private;
- return single_open(file, bat_stats_clear, net_dev);
+}
How about writing 0 to the stats file resets the counters, thus avoiding a second file ?
That is possible, but then I would have to write the debugfs-handling myself, instead of using the nice macro defined in bat_debugfs.c. It's a tradeoff between code size and number of files in debugfs.
+/* Update one or more stat counters by passing the appropriate flags. */ +void bat_stats_update(struct bat_priv *bat_priv, uint32_t flags) +{
- if (bat_priv&& flags) {
write_seqlock(&bat_priv->bat_stats->lock);if (flags& BAT_STAT_XMIT)atomic_inc(&bat_priv->bat_stats->transmitted);if (flags& BAT_STAT_RECV)atomic_inc(&bat_priv->bat_stats->received);if (flags& BAT_STAT_FORWARD)atomic_inc(&bat_priv->bat_stats->forwarded);if (flags& BAT_STAT_CODE)atomic_inc(&bat_priv->bat_stats->coded);if (flags& BAT_STAT_DECODE)atomic_inc(&bat_priv->bat_stats->decoded);if (flags& BAT_STAT_DECODE_FAIL)atomic_inc(&bat_priv->bat_stats->decode_failed);if (flags& BAT_STAT_RECODE)atomic_inc(&bat_priv->bat_stats->recoded);if (flags& BAT_STAT_OVERHEARD)atomic_inc(&bat_priv->bat_stats->overheard);write_sequnlock(&bat_priv->bat_stats->lock);- }
+}
The network coding flags should be added with the network coding patchset.
True.
+/* debugfs function to list statistics */ +int bat_stats_show(struct seq_file *seq, void *offset) +{
- struct net_device *net_dev = (struct net_device *)seq->private;
- struct bat_priv *bat_priv = netdev_priv(net_dev);
- struct bat_stats *stats = bat_priv->bat_stats;
- seqlock_t *lock =&stats->lock;
- int transmitted, received, forwarded, coded, decoded, decode_failed,
recoded, overheard;- unsigned long sval;
- do {
sval = read_seqbegin(lock);transmitted = atomic_read(&stats->transmitted);received = atomic_read(&stats->received);forwarded = atomic_read(&stats->forwarded);coded = atomic_read(&stats->coded);decoded = atomic_read(&stats->decoded);decode_failed = atomic_read(&stats->decode_failed);recoded = atomic_read(&stats->recoded);overheard = atomic_read(&stats->overheard);- } while (read_seqretry(lock, sval));
- seq_printf(seq, "Transmitted: %d\n", transmitted);
- seq_printf(seq, "Received: %d\n", received);
- seq_printf(seq, "Forwarded: %d\n", forwarded);
- seq_printf(seq, "Coded: %d\n", coded);
- seq_printf(seq, "Recoded: %d\n", recoded);
- seq_printf(seq, "Decoded: %d\n", decoded);
- seq_printf(seq, "Failed: %d\n", decode_failed);
- seq_printf(seq, "Overheard: %d\n", overheard);
- return 0;
+}
Why not simply printing the atomic counters ? I also don't quite understand why we need to add an additional layer of locking. This potentially slows down the traffic forwarding.
True, I don't suppose the stats need to be that precise. The original reason for locking, was that multiple counters could be incremented with one call to bat_stats_update(). I don't think it is used with more than one flag anywhere, so converting the entire usage into simple increments should be preferable.
diff --git a/types.h b/types.h index 15f538a..41b2217 100644 --- a/types.h +++ b/types.h @@ -240,6 +240,7 @@ struct bat_priv { #endif struct vis_info *my_vis_info; struct bat_algo_ops *bat_algo_ops;
- struct bat_stats *bat_stats; };
If you add an ifdef around the definition here would be no need to malloc bat_stats separately, right ?
Do we prefer IFDEFs in the definition over mallocs? (I guess so by your comment). If thats the case, then no problem with me.
+struct bat_stats {
- seqlock_t lock; /* seqlock for fast write operation */
- struct timespec timestamp; /* Timestamp of data */
What is the timestamp used for ?
It should be updated at every increment and printed in the output, but it is not, so it will be removed.