Hi Antonio,
Thank you for your comments.
On 04/17/2012 09:12 AM, Antonio Quartulli wrote:
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
dht_send_data(bat_priv, skb, ip_src, BAT_P_DAT_DHT_PUT);
- fetched from the ARP reply */
Actually in dht_send_data() the skb is cloned and sent multiple times. Should we count all the skbs that we send out?
I would say that it should be up to the author of the code :)
@@ -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).
As described on IRC, get_ethtool_stats() expects the strings to be aligned to ETH_GSTRING_LEN and in this way, we can copy all the strings in one chunk.
+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.
As you wrote in your later email, you should get more sleep :)
--- 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.
Okay, so if you change bat_get_strings() and bat_get_ethtool_stats() to take uint64_t it is OK, although it is called with u64? (This is what I would prefer.)
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
If we don't mind the performance penalty from using atomic_t, that can easily be changed. But keep in mind that I plan to add counters that are incremented for each and every packet.
To use per_cpu counters, I need to make dynamic allocations of these, which I would gladly do. I just didn't want to over complicate things in the first edition :)
Cheers!