[B.A.T.M.A.N.] [RFC 0/2] Create counters for measuring behaviour

[B.A.T.M.A.N.] [RFC 0/2] Create counters for measuring behaviour

[Martin Hundebøll]

Before sending an RFC on my work with network coding/catwoman, I would like some feedback on this stats-feature I build to measure the behaviour of catwoman.

It is basically a set of counters that are incremented on various places in the code as a more detailed alternative to the interface statistics.

Currently, my catwoman branch uses these counters (e.g. to tell how many packets are coded together), but other counters can easily be added. E.g. counters relevant to the BLA, DAT, OGM/ELP code.

If this feature is unneeded/unwanted, I will remove the dependency in catwoman.

Martin Hundebøll (2):
  batman-adv: Add interface to keep stats
  batman-adv: Increment stat counters on rx, tx, fwd

 Makefile               |    2 +
 Makefile.kbuild        |    1 +
 bat_debugfs.c          |   17 +++++++
 bat_stats.c            |  132 ++++++++++++++++++++++++++++++++++++++++++++++++
 bat_stats.h            |   60 ++++++++++++++++++++++
 gen-compat-autoconf.sh |    1 +
 main.c                 |    6 +++
 routing.c              |    2 +
 soft-interface.c       |    3 ++
 types.h                |   20 ++++++++
 10 files changed, 244 insertions(+)
 create mode 100644 bat_stats.c
 create mode 100644 bat_stats.h

-- 
1.7.9.5

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

[Martin Hundebøll]

For debugging batman-adv and measuring events, counters may be added to
the new bat_stats structure in types.h. Counters are incremented by
calling bat_stats_update() and passing one or more flags.

The stats are exported in the bat_stats file in debugfs and are cleared
by reading the bat_stats_clear file.

Signed-off-by: Martin Hundebøll <martin@hundeboll.net>
---
 Makefile               |    2 +
 Makefile.kbuild        |    1 +
 bat_debugfs.c          |   17 +++++++
 bat_stats.c            |  132 ++++++++++++++++++++++++++++++++++++++++++++++++
 bat_stats.h            |   60 ++++++++++++++++++++++
 gen-compat-autoconf.sh |    1 +
 main.c                 |    6 +++
 types.h                |   20 ++++++++
 8 files changed, 239 insertions(+)
 create mode 100644 bat_stats.c
 create mode 100644 bat_stats.h

diff --git a/Makefile b/Makefile
index ac84fba..0a4b2db 100644
--- a/Makefile
+++ b/Makefile
@@ -25,6 +25,8 @@ export CONFIG_BATMAN_ADV_DEBUG=n
 export CONFIG_BATMAN_ADV_BLA=y
 # B.A.T.M.A.N. distributed ARP table:
 export CONFIG_BATMAN_ADV_DAT=y
+# B.A.T.M.A.N. rx/tx statistics:
+export CONFIG_BATMAN_ADV_STATS=y
 
 PWD:=$(shell pwd)
 KERNELPATH ?= /lib/modules/$(shell uname -r)/build
diff --git a/Makefile.kbuild b/Makefile.kbuild
index ad002cd..37731b3 100644
--- a/Makefile.kbuild
+++ b/Makefile.kbuild
@@ -21,6 +21,7 @@
 obj-$(CONFIG_BATMAN_ADV) += batman-adv.o
 batman-adv-y += bat_debugfs.o
 batman-adv-y += bat_iv_ogm.o
+batman-adv-$(CONFIG_BATMAN_ADV_STATS) += bat_stats.o
 batman-adv-y += bat_sysfs.o
 batman-adv-y += bitarray.o
 batman-adv-$(CONFIG_BATMAN_ADV_BLA) += bridge_loop_avoidance.o
diff --git a/bat_debugfs.c b/bat_debugfs.c
index 3b588f8..64e19ad 100644
--- a/bat_debugfs.c
+++ b/bat_debugfs.c
@@ -33,6 +33,7 @@
 #include "vis.h"
 #include "icmp_socket.h"
 #include "bridge_loop_avoidance.h"
+#include "bat_stats.h"
 
 static struct dentry *bat_debugfs;
 
