[PATCH iproute2 v4 0/4] update ifstat for new stats

[PATCH iproute2 v4 0/4] update ifstat for new stats

[Nogah Frankel]

Previously stats were gotten by RTM_GETLINK which returns 32 bits based
statistics. It supports only one type of stats.
Lately, a new method to get stats was added - RTM_GETSTATS. It supports
ability to choose stats type. The basic stats were changed from 32 bits
based to 64 bits based.

This patchset adds ifstat the ability to get extended stats by this
method. Its adds two types of extended stats:
64bits - the same as the "normal" stats but get the stats from the cpu
in 64 bits based struct.
cpu_hits - for packets that hit cpu.

---
v3->v4:
- patch 2/4:
 - change xstat name read to avoid redundant copy.
 - delete extra line
- patch 4/4:
 - change xstat name.

v2->v3:
- patch 1/4:
 - add a new patch to reorder includes in misc/ifstat.c
- patch 2/4: (previously 1/3)
 - fix typos.
 - change error print to use fprintf.

v1->v2:
 - change from using RTM_GETSTATS always to using it only for extended
   stats.
 - Add 64bits extended stats type.

Nogah Frankel (4):
  ifstat: Includes reorder
  ifstat: Add extended statistics to ifstat
  ifstat: Add 64 bits based stats to extended statistics
  ifstat: Add "sw only" extended statistics to ifstat

 misc/ifstat.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 152 insertions(+), 18 deletions(-)

-- 
2.4.3

[PATCH iproute2 v4 1/4] ifstat: Includes reorder

[Nogah Frankel]

Reorder the includes order in misc/ifstat.c to match convention.

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 misc/ifstat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 92d67b0..5bcbcc8 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -28,12 +28,12 @@
 #include <math.h>
 #include <getopt.h>
 
-#include <libnetlink.h>
-#include <json_writer.h>
 #include <linux/if.h>
 #include <linux/if_link.h>
 
-#include <SNAPSHOT.h>
+#include "libnetlink.h"
+#include "json_writer.h"
+#include "SNAPSHOT.h"
 
 int dump_zeros;
 int reset_history;
-- 
2.4.3

[PATCH iproute2 v4 2/4] ifstat: Add extended statistics to ifstat

[Nogah Frankel]

Extended stats are part of the RTM_GETSTATS method. This patch adds them
to ifstat.
While extended stats can come in many forms, we support only the
rtnl_link_stats64 struct for them (which is the 64 bits version of struct
rtnl_link_stats).
We support stats in the main nesting level, or one lower.
The extension can be called by its name or any shorten of it. If there is
more than one matched, the first one will be picked.

To get the extended stats the flag -x <stats type> is used.

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 misc/ifstat.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 145 insertions(+), 15 deletions(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 5bcbcc8..9467119 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -34,6 +34,7 @@
 #include "libnetlink.h"
 #include "json_writer.h"
 #include "SNAPSHOT.h"
+#include "utils.h"
 
 int dump_zeros;
 int reset_history;
@@ -48,17 +49,21 @@ int pretty;
 double W;
 char **patterns;
 int npatterns;
+bool is_extended;
+int filter_type;
+int sub_type;
 
 char info_source[128];
 int source_mismatch;
 
 #define MAXS (sizeof(struct rtnl_link_stats)/sizeof(__u32))
+#define NO_SUB_TYPE 0xffff
 
 struct ifstat_ent {
 	struct ifstat_ent	*next;
 	char			*name;
 	int			ifindex;
-	unsigned long long	val[MAXS];
+	__u64			val[MAXS];
 	double			rate[MAXS];
 	__u32			ival[MAXS];
 };
@@ -106,6 +111,48 @@ static int match(const char *id)
 	return 0;
 }
 
