Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Add get_ethtool_stats() support

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 3554 bytes --]

On Tue, Apr 17, 2012 at 01:24:55 +0200, Martin Hundebøll wrote:
> @@ -488,6 +492,8 @@ bool dat_snoop_outgoing_arp_reply(struct bat_priv *bat_priv,
>  	arp_neigh_update(bat_priv, ip_src, hw_src);
>  	arp_neigh_update(bat_priv, ip_dst, hw_dst);
>  
> +	bat_priv->bat_stats.dat_reply_tx++;
> +
>  	/* Send the ARP reply to the candidates for both the IP addresses we
>  	 * fetched from the ARP reply */
>  	dht_send_data(bat_priv, skb, ip_src, BAT_P_DAT_DHT_PUT);

Actually in dht_send_data() the skb is cloned and sent multiple times.
Should we count all the skbs that we send out?



> @@ -498,3 +505,57 @@ static u32 bat_get_link(struct net_device *dev)
>  {
>  	return 1;
>  }
> +
> +/* Inspired by drivers/net/ethernet/dlink/sundance.c:1702 */
> +static const struct {
> +	const char name[ETH_GSTRING_LEN];
> +} bat_stats_strings[] = {
> +	{ "forward" },
> +	{ "mgmt_tx" },
> +	{ "mgmt_rx" },
> +	{ "tt_request_tx" },
> +	{ "tt_request_rx" },
> +	{ "tt_response_tx" },
> +	{ "tt_response_rx" },
> +	{ "tt_roam_adv_tx" },
> +	{ "tt_roam_adv_rx" },
> +	{ "dat_request_tx" },
> +	{ "dat_request_rx" },
> +	{ "dat_reply_tx" },
> +	{ "dat_reply_rx" },
> +};
> +#define BAT_STATS_NUM 13

Do you really need internal { } ?
And why not using const char name* instead of name[N] ? All the strings are
const char * and they are already in the constant space (however it is called).

> +
> +static void bat_get_ethtool_stats(struct net_device *dev,
> +				  struct ethtool_stats *stats, u64 *data)
> +{
> +	struct bat_priv *bat_priv = netdev_priv(dev);
> +
> +	data[0] = bat_priv->bat_stats.forward;
> +	data[1] = bat_priv->bat_stats.mgmt_tx;
> +	data[2] = bat_priv->bat_stats.mgmt_rx;
> +	data[3] = bat_priv->bat_stats.tt_request_tx;
> +	data[4] = bat_priv->bat_stats.tt_request_rx;
> +	data[5] = bat_priv->bat_stats.tt_response_tx;
> +	data[6] = bat_priv->bat_stats.tt_response_rx;
> +	data[7] = bat_priv->bat_stats.tt_roam_adv_tx;
> +	data[8] = bat_priv->bat_stats.tt_roam_adv_rx;
> +	data[9] = bat_priv->bat_stats.dat_request_tx;
> +	data[10] = bat_priv->bat_stats.dat_request_rx;
> +	data[11] = bat_priv->bat_stats.dat_reply_tx;
> +	data[12] = bat_priv->bat_stats.dat_reply_rx;
> +}

(guess mode ON) why using u64? what if the arch is 32bit? are the values
correctly assigned anyway? what about longer addresses (do they exist?)?
As Sven suggested on IRC, you probably want to use uintptr_t instead.

> --- a/types.h
> +++ b/types.h
> @@ -162,9 +162,26 @@ struct bcast_duplist_entry {
>  };
>  #endif
>  
> +struct bat_stats {
> +	uint64_t forward;
> +	uint64_t mgmt_tx;
> +	uint64_t mgmt_rx;
> +	uint64_t tt_request_tx;
> +	uint64_t tt_request_rx;
> +	uint64_t tt_response_tx;
> +	uint64_t tt_response_rx;
> +	uint64_t tt_roam_adv_tx;
> +	uint64_t tt_roam_adv_rx;
> +	uint64_t dat_request_tx;
> +	uint64_t dat_request_rx;
> +	uint64_t dat_reply_tx;
> +	uint64_t dat_reply_rx;
> +};

see previous comment.


One general comment: be consistent with integer types. I would suggest to use
uint*_t only, instead of mixing u* and uint*_t.

The last stuff we have to understand is if we can really use shared counters
without having them as atomic_t or per_cpu variable...

Sven linked me this page: http://lwn.net/Articles/22911/
it seems that per_cpu variables should be used for network stats..but the effort
to use them seems to be not negligible :-P


Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]