All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH iproute2 v5 0/4] update ifstat for new stats
@ 2017-01-26 12:44 Nogah Frankel
  2017-01-26 12:44 ` [PATCH iproute2 v5 1/4] ifstat: Includes reorder Nogah Frankel
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
  To: netdev
  Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
	idosch, yotamg, ogerlitz, 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 (xstats) by
this method. Its adds xstat of cpu_hits - for packets that hit cpu.

---
v4->v5:
 - remove patch 3/4 that added 64 based "normal" stats
 - add a new patch 4/4 that updates ifstat man page

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 "sw only" extended statistics to ifstat
  ifstat: Add xstat to ifstat man page

 man/man8/Makefile |   3 +-
 man/man8/ifstat.8 |  12 +++-
 misc/ifstat.c     | 168 ++++++++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 163 insertions(+), 20 deletions(-)

-- 
2.4.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH iproute2 v5 1/4] ifstat: Includes reorder
  2017-01-26 12:44 [PATCH iproute2 v5 0/4] update ifstat for new stats Nogah Frankel
@ 2017-01-26 12:44 ` Nogah Frankel
  2017-01-26 12:44 ` [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat Nogah Frankel
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
  To: netdev
  Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
	idosch, yotamg, ogerlitz, Nogah Frankel

Reorder the includes 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
  2017-01-26 12:44 [PATCH iproute2 v5 0/4] update ifstat for new stats Nogah Frankel
  2017-01-26 12:44 ` [PATCH iproute2 v5 1/4] ifstat: Includes reorder Nogah Frankel
@ 2017-01-26 12:44 ` Nogah Frankel
  2017-01-30  4:34   ` Stephen Hemminger
  2017-02-03 18:07   ` Stephen Hemminger
  2017-01-26 12:44 ` [PATCH iproute2 v5 3/4] ifstat: Add "sw only" " Nogah Frankel
  2017-01-26 12:44 ` [PATCH iproute2 v5 4/4] ifstat: Add xstat to ifstat man page Nogah Frankel
  3 siblings, 2 replies; 10+ messages in thread
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
  To: netdev
  Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
	idosch, yotamg, ogerlitz, 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH iproute2 v5 3/4] ifstat: Add "sw only" extended statistics to ifstat
  2017-01-26 12:44 [PATCH iproute2 v5 0/4] update ifstat for new stats Nogah Frankel
  2017-01-26 12:44 ` [PATCH iproute2 v5 1/4] ifstat: Includes reorder Nogah Frankel
  2017-01-26 12:44 ` [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat Nogah Frankel
@ 2017-01-26 12:44 ` Nogah Frankel
  2017-01-26 12:44 ` [PATCH iproute2 v5 4/4] ifstat: Add xstat to ifstat man page Nogah Frankel
  3 siblings, 0 replies; 10+ messages in thread
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
  To: netdev
  Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
	idosch, yotamg, ogerlitz, 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 9467119..a853ee6 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"
+"       cpu_hits       Counts only packets that went via the CPU.\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[] = {
+	{"cpu_hits",  IFLA_STATS_LINK_OFFLOAD_XSTATS, IFLA_OFFLOAD_XSTATS_CPU_HIT},
 };
 
 static const char *get_filter_type(const char *name)
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH iproute2 v5 4/4] ifstat: Add xstat to ifstat man page
  2017-01-26 12:44 [PATCH iproute2 v5 0/4] update ifstat for new stats Nogah Frankel
                   ` (2 preceding siblings ...)
  2017-01-26 12:44 ` [PATCH iproute2 v5 3/4] ifstat: Add "sw only" " Nogah Frankel
@ 2017-01-26 12:44 ` Nogah Frankel
  3 siblings, 0 replies; 10+ messages in thread
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
  To: netdev
  Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
	idosch, yotamg, ogerlitz, Nogah Frankel

Add documentation about the extended statistics to the ifstat man page.
Add ifstat man age to the man8 Makefile

Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 man/man8/Makefile |  3 ++-
 man/man8/ifstat.8 | 12 +++++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/man/man8/Makefile b/man/man8/Makefile
index 77d347c..bc2fc81 100644
--- a/man/man8/Makefile
+++ b/man/man8/Makefile
@@ -18,7 +18,8 @@ MAN8PAGES = $(TARGETS) ip.8 arpd.8 lnstat.8 routel.8 rtacct.8 rtmon.8 rtpr.8 ss.
 	tc-connmark.8 tc-csum.8 tc-mirred.8 tc-nat.8 tc-pedit.8 tc-police.8 \
 	tc-simple.8 tc-skbedit.8 tc-vlan.8 tc-xt.8  tc-ife.8 \
 	tc-tunnel_key.8 \
-	devlink.8 devlink-dev.8 devlink-monitor.8 devlink-port.8 devlink-sb.8
+	devlink.8 devlink-dev.8 devlink-monitor.8 devlink-port.8 devlink-sb.8 \
+	ifstat.8
 
 all: $(TARGETS)
 