+static int get_nlmsg_extended(const struct sockaddr_nl *who,
+			      struct nlmsghdr *m, void *arg)
+{
+	struct if_stats_msg *ifsm = NLMSG_DATA(m);
+	struct rtattr *tb[IFLA_STATS_MAX+1];
+	int len = m->nlmsg_len;
+	struct ifstat_ent *n;
+
+	if (m->nlmsg_type != RTM_NEWSTATS)
+		return 0;
+
+	len -= NLMSG_LENGTH(sizeof(*ifsm));
+	if (len < 0)
+		return -1;
+
+	parse_rtattr(tb, IFLA_STATS_MAX, IFLA_STATS_RTA(ifsm), len);
+	if (tb[filter_type] == NULL)
+		return 0;
+
+	n = malloc(sizeof(*n));
+	if (!n)
+		abort();
+
+	n->ifindex = ifsm->ifindex;
+	n->name = strdup(ll_index_to_name(ifsm->ifindex));
+
+	if (sub_type == NO_SUB_TYPE) {
+		memcpy(&n->val, RTA_DATA(tb[filter_type]), sizeof(n->val));
+	} else {
+		struct rtattr *attr;
+
+		attr = parse_rtattr_one_nested(sub_type, tb[filter_type]);
+		if (attr == NULL)
+			return 0;
+		memcpy(&n->val, RTA_DATA(attr), sizeof(n->val));
+	}
+	memset(&n->rate, 0, sizeof(n->rate));
+	n->next = kern_db;
+	kern_db = n;
+	return 0;
+}
+
 static int get_nlmsg(const struct sockaddr_nl *who,
 		     struct nlmsghdr *m, void *arg)
 {
@@ -147,18 +194,34 @@ static void load_info(void)
 {
 	struct ifstat_ent *db, *n;
 	struct rtnl_handle rth;
+	__u32 filter_mask;
 
 	if (rtnl_open(&rth, 0) < 0)
 		exit(1);
 
-	if (rtnl_wilddump_request(&rth, AF_INET, RTM_GETLINK) < 0) {
-		perror("Cannot send dump request");
-		exit(1);
-	}
+	if (is_extended) {
+		ll_init_map(&rth);
+		filter_mask = IFLA_STATS_FILTER_BIT(filter_type);
+		if (rtnl_wilddump_stats_req_filter(&rth, AF_UNSPEC, RTM_GETSTATS,
+						   filter_mask) < 0) {
+			perror("Cannot send dump request");
+			exit(1);
+		}
 
-	if (rtnl_dump_filter(&rth, get_nlmsg, NULL) < 0) {
-		fprintf(stderr, "Dump terminated\n");
-		exit(1);
+		if (rtnl_dump_filter(&rth, get_nlmsg_extended, NULL) < 0) {
+			fprintf(stderr, "Dump terminated\n");
+			exit(1);
+		}
+	} else {
+		if (rtnl_wilddump_request(&rth, AF_INET, RTM_GETLINK) < 0) {
+			perror("Cannot send dump request");
+			exit(1);
+		}
+
+		if (rtnl_dump_filter(&rth, get_nlmsg, NULL) < 0) {
+			fprintf(stderr, "Dump terminated\n");
+			exit(1);
+		}
 	}
 
 	rtnl_close(&rth);
@@ -553,10 +616,17 @@ static void update_db(int interval)
 				}
 				for (i = 0; i < MAXS; i++) {
 					double sample;
-					unsigned long incr = h1->ival[i] - n->ival[i];
+					__u64 incr;
+
+					if (is_extended) {
+						incr = h1->val[i] - n->val[i];
+						n->val[i] = h1->val[i];
+					} else {
+						incr = (__u32) (h1->ival[i] - n->ival[i]);
+						n->val[i] += incr;
+						n->ival[i] = h1->ival[i];
+					}
 
-					n->val[i] += incr;
-					n->ival[i] = h1->ival[i];
 					sample = (double)(incr*1000)/interval;
 					if (interval >= scan_interval) {
 						n->rate[i] += W*(sample-n->rate[i]);
@@ -656,6 +726,47 @@ static int verify_forging(int fd)
 	return -1;
 }
 
+static void xstat_usage(void)
+{
+	fprintf(stderr,
+"Usage: ifstat supported xstats:\n");
+}
+
+struct extended_stats_options_t {
+	char *name;
+	int id;
+	int sub_type;
+};
+
+/* Note: if one xstat name is subset of another, it should be before it in this
+ * list.
+ * Name length must be under 64 chars.
+ */
+static const struct extended_stats_options_t extended_stats_options[] = {
+};
+
+static const char *get_filter_type(const char *name)
+{
+	int name_len;
+	int i;
+
+	name_len = strlen(name);
+	for (i = 0; i < ARRAY_SIZE(extended_stats_options); i++) {
+		const struct extended_stats_options_t *xstat;
+
+		xstat = &extended_stats_options[i];
+		if (strncmp(name, xstat->name, name_len) == 0) {
+			filter_type = xstat->id;
+			sub_type = xstat->sub_type;
+			return xstat->name;
+		}
+	}
+
+	fprintf(stderr, "invalid ifstat extension %s\n", name);
+	xstat_usage();
+	return NULL;
+}
+
 static void usage(void) __attribute__((noreturn));
 
 static void usage(void)
@@ -673,7 +784,8 @@ static void usage(void)
 "   -s, --noupdate       don't update history\n"
 "   -t, --interval=SECS  report average over the last SECS\n"
 "   -V, --version        output version information\n"
-"   -z, --zeros          show entries with zero activity\n");
+"   -z, --zeros          show entries with zero activity\n"
+"   -x, --extended=TYPE  show extended stats of TYPE\n");
 
 	exit(-1);
 }
