Re: [B.A.T.M.A.N.] [RFC 1/2] batman-adv: Add interface to keep stats

[Marek Lindner <lindner_marek@yahoo.de>]

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() ?


> +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 ?


> +/* 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.


> +/* 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.


> 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 ?


> +struct bat_stats {
> +	seqlock_t lock;			/* seqlock for fast write operation */
> +	struct timespec timestamp;	/* Timestamp of data */

What is the timestamp used for ?

Regards,
Marek