@@ -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);
+}
+
+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);
+}
+
 struct bat_debuginfo {
 	struct attribute attr;
 	const struct file_operations fops;
@@ -291,6 +304,8 @@ static BAT_DEBUGINFO(bla_claim_table, S_IRUGO, bla_claim_table_open);
 #endif
 static BAT_DEBUGINFO(transtable_local, S_IRUGO, transtable_local_open);
 static BAT_DEBUGINFO(vis_data, S_IRUGO, vis_data_open);
+static BAT_DEBUGINFO(bat_stats, S_IRUGO, bat_stats_open);
+static BAT_DEBUGINFO(bat_stats_clear, S_IRUGO, bat_stats_clear_open);
 
 static struct bat_debuginfo *mesh_debuginfos[] = {
 	&bat_debuginfo_originators,
@@ -301,6 +316,8 @@ static struct bat_debuginfo *mesh_debuginfos[] = {
 #endif
 	&bat_debuginfo_transtable_local,
 	&bat_debuginfo_vis_data,
+	&bat_debuginfo_bat_stats,
+	&bat_debuginfo_bat_stats_clear,
 	NULL,
 };
 
diff --git a/bat_stats.c b/bat_stats.c
new file mode 100644
index 0000000..d95fb02
--- /dev/null
+++ b/bat_stats.c
@@ -0,0 +1,132 @@
+/*
+ * Copyright (C) 2007-2012 B.A.T.M.A.N. contributors:
+ *
+ * Martin Hundebøll, Jeppe Ledet-Pedersen
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA
+ *
+ */
+
+#include "main.h"
+#include "bat_stats.h"
+
+/* 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);
+	}
+}
+
+/* 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;
+}
+
+/* Reset stat counters */
+static void _bat_stats_clear(struct bat_priv *bat_priv)
+{
+	write_seqlock(&bat_priv->bat_stats->lock);
+	atomic_set(&bat_priv->bat_stats->transmitted, 0);
+	atomic_set(&bat_priv->bat_stats->received, 0);
+	atomic_set(&bat_priv->bat_stats->forwarded, 0);
+	atomic_set(&bat_priv->bat_stats->coded, 0);
+	atomic_set(&bat_priv->bat_stats->decoded, 0);
+	atomic_set(&bat_priv->bat_stats->decode_failed, 0);
+	atomic_set(&bat_priv->bat_stats->recoded, 0);
+	atomic_set(&bat_priv->bat_stats->overheard, 0);
+	write_sequnlock(&bat_priv->bat_stats->lock);
+}
+
+/* Callback function to debugfs */
+int bat_stats_clear(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);
+	_bat_stats_clear(bat_priv);
+
+	return 0;
+}
+
+/* Initialize stat counters */
+int bat_stats_init(struct bat_priv *bat_priv)
+{
+	if (bat_priv->bat_stats)
+		return 0;
+
+	bat_priv->bat_stats = kmalloc(sizeof(*bat_priv->bat_stats), GFP_ATOMIC);
+	if (!bat_priv->bat_stats)
+		return -1;
+
+	seqlock_init(&bat_priv->bat_stats->lock);
+	_bat_stats_clear(bat_priv);
+
+	return 0;
+}
+
+void bat_stats_free(struct bat_priv *bat_priv)
+{
+	if (!bat_priv->bat_stats)
+		return;
+
+	kfree(bat_priv->bat_stats);
+}
diff --git a/bat_stats.h b/bat_stats.h
new file mode 100644
index 0000000..34cd1ae
--- /dev/null
+++ b/bat_stats.h
@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2007-2012 B.A.T.M.A.N. contributors:
+ *
+ * Martin Hundebøll, Jeppe Ledet-Pedersen
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA
+ *
+ */
+
+#ifndef _NET_BATMAN_ADV_STATS_H_
+#define _NET_BATMAN_ADV_STATS_H_
+
+#define BAT_STAT_XMIT		(1 << 1)
+#define BAT_STAT_RECV		(1 << 2)
+#define BAT_STAT_FORWARD	(1 << 3)
+#define BAT_STAT_CODE		(1 << 4)
+#define BAT_STAT_DECODE		(1 << 5)
+#define BAT_STAT_DECODE_FAIL	(1 << 6)
+#define BAT_STAT_RECODE		(1 << 7)
+#define BAT_STAT_OVERHEARD	(1 << 8)
+
+#ifdef CONFIG_BATMAN_ADV_STATS
+
+int bat_stats_show(struct seq_file *seq, void *offset);
+int bat_stats_clear(struct seq_file *seq, void *offset);
+int bat_stats_init(struct bat_priv *bat_priv);
+void bat_stats_free(struct bat_priv *bat_priv);
+void bat_stats_update(struct bat_priv *bat_priv, uint32_t flags);
+
+#else /* CONFIG_BATMAN_ADV_STATS */
+
+static inline int bat_stats_show(struct seq_file *seq, void *offset)
+{
+	return 0;
+}
+
+static inline int bat_stats_clear(struct seq_file *seq, void *offset)
+{
+	return 0;
+}
+
+#define bat_stats_init(...)	(0)
+#define bat_stats_free(...)	{}
+#define bat_stats_update(...)	{}
+
+#endif /* CONFIG_BATMAN_ADV_STATS */
+
+#endif /* _NET_BATMAN_ADV_STATS_H_ */
diff --git a/gen-compat-autoconf.sh b/gen-compat-autoconf.sh
index 7ea42aa..98b66a0 100755
--- a/gen-compat-autoconf.sh
+++ b/gen-compat-autoconf.sh
@@ -39,6 +39,7 @@ gen_config() {
 gen_config 'CONFIG_BATMAN_ADV_DEBUG' ${CONFIG_BATMAN_ADV_DEBUG:="n"} >> "${TMP}"
 gen_config 'CONFIG_BATMAN_ADV_BLA' ${CONFIG_BATMAN_ADV_BLA:="y"} >> "${TMP}"
 gen_config 'CONFIG_BATMAN_ADV_DAT' ${CONFIG_BATMAN_ADV_DAT:="y"} >> "${TMP}"
+gen_config 'CONFIG_BATMAN_ADV_STATS' ${CONFIG_BATMAN_ADV_STATS:="n"} >> "${TMP}"
 
 # only regenerate compat-autoconf.h when config was changed
 diff "${TMP}" "${TARGET}" > /dev/null 2>&1 || cp "${TMP}" "${TARGET}"
diff --git a/main.c b/main.c
index 0757c2d..5251716 100644
--- a/main.c
+++ b/main.c
@@ -34,6 +34,7 @@
 #include "vis.h"
 #include "hash.h"
 #include "bat_algo.h"
+#include "bat_stats.h"
 
 
 /* List manipulations on hardif_list have to be rtnl_lock()'ed,
@@ -124,6 +125,9 @@ int mesh_init(struct net_device *soft_iface)
 	if (bla_init(bat_priv) < 1)
 		goto err;
 
+	if (bat_stats_init(bat_priv) < 0)
+		goto err;
+
 	atomic_set(&bat_priv->gw_reselect, 0);
 	atomic_set(&bat_priv->mesh_state, MESH_ACTIVE);
 	goto end;
@@ -153,6 +157,8 @@ void mesh_free(struct net_device *soft_iface)
 
 	bla_free(bat_priv);
 
+	bat_stats_free(bat_priv);
+
 	atomic_set(&bat_priv->mesh_state, MESH_INACTIVE);
 }
 
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;
 };
 
 struct socket_client {
@@ -415,4 +416,23 @@ struct dht_candidate {
 	struct orig_node *orig_node;
 };
 
+struct bat_stats {
+	seqlock_t lock;			/* seqlock for fast write operation */
+	struct timespec timestamp;	/* Timestamp of data */
+
+	/* Generic node stats */
+	atomic_t transmitted;		/* Packets transmitted */
+	atomic_t received;		/* Packets received */
+
+	/* Relay node stats */
+	atomic_t forwarded;		/* Packets forwarded */
+	atomic_t coded;			/* Packets coded */
+	atomic_t recoded;		/* Decoded packets coded */
+
+	/* End node stats */
+	atomic_t decoded;		/* Packets decoded */
+	atomic_t decode_failed;		/* Packets that failed to be decoded */
+	atomic_t overheard;		/* Received, but overheard, packets */
+};
+
 #endif /* _NET_BATMAN_ADV_TYPES_H_ */
-- 
1.7.9.5

[B.A.T.M.A.N.] [RFC 2/2] batman-adv: Increment stat counters on rx, tx, fwd

[Martin Hundebøll]

Make actual use of the stat interface introduced in prev commit.

Signed-off-by: Martin Hundebøll <martin@hundeboll.net>
---
 routing.c        |    2 ++
 soft-interface.c |    3 +++
 2 files changed, 5 insertions(+)

diff --git a/routing.c b/routing.c
index 795d3af..a119648 100644
--- a/routing.c
+++ b/routing.c
@@ -30,6 +30,7 @@
 #include "vis.h"
 #include "unicast.h"
 #include "bridge_loop_avoidance.h"
+#include "bat_stats.h"
 
 static int route_unicast_packet(struct sk_buff *skb,
 				struct hard_iface *recv_if);
@@ -870,6 +871,7 @@ static int route_unicast_packet(struct sk_buff *skb, struct hard_iface *recv_if)
 	unicast_packet->header.ttl--;
 
 	/* route it */
+	bat_stats_update(bat_priv, BAT_STAT_FORWARD);
 	send_skb_packet(skb, neigh_node->if_incoming, neigh_node->addr);
 	ret = NET_RX_SUCCESS;
 
diff --git a/soft-interface.c b/soft-interface.c
index 92137af..774b412 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -38,6 +38,7 @@
 #include <linux/if_vlan.h>
 #include "unicast.h"
 #include "bridge_loop_avoidance.h"
+#include "bat_stats.h"
 
 
 static int bat_get_settings(struct net_device *dev, struct ethtool_cmd *cmd);
@@ -235,6 +236,7 @@ static int interface_tx(struct sk_buff *skb, struct net_device *soft_iface)
 
 		dat_snoop_outgoing_arp_reply(bat_priv, skb);
 
+		bat_stats_update(bat_priv, BAT_STAT_XMIT);
 		ret = unicast_send_skb(skb, bat_priv);
 		if (ret != 0)
 			goto dropped_freed;
@@ -302,6 +304,7 @@ void interface_rx(struct net_device *soft_iface,
 
 /*	skb->ip_summed = CHECKSUM_UNNECESSARY;*/
 
+	bat_stats_update(bat_priv, BAT_STAT_RECV);
 	bat_priv->stats.rx_packets++;
 	bat_priv->stats.rx_bytes += skb->len + ETH_HLEN;
 
-- 
1.7.9.5

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

[Marek Lindner]

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

Re: [B.A.T.M.A.N.] [RFC 0/2] Create counters for measuring behaviour

[Marek Lindner]

On Monday, April 02, 2012 14:48:14 Martin Hundebøll wrote:
> Before sending an RFC on my work with network coding/catwoman, I would like
> some feedback on this stats-feature I build to measure the behaviour of
> catwoman.
> 
> It is basically a set of counters that are incremented on various places in
> the code as a more detailed alternative to the interface statistics.
> 
> Currently, my catwoman branch uses these counters (e.g. to tell how many
> packets are coded together), but other counters can easily be added. E.g.
> counters relevant to the BLA, DAT, OGM/ELP code.
> 
> If this feature is unneeded/unwanted, I will remove the dependency in
> catwoman.

Could you explain in a few words how catwoman depends on the stats counter ?

Regards,
Marek

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

[Martin Hundebøll]

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.


-- 
Kind Regards,
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C

+45 61 65 54 61
martin@hundeboll.net

Re: [B.A.T.M.A.N.] [RFC 0/2] Create counters for measuring behaviour

[Martin Hundebøll]

On 04/04/2012 10:08 AM, Marek Lindner wrote:
> On Monday, April 02, 2012 14:48:14 Martin Hundebøll wrote:
>> Before sending an RFC on my work with network coding/catwoman, I would like
>> some feedback on this stats-feature I build to measure the behaviour of
>> catwoman.
>>
>> It is basically a set of counters that are incremented on various places in
>> the code as a more detailed alternative to the interface statistics.
>>
>> Currently, my catwoman branch uses these counters (e.g. to tell how many
>> packets are coded together), but other counters can easily be added. E.g.
>> counters relevant to the BLA, DAT, OGM/ELP code.
>>
>> If this feature is unneeded/unwanted, I will remove the dependency in
>> catwoman.
>
> Could you explain in a few words how catwoman depends on the stats counter ?

Sure, it's merely because I use the bat_stats-functions in the catwoman branch. Nothing more than that.

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

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

[Marek Lindner]

On Tuesday, April 10, 2012 13:33:11 Martin Hundebøll wrote:
> 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?

No, I did not mean we should patch netdevice.h. The kernel has traffic 
counters we might be able to re-use for our purpose. Especially 
get_ethtool_stats() allows to export custom counters to userspace.
The kernel developers don't like duplicating existing infrastructure. At least 
we should know what the kernel offers and come up with a reasonable 
explanation why they don't help us.


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

Alternatively, you could extend the existing macro to also set a file handler 
for writes instead of assuming only reads. :-)


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

The types.h is full of ifdefs already. It helps to keep the structs smaller 
when functionality is not compiled into the module.

Regards,
Marek

Re: [B.A.T.M.A.N.] [RFC 0/2] Create counters for measuring behaviour

[Marek Lindner]

On Tuesday, April 10, 2012 13:34:46 Martin Hundebøll wrote:
> >> Currently, my catwoman branch uses these counters (e.g. to tell how many
> >> packets are coded together), but other counters can easily be added.
> >> E.g. counters relevant to the BLA, DAT, OGM/ELP code.
> >> 
> >> If this feature is unneeded/unwanted, I will remove the dependency in
> >> catwoman.
> > 
> > Could you explain in a few words how catwoman depends on the stats
> > counter ?
> 
> Sure, it's merely because I use the bat_stats-functions in the catwoman
> branch. Nothing more than that.

If the traffic counter code provides empty functions to be called when it is 
not compiled into the module it should all work ?

Regards,
Marek

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

[Martin Hundebøll]

On 04/10/2012 01:05 PM, Marek Lindner wrote:
> On Tuesday, April 10, 2012 13:33:11 Martin Hundebøll wrote:
>> 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?
>
> No, I did not mean we should patch netdevice.h. The kernel has traffic
> counters we might be able to re-use for our purpose. Especially
> get_ethtool_stats() allows to export custom counters to userspace.
> The kernel developers don't like duplicating existing infrastructure. At least
> we should know what the kernel offers and come up with a reasonable
> explanation why they don't help us.

Sounds like get_ethtool_stats() could do the job - I will look into that.
  
>>>> +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.
>
> Alternatively, you could extend the existing macro to also set a file handler
> for writes instead of assuming only reads. :-)
  
