Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1330075 > unrolled thread

Re: [PATCH net v3 2/4] net: add rx_nohandler stat counter

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net v3 2/4] net: add rx_nohandler stat counter Jamal Hadi Salim <jhs@mojatatu.com> - 2016-02-09 12:00 +0100
    [PATCH net-next iproute2] iplink: display rx nohandler stats Stephen Hemminger <stephen@networkplumber.org> - 2016-02-09 20:20 +0100
      Re: [PATCH net-next iproute2] iplink: display rx nohandler stats Jarod Wilson <jarod@redhat.com> - 2016-02-10 01:00 +0100
        Re: [PATCH net-next iproute2] iplink: display rx nohandler stats Stephen Hemminger <stephen@networkplumber.org> - 2016-02-10 02:50 +0100
          Re: [PATCH net-next iproute2] iplink: display rx nohandler stats Eric Dumazet <eric.dumazet@gmail.com> - 2016-02-10 06:00 +0100
            Re: [PATCH net-next iproute2] iplink: display rx nohandler stats Jarod Wilson <jarod@redhat.com> - 2016-02-10 14:30 +0100
              Re: [PATCH net-next iproute2] iplink: display rx nohandler stats Andy Gospodarek <gospo@cumulusnetworks.com> - 2016-02-10 16:10 +0100

#1330075 — Re: [PATCH net v3 2/4] net: add rx_nohandler stat counter

On 16-02-09 03:40 AM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 08 Feb 2016 14:57:40 -0800
>
>> Whole point of TLV is that it allows us to add new fields at the end of
>> the structures.
>   ...
>> Look at iproute2, you were the one adding in 2004 code to cope with
>> various tcp_info sizes.
>>
>> So 12 years later, you cannot say it does not work anymore.
>
> +1
>

The TLV L should be canonical way to determine length. i.e should be
sufficient to just look at L and understand that content has changed.
But:
Using sizeof could be dangerous unless the data is packed to be
32-bit aligned. Looking INET_DIAG_INFO check for sizeof
there is a small 8 bit hole in tcp_info I think between
these two fields:

----
__u8    tcpi_snd_wscale : 4, tcpi_rcv_wscale : 4;
__u32   tcpi_rto;
---

The kernel will pad to make sure the TLV data is 32-bit aligned.
I am not sure if that will be the same length as sizeof() in all
hardware + compilers... For this case,
it is almost safe to just add a version field - probably in the hole.
Or have a #define to say what the expected length should be. Or add
an 8 bit pad.

In general adding new fields that are non-optional is problematic. i.e
by non-optional i mean always expected to be present.
I think a good test is old kernel with new iproute2. If the new field
is non-optional, it will fail (example iproute2 may try to print a value
that it expects but because old kernel doesnt understand it; it is 
non-existent).

cheers,
jamal

[toc] | [next] | [standalone]

#1330596 — [PATCH net-next iproute2] iplink: display rx nohandler stats