@@ -691,6 +803,7 @@ static const struct option longopts[] = {
 	{ "interval", 1, 0, 't' },
 	{ "version", 0, 0, 'V' },
 	{ "zeros", 0, 0, 'z' },
+	{ "extended", 1, 0, 'x'},
 	{ 0 }
 };
 
@@ -699,10 +812,12 @@ int main(int argc, char *argv[])
 	char hist_name[128];
 	struct sockaddr_un sun;
 	FILE *hist_fp = NULL;
+	const char *stats_type = NULL;
 	int ch;
 	int fd;
 
-	while ((ch = getopt_long(argc, argv, "hjpvVzrnasd:t:e",
+	is_extended = false;
+	while ((ch = getopt_long(argc, argv, "hjpvVzrnasd:t:ex:",
 			longopts, NULL)) != EOF) {
 		switch (ch) {
 		case 'z':
@@ -743,6 +858,10 @@ int main(int argc, char *argv[])
 				exit(-1);
 			}
 			break;
+		case 'x':
+			stats_type = optarg;
+			is_extended = true;
+			break;
 		case 'v':
 		case 'V':
 			printf("ifstat utility, iproute2-ss%s\n", SNAPSHOT);
@@ -757,6 +876,12 @@ int main(int argc, char *argv[])
 	argc -= optind;
 	argv += optind;
 
+	if (stats_type) {
+		stats_type = get_filter_type(stats_type);
+		if (!stats_type)
+			exit(-1);
+	}
+
 	sun.sun_family = AF_UNIX;
 	sun.sun_path[0] = 0;
 	sprintf(sun.sun_path+1, "ifstat%d", getuid());
@@ -795,8 +920,13 @@ int main(int argc, char *argv[])
 		snprintf(hist_name, sizeof(hist_name),
 			 "%s", getenv("IFSTAT_HISTORY"));
 	else
-		snprintf(hist_name, sizeof(hist_name),
-			 "%s/.ifstat.u%d", P_tmpdir, getuid());
+		if (!stats_type)
+			snprintf(hist_name, sizeof(hist_name),
+				 "%s/.ifstat.u%d", P_tmpdir, getuid());
+		else
+			snprintf(hist_name, sizeof(hist_name),
+				 "%s/.%s_ifstat.u%d", P_tmpdir, stats_type,
+				 getuid());
 
 	if (reset_history)
 		unlink(hist_name);
-- 
2.4.3

[PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics

[Nogah Frankel]

The default stats for ifstat are 32 bits based.
The kernel supports 64 bits based stats. (They are returned in struct
rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
which the "normal" stats are returned, but with fields of u64 instead of
u32). This patch adds them as an extended stats.

It is read with filter type IFLA_STATS_LINK_64 and no sub type.

It is under the name 64bits
(or any shorten of it as "64")

For example:
ifstat -x 64bit

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 misc/ifstat.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 9467119..3478f0a 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -729,7 +729,8 @@ static int verify_forging(int fd)
 static void xstat_usage(void)
 {
 	fprintf(stderr,
-"Usage: ifstat supported xstats:\n");
+"Usage: ifstat supported xstats:\n"
+"       64bits         default stats, with 64 bits support\n");
 }
 
 struct extended_stats_options_t {
@@ -743,6 +744,7 @@ struct extended_stats_options_t {
  * Name length must be under 64 chars.
  */
 static const struct extended_stats_options_t extended_stats_options[] = {
+	{"64bits", IFLA_STATS_LINK_64, NO_SUB_TYPE},
 };
 
 static const char *get_filter_type(const char *name)
-- 
2.4.3

[PATCH iproute2 v4 4/4] ifstat: Add "sw only" extended statistics to ifstat

[Nogah Frankel]

Add support for extended statistics of SW only type, for counting only the
packets that went via the cpu. (useful for systems with forward
offloading). It reads it from filter type IFLA_STATS_LINK_OFFLOAD_XSTATS
and sub type IFLA_OFFLOAD_XSTATS_CPU_HIT.

It is under the name 'cpu_hits'
(or any shorten of it as 'cpu' or simply 'c')

For example:
ifstat -x c

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 misc/ifstat.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 3478f0a..5b6a36b 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -730,7 +730,8 @@ static void xstat_usage(void)
 {
 	fprintf(stderr,
 "Usage: ifstat supported xstats:\n"
-"       64bits         default stats, with 64 bits support\n");
+"       64bits         default stats, with 64 bits support\n"
+"       cpu_hits       Counts only packets that went via the CPU.\n");
 }
 
 struct extended_stats_options_t {
@@ -745,6 +746,7 @@ struct extended_stats_options_t {
  */
 static const struct extended_stats_options_t extended_stats_options[] = {
 	{"64bits", IFLA_STATS_LINK_64, NO_SUB_TYPE},
+	{"cpu_hits",  IFLA_STATS_LINK_OFFLOAD_XSTATS, IFLA_OFFLOAD_XSTATS_CPU_HIT},
 };
 
 static const char *get_filter_type(const char *name)
-- 
2.4.3

Re: [PATCH iproute2 v4 0/4] update ifstat for new stats

[Jiri Benc]

On Thu, 12 Jan 2017 15:49:47 +0200, Nogah Frankel wrote:
> Previously stats were gotten by RTM_GETLINK which returns 32 bits based
> statistics. It supports only one type of stats.
> Lately, a new method to get stats was added - RTM_GETSTATS. It supports
> ability to choose stats type. The basic stats were changed from 32 bits
> based to 64 bits based.
> 
> This patchset adds ifstat the ability to get extended stats by this
> method. Its adds two types of extended stats:
> 64bits - the same as the "normal" stats but get the stats from the cpu
> in 64 bits based struct.
> cpu_hits - for packets that hit cpu.

Please add also documentation to the respective man pages.

 Jiri

Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics

[Stephen Hemminger]

On Thu, 12 Jan 2017 15:49:50 +0200
Nogah Frankel <nogahf@mellanox.com> wrote:

> The default stats for ifstat are 32 bits based.
> The kernel supports 64 bits based stats. (They are returned in struct
> rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
> which the "normal" stats are returned, but with fields of u64 instead of
> u32). This patch adds them as an extended stats.
> 
> It is read with filter type IFLA_STATS_LINK_64 and no sub type.
> 
> It is under the name 64bits
> (or any shorten of it as "64")
> 
> For example:
> ifstat -x 64bit
> 
> Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Other commands (like ip link) always use the 64 bit statistics if available
from the device. I see no reason that ifstat needs to be different.

Re: [PATCH iproute2 v4 1/4] ifstat: Includes reorder

[Sergei Shtylyov]

On 1/12/2017 4:49 PM, Nogah Frankel wrote:

> Reorder the includes order in misc/ifstat.c to match convention.

    "Reorder the ... order" sounds a bit tautological. :-)

MBR, Sergei

RE: [PATCH iproute2 v4 1/4] ifstat: Includes reorder

[Nogah Frankel]

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> Sent: Friday, January 13, 2017 12:11 PM
> To: Nogah Frankel <nogahf@mellanox.com>; netdev@vger.kernel.org
> Cc: stephen@networkplumber.org; roszenrami@gmail.com;
> roopa@cumulusnetworks.com; Jiri Pirko <jiri@mellanox.com>; Ido Schimmel
> <idosch@mellanox.com>; Elad Raz <eladr@mellanox.com>; Yotam Gigi
> <yotamg@mellanox.com>; Or Gerlitz <ogerlitz@mellanox.com>
> Subject: Re: [PATCH iproute2 v4 1/4] ifstat: Includes reorder
> 
> On 1/12/2017 4:49 PM, Nogah Frankel wrote:
> 
> > Reorder the includes order in misc/ifstat.c to match convention.
> 
>     "Reorder the ... order" sounds a bit tautological. :-)
> 
> MBR, Sergei

I'll find a better description.

Thanks :)

RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics

[Nogah Frankel]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, January 13, 2017 3:44 AM
> To: Nogah Frankel <nogahf@mellanox.com>
> Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com; Jiri
> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> <ogerlitz@mellanox.com>
> Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
> 
> On Thu, 12 Jan 2017 15:49:50 +0200
> Nogah Frankel <nogahf@mellanox.com> wrote:
> 
> > The default stats for ifstat are 32 bits based.
> > The kernel supports 64 bits based stats. (They are returned in struct
> > rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
> > which the "normal" stats are returned, but with fields of u64 instead of
> > u32). This patch adds them as an extended stats.
> >
> > It is read with filter type IFLA_STATS_LINK_64 and no sub type.
> >
> > It is under the name 64bits
> > (or any shorten of it as "64")
> >
> > For example:
> > ifstat -x 64bit
> >
> > Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> > Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> 
> Other commands (like ip link) always use the 64 bit statistics if available
> from the device. I see no reason that ifstat needs to be different.
> 

Do you mean to change the default ifstat results to be 64 bits based?
I tried it in the first version, but Roopa commented that it was not a good idea.
She said they tried it in the past and it caused backward compatibilities problems.
(Or maybe I didn't understand correctly)

RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics

[Nogah Frankel]

> -----Original Message-----
> From: Nogah Frankel
> Sent: Sunday, January 15, 2017 3:55 PM
> To: 'Stephen Hemminger' <stephen@networkplumber.org>
> Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com; Jiri
> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> <ogerlitz@mellanox.com>
> Subject: RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
> 
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Friday, January 13, 2017 3:44 AM
> > To: Nogah Frankel <nogahf@mellanox.com>
> > Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com;
> Jiri
> > Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> > <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> > <ogerlitz@mellanox.com>
> > Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
> >
> > On Thu, 12 Jan 2017 15:49:50 +0200
> > Nogah Frankel <nogahf@mellanox.com> wrote:
> >
> > > The default stats for ifstat are 32 bits based.
> > > The kernel supports 64 bits based stats. (They are returned in struct
> > > rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
> > > which the "normal" stats are returned, but with fields of u64 instead of
> > > u32). This patch adds them as an extended stats.
> > >
> > > It is read with filter type IFLA_STATS_LINK_64 and no sub type.
> > >
> > > It is under the name 64bits
> > > (or any shorten of it as "64")
> > >
> > > For example:
> > > ifstat -x 64bit
> > >
> > > Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> > > Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> >
> > Other commands (like ip link) always use the 64 bit statistics if available
> > from the device. I see no reason that ifstat needs to be different.
> >
> 
> Do you mean to change the default ifstat results to be 64 bits based?
> I tried it in the first version, but Roopa commented that it was not a good idea.
> She said they tried it in the past and it caused backward compatibilities problems.
> (Or maybe I didn't understand correctly)

So, can I leave the default ifstat results to be 32 bits based, for the time being?

Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics

[Roopa Prabhu]

On 1/19/17, 7:21 AM, Nogah Frankel wrote:
>> -----Original Message-----
>> From: Nogah Frankel
>> Sent: Sunday, January 15, 2017 3:55 PM
>> To: 'Stephen Hemminger' <stephen@networkplumber.org>
>> Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com; Jiri
>> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
>> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
>> <ogerlitz@mellanox.com>
>> Subject: RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
>>
>>
>>
>>> -----Original Message-----
>>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>>> Sent: Friday, January 13, 2017 3:44 AM
>>> To: Nogah Frankel <nogahf@mellanox.com>
>>> Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com;
>> Jiri
>>> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
>>> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
>>> <ogerlitz@mellanox.com>
>>> Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
>>>
>>> On Thu, 12 Jan 2017 15:49:50 +0200
>>> Nogah Frankel <nogahf@mellanox.com> wrote:
>>>
>>>> The default stats for ifstat are 32 bits based.
>>>> The kernel supports 64 bits based stats. (They are returned in struct
>>>> rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
>>>> which the "normal" stats are returned, but with fields of u64 instead of
>>>> u32). This patch adds them as an extended stats.
>>>>
>>>> It is read with filter type IFLA_STATS_LINK_64 and no sub type.
>>>>
>>>> It is under the name 64bits
>>>> (or any shorten of it as "64")
>>>>
>>>> For example:
>>>> ifstat -x 64bit
>>>>
>>>> Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
>>>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>>> Other commands (like ip link) always use the 64 bit statistics if available
>>> from the device. I see no reason that ifstat needs to be different.
>>>
>> Do you mean to change the default ifstat results to be 64 bits based?
>> I tried it in the first version, but Roopa commented that it was not a good idea.
>> She said they tried it in the past and it caused backward compatibilities problems.
>> (Or maybe I didn't understand correctly)
> So, can I leave the default ifstat results to be 32 bits based, for the time being?
>
>From past discussions: Moving the default to 64bit has compat issues with the old history file.
There is a way to make it work by using a new file header (to indicate that it is 64 bit) in
a freshly created history file and also check this header before dumping stats into the history file.
ie maintain backward compat without introducing a new option. It is doable.

One approach is, you can drop the 64bit option from this series and
try updating the default to 64 bit (with compat handling code) in a later series.

Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics

[Stephen Hemminger]

On Thu, 19 Jan 2017 08:06:21 -0800
Roopa Prabhu <roopa@cumulusnetworks.com> wrote:

> On 1/19/17, 7:21 AM, Nogah Frankel wrote:
> >> -----Original Message-----
> >> From: Nogah Frankel
> >> Sent: Sunday, January 15, 2017 3:55 PM
> >> To: 'Stephen Hemminger' <stephen@networkplumber.org>
> >> Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com; Jiri
> >> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> >> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> >> <ogerlitz@mellanox.com>
> >> Subject: RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
> >>
> >>
> >>  
> >>> -----Original Message-----
> >>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> >>> Sent: Friday, January 13, 2017 3:44 AM
> >>> To: Nogah Frankel <nogahf@mellanox.com>
> >>> Cc: netdev@vger.kernel.org; roszenrami@gmail.com; roopa@cumulusnetworks.com;  
> >> Jiri  
> >>> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> >>> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> >>> <ogerlitz@mellanox.com>
> >>> Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
> >>>
> >>> On Thu, 12 Jan 2017 15:49:50 +0200
> >>> Nogah Frankel <nogahf@mellanox.com> wrote:
> >>>  
> >>>> The default stats for ifstat are 32 bits based.
> >>>> The kernel supports 64 bits based stats. (They are returned in struct
> >>>> rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
> >>>> which the "normal" stats are returned, but with fields of u64 instead of
> >>>> u32). This patch adds them as an extended stats.
> >>>>
> >>>> It is read with filter type IFLA_STATS_LINK_64 and no sub type.
> >>>>
> >>>> It is under the name 64bits
> >>>> (or any shorten of it as "64")
> >>>>
> >>>> For example:
> >>>> ifstat -x 64bit
> >>>>
> >>>> Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> >>>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>  
> >>> Other commands (like ip link) always use the 64 bit statistics if available
> >>> from the device. I see no reason that ifstat needs to be different.
> >>>  
> >> Do you mean to change the default ifstat results to be 64 bits based?
> >> I tried it in the first version, but Roopa commented that it was not a good idea.
> >> She said they tried it in the past and it caused backward compatibilities problems.
> >> (Or maybe I didn't understand correctly)  
> > So, can I leave the default ifstat results to be 32 bits based, for the time being?
> >  
> From past discussions: Moving the default to 64bit has compat issues with the old history file.
> There is a way to make it work by using a new file header (to indicate that it is 64 bit) in
> a freshly created history file and also check this header before dumping stats into the history file.
> ie maintain backward compat without introducing a new option. It is doable.
> 
> One approach is, you can drop the 64bit option from this series and
> try updating the default to 64 bit (with compat handling code) in a later series.

The ifstat code could do conversion based on file size.

  if (history_file_is_32bit()) {
	printf("converting to 64 bit format\n");
        ...
  }

RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics

[Nogah Frankel]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, January 19, 2017 9:11 PM
> To: Roopa Prabhu <roopa@cumulusnetworks.com>
> Cc: Nogah Frankel <nogahf@mellanox.com>; netdev@vger.kernel.org;
> roszenrami@gmail.com; Jiri Pirko <jiri@mellanox.com>; Ido Schimmel
> <idosch@mellanox.com>; Elad Raz <eladr@mellanox.com>; Yotam Gigi
> <yotamg@mellanox.com>; Or Gerlitz <ogerlitz@mellanox.com>
> Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
> 
> On Thu, 19 Jan 2017 08:06:21 -0800
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
> 
> > On 1/19/17, 7:21 AM, Nogah Frankel wrote:
> > >> -----Original Message-----
> > >> From: Nogah Frankel
> > >> Sent: Sunday, January 15, 2017 3:55 PM
> > >> To: 'Stephen Hemminger' <stephen@networkplumber.org>
> > >> Cc: netdev@vger.kernel.org; roszenrami@gmail.com;
> roopa@cumulusnetworks.com; Jiri
> > >> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> > >> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> > >> <ogerlitz@mellanox.com>
> > >> Subject: RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended
> statistics
> > >>
> > >>
> > >>
> > >>> -----Original Message-----
> > >>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > >>> Sent: Friday, January 13, 2017 3:44 AM
> > >>> To: Nogah Frankel <nogahf@mellanox.com>
> > >>> Cc: netdev@vger.kernel.org; roszenrami@gmail.com;
> roopa@cumulusnetworks.com;
> > >> Jiri
> > >>> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> > >>> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> > >>> <ogerlitz@mellanox.com>
> > >>> Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended
> statistics
> > >>>
> > >>> On Thu, 12 Jan 2017 15:49:50 +0200
> > >>> Nogah Frankel <nogahf@mellanox.com> wrote:
> > >>>
> > >>>> The default stats for ifstat are 32 bits based.
> > >>>> The kernel supports 64 bits based stats. (They are returned in struct
> > >>>> rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
> > >>>> which the "normal" stats are returned, but with fields of u64 instead of
> > >>>> u32). This patch adds them as an extended stats.
> > >>>>
> > >>>> It is read with filter type IFLA_STATS_LINK_64 and no sub type.
> > >>>>
> > >>>> It is under the name 64bits
> > >>>> (or any shorten of it as "64")
> > >>>>
> > >>>> For example:
> > >>>> ifstat -x 64bit
> > >>>>
> > >>>> Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> > >>>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> > >>> Other commands (like ip link) always use the 64 bit statistics if available
> > >>> from the device. I see no reason that ifstat needs to be different.
> > >>>
> > >> Do you mean to change the default ifstat results to be 64 bits based?
> > >> I tried it in the first version, but Roopa commented that it was not a good idea.
> > >> She said they tried it in the past and it caused backward compatibilities problems.
> > >> (Or maybe I didn't understand correctly)
> > > So, can I leave the default ifstat results to be 32 bits based, for the time being?
> > >
> > From past discussions: Moving the default to 64bit has compat issues with the old
> history file.
> > There is a way to make it work by using a new file header (to indicate that it is 64 bit) in
> > a freshly created history file and also check this header before dumping stats into the
> history file.
> > ie maintain backward compat without introducing a new option. It is doable.
> >
> > One approach is, you can drop the 64bit option from this series and
> > try updating the default to 64 bit (with compat handling code) in a later series.

I think I will take your suggestion to drop the 64 bits from this series.
Hopefully, I'll return to it in some later series in the future.
Thanks

> 
> The ifstat code could do conversion based on file size.
> 
>   if (history_file_is_32bit()) {
> 	printf("converting to 64 bit format\n");
>         ...
>   }
> 
>