Definitely an option :)
  
>>> 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.
>
> The types.h is full of ifdefs already. It helps to keep the structs smaller
> when functionality is not compiled into the module.

As long as we agree on ifdefs. They weren't there until BLAII and DAT added them, but I don't have a problem with'em. Let's see how long this patch-set will live, as I expect to use the get_ethtool_stats(), if possible.

-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

Re: [B.A.T.M.A.N.] [RFC 0/2] Create counters for measuring behaviour

[Martin Hundebøll]

On 04/10/2012 01:08 PM, Marek Lindner wrote:
> On Tuesday, April 10, 2012 13:34:46 Martin Hundebøll wrote:
>>>> Currently, my catwoman branch uses these counters (e.g. to tell how many
>>>> packets are coded together), but other counters can easily be added.
>>>> E.g. counters relevant to the BLA, DAT, OGM/ELP code.
>>>>
>>>> If this feature is unneeded/unwanted, I will remove the dependency in
>>>> catwoman.
>>>
>>> Could you explain in a few words how catwoman depends on the stats
>>> counter ?
>>
>> Sure, it's merely because I use the bat_stats-functions in the catwoman
>> branch. Nothing more than that.
>
> If the traffic counter code provides empty functions to be called when it is
> not compiled into the module it should all work ?

Yeah, but these empty functions wan't be here, if the bat_stats code is never merged :)


-- 
Kind Regards
Martin Hundebøll
Frederiks Allé 99A, 1.th
8000 Aarhus C
Denmark
+45 61 65 54 61
martin@hundeboll.net

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

[Marek Lindner]

On Tuesday, April 10, 2012 14:17:28 Martin Hundebøll wrote:
> >>> 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.
> > 
> > The types.h is full of ifdefs already. It helps to keep the structs
> > smaller when functionality is not compiled into the module.
> 
> As long as we agree on ifdefs. They weren't there until BLAII and DAT added
> them, but I don't have a problem with'em. Let's see how long this
> patch-set will live, as I expect to use the get_ethtool_stats(), if
> possible.

Some of the counters might exist in bat_priv->stats as Antonio already pointed 
out (bat_priv->stats.rx_packets). They have the advantage of being printed 
with ifconfig and friends.

Regards,
Marek