Support for the new rx_nohandler statistic.
This code is designed to handle the case where the kernel reported statistic
structure is smaller than the larger structure in later releases (and vice versa).

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 ip/ipaddress.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 9d254d2..c4a8fc3 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -481,7 +481,8 @@ static void print_link_stats64(FILE *fp, const struct rtnl_link_stats64 *s,
 	/* RX error stats */
 	if (show_stats > 1) {
 		fprintf(fp, "%s", _SL_);
-		fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s", _SL_);
+		fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s%s",
+			s->rx_nohandler ? "   nohandler" : "",  _SL_);
 
 		fprintf(fp, "               ");
 		print_num(fp, 8, s->rx_length_errors);
@@ -489,6 +490,9 @@ static void print_link_stats64(FILE *fp, const struct rtnl_link_stats64 *s,
 		print_num(fp, 7, s->rx_frame_errors);
 		print_num(fp, 7, s->rx_fifo_errors);
 		print_num(fp, 7, s->rx_missed_errors);
+		if (s->rx_nohandler)
+			print_num(fp, 7, s->rx_nohandler);
+
 	}
 	fprintf(fp, "%s", _SL_);
 
@@ -496,7 +500,6 @@ static void print_link_stats64(FILE *fp, const struct rtnl_link_stats64 *s,
 	fprintf(fp, "    TX: bytes  packets  errors  dropped carrier collsns %s%s",
 		s->tx_compressed ? "compressed" : "", _SL_);
 
-
 	fprintf(fp, "    ");
 	print_num(fp, 10, s->tx_bytes);
 	print_num(fp, 8, s->tx_packets);
@@ -546,13 +549,16 @@ static void print_link_stats32(FILE *fp, const struct rtnl_link_stats *s,
 	/* RX error stats */
 	if (show_stats > 1) {
 		fprintf(fp, "%s", _SL_);
-		fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s", _SL_);
+		fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s%s",
+			s->rx_nohandler ? "   nohandler" : "",  _SL_);
 		fprintf(fp, "               ");
 		print_num(fp, 8, s->rx_length_errors);
 		print_num(fp, 7, s->rx_crc_errors);
 		print_num(fp, 7, s->rx_frame_errors);
 		print_num(fp, 7, s->rx_fifo_errors);
 		print_num(fp, 7, s->rx_missed_errors);
+		if (s->rx_nohandler)
+			print_num(fp, 7, s->rx_nohandler);
 	}
 	fprintf(fp, "%s", _SL_);
 
@@ -590,12 +596,23 @@ static void print_link_stats32(FILE *fp, const struct rtnl_link_stats *s,
 
 static void __print_link_stats(FILE *fp, struct rtattr **tb)
 {
-	if (tb[IFLA_STATS64])
-		print_link_stats64(fp, RTA_DATA(tb[IFLA_STATS64]),
-					tb[IFLA_CARRIER_CHANGES]);
-	else if (tb[IFLA_STATS])
-		print_link_stats32(fp, RTA_DATA(tb[IFLA_STATS]),
-					tb[IFLA_CARRIER_CHANGES]);
+	const struct rtattr *carrier_changes = tb[IFLA_CARRIER_CHANGES];
+
+	if (tb[IFLA_STATS64]) {
+		struct rtnl_link_stats64 stats = { 0 };
+
+		memcpy(&stats, RTA_DATA(tb[IFLA_STATS64]),
+		       MIN(RTA_PAYLOAD(tb[IFLA_STATS64]), sizeof(stats)));
+
+		print_link_stats64(fp, &stats, carrier_changes);
+	} else if (tb[IFLA_STATS]) {
+		struct rtnl_link_stats stats = { 0 };
+
+		memcpy(&stats, RTA_DATA(tb[IFLA_STATS]),
+		       MIN(RTA_PAYLOAD(tb[IFLA_STATS]), sizeof(stats)));
+
+		print_link_stats32(fp, &stats, carrier_changes);
+	}
 }
 
 static void print_link_stats(FILE *fp, struct nlmsghdr *n)
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1330778 — Re: [PATCH net-next iproute2] iplink: display rx nohandler stats

On Tue, Feb 09, 2016 at 11:17:57AM -0800, Stephen Hemminger wrote:
> Support for the new rx_nohandler statistic.
> This code is designed to handle the case where the kernel reported statistic
> structure is smaller than the larger structure in later releases (and vice versa).

This seems to work here, for the most part. However, if you are running a
kernel with the new counter, and the counter happens to contain 0, aren't
we going to not print anything?

I've got a tweaked version here locally that gets a touch messy, where I
get a count of members from RTA_DATA(IFLA_STATS{,64} / sizeof(__u{32,64}),
pass that into the print functions, and key off that length for whether or
not to print the extra members, so they'll show up even when 0, if they're
supported. This does rely on strict ordering of the struct members, no
reordering, no removals, etc., but I think everyone is already in favor of
that. Looks like the same sort of length checks could be used for
rx_compressed and tx_compressed as well, as I think they fall victim to
the same issue of not printing if those counters are legitimately 0. Yes,
it's a little uglier, and more brittle, but more accurate output.

Work-in-progress patch:

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 9d254d2..ae4359a 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -462,7 +462,8 @@ static void print_vf_stats64(FILE *fp, struct rtattr *vfstats)
 }
 
 static void print_link_stats64(FILE *fp, const struct rtnl_link_stats64 *s,
-                               const struct rtattr *carrier_changes)
+                               const struct rtattr *carrier_changes,
+			       unsigned slen)
 {
 	/* RX stats */
 	fprintf(fp, "    RX: bytes  packets  errors  dropped overrun mcast   %s%s",
@@ -481,14 +482,16 @@ static void print_link_stats64(FILE *fp, const struct rtnl_link_stats64 *s,
 	/* RX error stats */
 	if (show_stats > 1) {
 		fprintf(fp, "%s", _SL_);
-		fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s", _SL_);
-
+		fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s%s",
+			slen > 23 ? "  nohandler" : "", _SL_);
 		fprintf(fp, "               ");
 		print_num(fp, 8, s->rx_length_errors);
 		print_num(fp, 7, s->rx_crc_errors);
 		print_num(fp, 7, s->rx_frame_errors);
 		print_num(fp, 7, s->rx_fifo_errors);
 		print_num(fp, 7, s->rx_missed_errors);
+		if (slen > 23)
+			print_num(fp, 7, s->rx_nohandler);
 	}
 	fprintf(fp, "%s", _SL_);
 
@@ -496,7 +499,6 @@ static void print_link_stats64(FILE *fp, const struct rtnl_link_stats64 *s,
 	fprintf(fp, "    TX: bytes  packets  errors  dropped carrier collsns %s%s",
 		s->tx_compressed ? "compressed" : "", _SL_);
 
-
 	fprintf(fp, "    ");
 	print_num(fp, 10, s->tx_bytes);
 	print_num(fp, 8, s->tx_packets);
@@ -526,13 +528,13 @@ static void print_link_stats64(FILE *fp, const struct rtnl_link_stats64 *s,
 }
 
 static void print_link_stats32(FILE *fp, const struct rtnl_link_stats *s,
-			       const struct rtattr *carrier_changes)
+			       const struct rtattr *carrier_changes,
+			       unsigned slen)
 {
 	/* RX stats */
 	fprintf(fp, "    RX: bytes  packets  errors  dropped overrun mcast   %s%s",
 		s->rx_compressed ? "compressed" : "", _SL_);
 
-
 	fprintf(fp, "    ");
 	print_num(fp, 10, s->rx_bytes);
 	print_num(fp, 8, s->rx_packets);
@@ -546,13 +548,16 @@ static void print_link_stats32(FILE *fp, const struct rtnl_link_stats *s,
 	/* RX error stats */
 	if (show_stats > 1) {
 		fprintf(fp, "%s", _SL_);
-		fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s", _SL_);
+		fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s%s",
+			slen > 23 ? "  nohandler" : "", _SL_);
 		fprintf(fp, "               ");
 		print_num(fp, 8, s->rx_length_errors);
 		print_num(fp, 7, s->rx_crc_errors);
 		print_num(fp, 7, s->rx_frame_errors);
 		print_num(fp, 7, s->rx_fifo_errors);
 		print_num(fp, 7, s->rx_missed_errors);
+		if (slen > 23)
+			print_num(fp, 7, s->rx_nohandler);
 	}
 	fprintf(fp, "%s", _SL_);
 
@@ -590,12 +595,27 @@ static void print_link_stats32(FILE *fp, const struct rtnl_link_stats *s,
 
 static void __print_link_stats(FILE *fp, struct rtattr **tb)
 {
-	if (tb[IFLA_STATS64])
-		print_link_stats64(fp, RTA_DATA(tb[IFLA_STATS64]),
-					tb[IFLA_CARRIER_CHANGES]);
-	else if (tb[IFLA_STATS])
-		print_link_stats32(fp, RTA_DATA(tb[IFLA_STATS]),
-					tb[IFLA_CARRIER_CHANGES]);
+	const struct rtattr *carrier_changes = tb[IFLA_CARRIER_CHANGES];
+	unsigned slen;
+
+	if (tb[IFLA_STATS64]) {
+		struct rtnl_link_stats64 stats = { 0 };
+		slen = RTA_PAYLOAD(tb[IFLA_STATS64]) / sizeof(__u64);
+
+		memcpy(&stats, RTA_DATA(tb[IFLA_STATS64]),
+		       MIN(RTA_PAYLOAD(tb[IFLA_STATS64]), sizeof(stats)));
+
+		print_link_stats64(fp, &stats, carrier_changes, slen);
+	} else if (tb[IFLA_STATS]) {
+		struct rtnl_link_stats stats = { 0 };
+		slen = RTA_PAYLOAD(tb[IFLA_STATS]) / sizeof(__u32);
+
+		memcpy(&stats, RTA_DATA(tb[IFLA_STATS]),
+		       MIN(RTA_PAYLOAD(tb[IFLA_STATS]), sizeof(stats)));
+
+		print_link_stats32(fp, &stats, carrier_changes, slen);
+	}
+
 }
 
 static void print_link_stats(FILE *fp, struct nlmsghdr *n)

-- 
Jarod Wilson
jarod@redhat.com

[toc] | [prev] | [next] | [standalone]

#1330839 — Re: [PATCH net-next iproute2] iplink: display rx nohandler stats

On Tue, 9 Feb 2016 18:51:35 -0500
Jarod Wilson <jarod@redhat.com> wrote:

> On Tue, Feb 09, 2016 at 11:17:57AM -0800, Stephen Hemminger wrote:
> > Support for the new rx_nohandler statistic.
> > This code is designed to handle the case where the kernel reported statistic
> > structure is smaller than the larger structure in later releases (and vice versa).
> 
> This seems to work here, for the most part. However, if you are running a
> kernel with the new counter, and the counter happens to contain 0, aren't
> we going to not print anything?

That is the desirable outcome, since if run on older system the
output format will not change from current format.


> I've got a tweaked version here locally that gets a touch messy, where I
> get a count of members from RTA_DATA(IFLA_STATS{,64} / sizeof(__u{32,64}),
> pass that into the print functions, and key off that length for whether or
> not to print the extra members, so they'll show up even when 0, if they're
> supported. This does rely on strict ordering of the struct members, no
> reordering, no removals, etc., but I think everyone is already in favor of
> that. Looks like the same sort of length checks could be used for
> rx_compressed and tx_compressed as well, as I think they fall victim to
> the same issue of not printing if those counters are legitimately 0. Yes,
> it's a little uglier, and more brittle, but more accurate output.
> 

I don't like the added complexity.

[toc] | [prev] | [next] | [standalone]

#1330885 — Re: [PATCH net-next iproute2] iplink: display rx nohandler stats

On Tue, 2016-02-09 at 17:41 -0800, Stephen Hemminger wrote:
> On Tue, 9 Feb 2016 18:51:35 -0500
> Jarod Wilson <jarod@redhat.com> wrote:
> 
> > On Tue, Feb 09, 2016 at 11:17:57AM -0800, Stephen Hemminger wrote:
> > > Support for the new rx_nohandler statistic.
> > > This code is designed to handle the case where the kernel reported statistic
> > > structure is smaller than the larger structure in later releases (and vice versa).
> > 
> > This seems to work here, for the most part. However, if you are running a
> > kernel with the new counter, and the counter happens to contain 0, aren't
> > we going to not print anything?
> 
> That is the desirable outcome, since if run on older system the
> output format will not change from current format.

The problem here is that a change in output might break some user
scripts using sed/whatever games.

So it might be better to output a zero field, so that such breakages are
detected early, even if no packet was dropped at the time the new kernel
was tested.

Having a binary that adds the new field only in some cases hides the
change. It looks fine for us humans, but not for programs processing the
output.

[toc] | [prev] | [next] | [standalone]

#1331161 — Re: [PATCH net-next iproute2] iplink: display rx nohandler stats

On Tue, Feb 09, 2016 at 08:52:38PM -0800, Eric Dumazet wrote:
> On Tue, 2016-02-09 at 17:41 -0800, Stephen Hemminger wrote:
> > On Tue, 9 Feb 2016 18:51:35 -0500
> > Jarod Wilson <jarod@redhat.com> wrote:
> > 
> > > On Tue, Feb 09, 2016 at 11:17:57AM -0800, Stephen Hemminger wrote:
> > > > Support for the new rx_nohandler statistic.
> > > > This code is designed to handle the case where the kernel reported statistic
> > > > structure is smaller than the larger structure in later releases (and vice versa).
> > > 
> > > This seems to work here, for the most part. However, if you are running a
> > > kernel with the new counter, and the counter happens to contain 0, aren't
> > > we going to not print anything?
> > 
> > That is the desirable outcome, since if run on older system the
> > output format will not change from current format.
> 
> The problem here is that a change in output might break some user
> scripts using sed/whatever games.
> 
> So it might be better to output a zero field, so that such breakages are
> detected early, even if no packet was dropped at the time the new kernel
> was tested.
> 
> Having a binary that adds the new field only in some cases hides the
> change. It looks fine for us humans, but not for programs processing the
> output.

On my test setup, my bond's active interface currently has 0, while the
backup interface has a few thousand, so I can alternate back and forth
checking the interfaces, and one doesn't print the counter while the other
does, which is what seemed odd to me and prompted the added ugliness. But
most setups (anything outside of bond/team currently) should never have
this counter incremented, we do have prior art with the compressed fields,
and scripts really probably ought to be scraping stats out of sysfs rather
than using ip, so I can sort of understand not wanting the added ugliness.
I do tend to prefer consistency though.

-- 
Jarod Wilson
jarod@redhat.com

[toc] | [prev] | [next] | [standalone]

#1331256 — Re: [PATCH net-next iproute2] iplink: display rx nohandler stats

On Wed, Feb 10, 2016 at 08:20:59AM -0500, Jarod Wilson wrote:
> On Tue, Feb 09, 2016 at 08:52:38PM -0800, Eric Dumazet wrote:
> > On Tue, 2016-02-09 at 17:41 -0800, Stephen Hemminger wrote:
> > > On Tue, 9 Feb 2016 18:51:35 -0500
> > > Jarod Wilson <jarod@redhat.com> wrote:
> > > 
> > > > On Tue, Feb 09, 2016 at 11:17:57AM -0800, Stephen Hemminger wrote:
> > > > > Support for the new rx_nohandler statistic.
> > > > > This code is designed to handle the case where the kernel reported statistic
> > > > > structure is smaller than the larger structure in later releases (and vice versa).
> > > > 
> > > > This seems to work here, for the most part. However, if you are running a
> > > > kernel with the new counter, and the counter happens to contain 0, aren't
> > > > we going to not print anything?
> > > 
> > > That is the desirable outcome, since if run on older system the
> > > output format will not change from current format.
> > 
> > The problem here is that a change in output might break some user
> > scripts using sed/whatever games.
> > 
> > So it might be better to output a zero field, so that such breakages are
> > detected early, even if no packet was dropped at the time the new kernel
> > was tested.
> > 
> > Having a binary that adds the new field only in some cases hides the
> > change. It looks fine for us humans, but not for programs processing the
> > output.
> 
> On my test setup, my bond's active interface currently has 0, while the
> backup interface has a few thousand, so I can alternate back and forth
> checking the interfaces, and one doesn't print the counter while the other
> does, which is what seemed odd to me and prompted the added ugliness. But
> most setups (anything outside of bond/team currently) should never have
> this counter incremented, we do have prior art with the compressed fields,
> and scripts really probably ought to be scraping stats out of sysfs rather
> than using ip, so I can sort of understand not wanting the added ugliness.
> I do tend to prefer consistency though.

FWIW, I tend to agree with Jarod and Eric on this.  Consistency seems
better, even if 0 all the time.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web