diff --git a/man/man8/ifstat.8 b/man/man8/ifstat.8
index e49d868..3ba0088 100644
--- a/man/man8/ifstat.8
+++ b/man/man8/ifstat.8
@@ -14,7 +14,8 @@ ifstat \- handy utility to read network interface statistics
 The utility keeps records of the previous data displayed in history files and
 by default only shows difference between the last and the current call.
 Location of the history files defaults to /tmp/.ifstat.u$UID but may be
-overridden with the IFSTAT_HISTORY environment variable.
+overridden with the IFSTAT_HISTORY environment variable. Similarly, the default
+location for xstat (extended stats) is /tmp/.<xstat name>_ifstat.u$UID.
 .SH OPTIONS
 .TP
 .B \-h, \-\-help
@@ -46,6 +47,15 @@ Report average over the last SECS seconds.
 .TP
 .B \-z, \-\-zeros
 Show entries with zero activity.
+.TP
+.B \-x, \-\-extended=TYPE
+Show extended stats of TYPE. Supported types are:
+
+.in +8
+.B cpu_hits
+- Counts only packets that went via the CPU.
+.in -8
+
 .SH ENVIRONMENT
 .TP
 .B IFSTAT_HISTORY
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
  2017-01-26 12:44 ` [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat Nogah Frankel
@ 2017-01-30  4:34   ` Stephen Hemminger
  2017-02-02  8:53     ` Nogah Frankel
  2017-02-03 18:07   ` Stephen Hemminger
  1 sibling, 1 reply; 10+ messages in thread
From: Stephen Hemminger @ 2017-01-30  4:34 UTC (permalink / raw)
  To: Nogah Frankel
  Cc: netdev, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
	idosch, yotamg, ogerlitz

On Thu, 26 Jan 2017 14:44:39 +0200
Nogah Frankel <nogahf@mellanox.com> wrote:

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

It would be better if this command used 64 bit statistics transparently
like other tools in iproute2. Transparency is always better user experience.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
  2017-01-30  4:34   ` Stephen Hemminger
@ 2017-02-02  8:53     ` Nogah Frankel
  0 siblings, 0 replies; 10+ messages in thread
From: Nogah Frankel @ 2017-02-02  8:53 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: netdev, roopa, roszenrami, jbenc, sergei.shtylyov, Jiri Pirko,
	Elad Raz, Ido Schimmel, Yotam Gigi, Or Gerlitz



> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, January 30, 2017 6:34 AM
> To: Nogah Frankel <nogahf@mellanox.com>
> Cc: netdev@vger.kernel.org; roopa@cumulusnetworks.com; roszenrami@gmail.com;
> jbenc@redhat.com; sergei.shtylyov@cogentembedded.com; Jiri Pirko
> <jiri@mellanox.com>; Elad Raz <eladr@mellanox.com>; Ido Schimmel
> <idosch@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> <ogerlitz@mellanox.com>
> Subject: Re: [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
> 
> On Thu, 26 Jan 2017 14:44:39 +0200
> Nogah Frankel <nogahf@mellanox.com> wrote:
> 
> > 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.
> 
> It would be better if this command used 64 bit statistics transparently
> like other tools in iproute2. Transparency is always better user experience.

From user point of view, all the statistics in ifstat are 64 bits and are saved that way.
We currently get the stats in 32 bits structs and translate them to 64 bits. It keeps track
of  int overflows (in the 32 bits).
The problem with it is  that if between one ifstat call to another passes too much time,
one might lose one of the overflows. 
But to change it is not a trivial matter, and I believe it is not related to the xstat feature I
want to add.
I don't think I understood what is the change you want.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
  2017-01-26 12:44 ` [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat Nogah Frankel
  2017-01-30  4:34   ` Stephen Hemminger
@ 2017-02-03 18:07   ` Stephen Hemminger
  2017-02-07 12:59     ` Nogah Frankel
  1 sibling, 1 reply; 10+ messages in thread
From: Stephen Hemminger @ 2017-02-03 18:07 UTC (permalink / raw)
  To: Nogah Frankel
  Cc: netdev, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
	idosch, yotamg, ogerlitz

On Thu, 26 Jan 2017 14:44:39 +0200
Nogah Frankel <nogahf@mellanox.com> wrote:

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

Sorry I was confused because RTM_GETSTATS contains multiple statistics.
Your patch is about getting LINK_XSTATS and after looking in more detail, you are
correct this should be an option. Although it would make sense to show this as addition
to the basic statistics. And when I tested it no output happens which seems confusing.

$ ./misc/ifstat -p -x cpu_hits
#kernel
Interface        RX Pkts/Rate    TX Pkts/Rate    RX Data/Rate    TX Data/Rate  
                 RX Errs/Drop    TX Errs/Drop    RX Over/Rate    TX Coll/Rate 

What I was intending in earlier discussion was using IFLA_STATS_LINK_64 which would
allow supporting 64 bit statistics on 32 bit platforms.
 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
  2017-02-03 18:07   ` Stephen Hemminger
@ 2017-02-07 12:59     ` Nogah Frankel
  2017-02-07 16:48       ` Stephen Hemminger
  0 siblings, 1 reply; 10+ messages in thread
From: Nogah Frankel @ 2017-02-07 12:59 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: netdev, roopa, roszenrami, jbenc, sergei.shtylyov, Jiri Pirko,
	Elad Raz, Ido Schimmel, Yotam Gigi, Or Gerlitz


> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, February 03, 2017 8:07 PM
> To: Nogah Frankel <nogahf@mellanox.com>
> Cc: netdev@vger.kernel.org; roopa@cumulusnetworks.com; roszenrami@gmail.com;
> jbenc@redhat.com; sergei.shtylyov@cogentembedded.com; Jiri Pirko
> <jiri@mellanox.com>; Elad Raz <eladr@mellanox.com>; Ido Schimmel
> <idosch@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> <ogerlitz@mellanox.com>
> Subject: Re: [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
> 
> On Thu, 26 Jan 2017 14:44:39 +0200
> Nogah Frankel <nogahf@mellanox.com> wrote:
> 
> > 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>
> 
> Sorry I was confused because RTM_GETSTATS contains multiple statistics.
> Your patch is about getting LINK_XSTATS and after looking in more detail, you are
> correct this should be an option. Although it would make sense to show this as addition
> to the basic statistics. And when I tested it no output happens which seems confusing.
> 
> $ ./misc/ifstat -p -x cpu_hits
> #kernel
> Interface        RX Pkts/Rate    TX Pkts/Rate    RX Data/Rate    TX Data/Rate
>                  RX Errs/Drop    TX Errs/Drop    RX Over/Rate    TX Coll/Rate
> 

Not all devices support this xstat.
Do you prefer another print in this case?

About printing both the xstat and the default stats together, it may be problematic since 
ifstat is about diffs. I think it is better that one ifstat call for a specific stats, won't change
the other stats data. (And since we are talking about diffs, reading data meaning changing
it).

> What I was intending in earlier discussion was using IFLA_STATS_LINK_64 which would
> allow supporting 64 bit statistics on 32 bit platforms.
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
  2017-02-07 12:59     ` Nogah Frankel
@ 2017-02-07 16:48       ` Stephen Hemminger
  0 siblings, 0 replies; 10+ messages in thread
From: Stephen Hemminger @ 2017-02-07 16:48 UTC (permalink / raw)
  To: Nogah Frankel
  Cc: netdev, roopa, roszenrami, jbenc, sergei.shtylyov, Jiri Pirko,
	Elad Raz, Ido Schimmel, Yotam Gigi, Or Gerlitz

On Tue, 7 Feb 2017 12:59:11 +0000
Nogah Frankel <nogahf@mellanox.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Friday, February 03, 2017 8:07 PM
> > To: Nogah Frankel <nogahf@mellanox.com>
> > Cc: netdev@vger.kernel.org; roopa@cumulusnetworks.com; roszenrami@gmail.com;
> > jbenc@redhat.com; sergei.shtylyov@cogentembedded.com; Jiri Pirko
> > <jiri@mellanox.com>; Elad Raz <eladr@mellanox.com>; Ido Schimmel
> > <idosch@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> > <ogerlitz@mellanox.com>
> > Subject: Re: [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
> > 
> > On Thu, 26 Jan 2017 14:44:39 +0200
> > Nogah Frankel <nogahf@mellanox.com> wrote:
> >   
> > > 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>  
> > 
> > Sorry I was confused because RTM_GETSTATS contains multiple statistics.
> > Your patch is about getting LINK_XSTATS and after looking in more detail, you are
> > correct this should be an option. Although it would make sense to show this as addition
> > to the basic statistics. And when I tested it no output happens which seems confusing.
> > 
> > $ ./misc/ifstat -p -x cpu_hits
> > #kernel
> > Interface        RX Pkts/Rate    TX Pkts/Rate    RX Data/Rate    TX Data/Rate
> >                  RX Errs/Drop    TX Errs/Drop    RX Over/Rate    TX Coll/Rate
> >   
> 
> Not all devices support this xstat.
> Do you prefer another print in this case?
> 
> About printing both the xstat and the default stats together, it may be problematic since 
> ifstat is about diffs. I think it is better that one ifstat call for a specific stats, won't change
> the other stats data. (And since we are talking about diffs, reading data meaning changing
> it).

Maybe an error would be a better experience for  user.
I merged the current version but try and think about how to make it work better in general.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-02-07 16:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-26 12:44 [PATCH iproute2 v5 0/4] update ifstat for new stats Nogah Frankel
2017-01-26 12:44 ` [PATCH iproute2 v5 1/4] ifstat: Includes reorder Nogah Frankel
2017-01-26 12:44 ` [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat Nogah Frankel
2017-01-30  4:34   ` Stephen Hemminger
2017-02-02  8:53     ` Nogah Frankel
2017-02-03 18:07   ` Stephen Hemminger
2017-02-07 12:59     ` Nogah Frankel
2017-02-07 16:48       ` Stephen Hemminger
2017-01-26 12:44 ` [PATCH iproute2 v5 3/4] ifstat: Add "sw only" " Nogah Frankel
2017-01-26 12:44 ` [PATCH iproute2 v5 4/4] ifstat: Add xstat to ifstat man page Nogah Frankel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.