[PATCH net-next] show rx_otherhost_dropped stat in ip link show

[PATCH net-next] show rx_otherhost_dropped stat in ip link show

[Jeffrey Ji]

From: Jeffrey Ji <jeffreyji@google.com>

This stat was added in commit 794c24e9921f ("net-core: rx_otherhost_dropped to core_stats")

Tested: sent packet with wrong MAC address from 1
network namespace to another, verified that counter showed "1" in
`ip -s -s link sh` and `ip -s -s -j link sh`

Signed-off-by: Jeffrey Ji <jeffreyji@google.com>
---
 include/uapi/linux/if_link.h |  2 ++
 ip/ipaddress.c               | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 22e21e57afc9..50477985bfea 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -243,6 +243,8 @@ struct rtnl_link_stats64 {
 	__u64	rx_compressed;
 	__u64	tx_compressed;
 	__u64	rx_nohandler;
+
+	__u64	rx_otherhost_dropped;
 };
 
 /* Subset of link stats useful for in-HW collection. Meaning of the fields is as
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index a80996efdc28..9d6af56e2a72 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -692,6 +692,7 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
 		strlen("heartbt"),
 		strlen("overrun"),
 		strlen("compressed"),
+		strlen("otherhost_dropped"),
 	};
 	int ret;
 
@@ -713,6 +714,10 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
 		if (s->rx_compressed)
 			print_u64(PRINT_JSON,
 				   "compressed", NULL, s->rx_compressed);
+		if (s->rx_otherhost_dropped)
+			print_u64(PRINT_JSON,
+				   "otherhost_dropped",
+				   NULL, s->rx_otherhost_dropped);
 
 		/* RX error stats */
 		if (show_stats > 1) {
@@ -795,11 +800,15 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
 				     rta_getattr_u32(carrier_changes) : 0);
 
 		/* RX stats */
-		fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%s",
+		fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%*s%s",
 			cols[0] - 4, "bytes", cols[1], "packets",
 			cols[2], "errors", cols[3], "dropped",
 			cols[4], "missed", cols[5], "mcast",
-			cols[6], s->rx_compressed ? "compressed" : "", _SL_);
+			s->rx_compressed ? cols[6] : 0,
+			s->rx_compressed ? "compressed " : "",
+			s->rx_otherhost_dropped ? cols[7] : 0,
+			s->rx_otherhost_dropped ? "otherhost_dropped" : "",
+			_SL_);
 
 		fprintf(fp, "    ");
 		print_num(fp, cols[0], s->rx_bytes);
@@ -810,6 +819,8 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
 		print_num(fp, cols[5], s->multicast);
 		if (s->rx_compressed)
 			print_num(fp, cols[6], s->rx_compressed);
+		if (s->rx_otherhost_dropped)
+			print_num(fp, cols[7], s->rx_otherhost_dropped);
 
 		/* RX error stats */
 		if (show_stats > 1) {
-- 
2.36.0.512.ge40c2bad7a-goog

Re: [PATCH net-next] show rx_otherhost_dropped stat in ip link show

[Brian Vazquez]

Hey Jeffrey, thanks for working on this, some comments:

I think you meant iproute2-next in the subject. Could you resend, please?

On Mon, May 9, 2022 at 12:18 PM Jeffrey Ji <jeffreyjilinux@gmail.com> wrote:
>
> From: Jeffrey Ji <jeffreyji@google.com>
>
> This stat was added in commit 794c24e9921f ("net-core: rx_otherhost_dropped to core_stats")
>
> Tested: sent packet with wrong MAC address from 1
> network namespace to another, verified that counter showed "1" in
> `ip -s -s link sh` and `ip -s -s -j link sh`
>
> Signed-off-by: Jeffrey Ji <jeffreyji@google.com>
> ---
>  include/uapi/linux/if_link.h |  2 ++
>  ip/ipaddress.c               | 15 +++++++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 22e21e57afc9..50477985bfea 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -243,6 +243,8 @@ struct rtnl_link_stats64 {
>         __u64   rx_compressed;
>         __u64   tx_compressed;
>         __u64   rx_nohandler;
> +
> +       __u64   rx_otherhost_dropped;
>  };
>
>  /* Subset of link stats useful for in-HW collection. Meaning of the fields is as
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index a80996efdc28..9d6af56e2a72 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -692,6 +692,7 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
>                 strlen("heartbt"),
>                 strlen("overrun"),
>                 strlen("compressed"),
> +               strlen("otherhost_dropped"),
>         };
>         int ret;
>
> @@ -713,6 +714,10 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
>                 if (s->rx_compressed)
>                         print_u64(PRINT_JSON,
>                                    "compressed", NULL, s->rx_compressed);
> +               if (s->rx_otherhost_dropped)
> +                       print_u64(PRINT_JSON,
> +                                  "otherhost_dropped",
> +                                  NULL, s->rx_otherhost_dropped);
>
>                 /* RX error stats */
>                 if (show_stats > 1) {
> @@ -795,11 +800,15 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
>                                      rta_getattr_u32(carrier_changes) : 0);
>
>                 /* RX stats */
> -               fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%s",
> +               fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%*s%s",
I think you're missing a space in the line above (but code shouldn't
be here, see my comment below)
>                         cols[0] - 4, "bytes", cols[1], "packets",
>                         cols[2], "errors", cols[3], "dropped",
>                         cols[4], "missed", cols[5], "mcast",
> -                       cols[6], s->rx_compressed ? "compressed" : "", _SL_);
> +                       s->rx_compressed ? cols[6] : 0,
> +                       s->rx_compressed ? "compressed " : "",
> +                       s->rx_otherhost_dropped ? cols[7] : 0,
> +                       s->rx_otherhost_dropped ? "otherhost_dropped" : "",
> +                       _SL_);
rx_otherhost_dropped code should be below in the "RX error stats" not
here, it should be after the rx_nohandler stat. Also IIUC, the code
should be:
  cols[7], s->rx_otherhost_dropped? "otherhost_dropped" : "",

>
>                 fprintf(fp, "    ");
>                 print_num(fp, cols[0], s->rx_bytes);
> @@ -810,6 +819,8 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
>                 print_num(fp, cols[5], s->multicast);
>                 if (s->rx_compressed)
>                         print_num(fp, cols[6], s->rx_compressed);
> +               if (s->rx_otherhost_dropped)
> +                       print_num(fp, cols[7], s->rx_otherhost_dropped);
>
>                 /* RX error stats */
>                 if (show_stats > 1) {
> --
> 2.36.0.512.ge40c2bad7a-goog
>

Re: [PATCH net-next] show rx_otherhost_dropped stat in ip link show

[Ido Schimmel]

On Mon, May 09, 2022 at 07:18:10PM +0000, Jeffrey Ji wrote:
> From: Jeffrey Ji <jeffreyji@google.com>
> 
> This stat was added in commit 794c24e9921f ("net-core: rx_otherhost_dropped to core_stats")
> 
> Tested: sent packet with wrong MAC address from 1
> network namespace to another, verified that counter showed "1" in
> `ip -s -s link sh` and `ip -s -s -j link sh`
> 
> Signed-off-by: Jeffrey Ji <jeffreyji@google.com>
> ---
>  include/uapi/linux/if_link.h |  2 ++
>  ip/ipaddress.c               | 15 +++++++++++++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 22e21e57afc9..50477985bfea 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -243,6 +243,8 @@ struct rtnl_link_stats64 {
>  	__u64	rx_compressed;
>  	__u64	tx_compressed;
>  	__u64	rx_nohandler;
> +
> +	__u64	rx_otherhost_dropped;

I believe you need to rebase against current iproute2-next. The kernel
headers are already updated there. This tree:
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git

>  };
>  
>  /* Subset of link stats useful for in-HW collection. Meaning of the fields is as
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index a80996efdc28..9d6af56e2a72 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -692,6 +692,7 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
>  		strlen("heartbt"),
>  		strlen("overrun"),
>  		strlen("compressed"),
> +		strlen("otherhost_dropped"),

There were a lot of changes in this area as part of the "ip stats"
work. See print_stats64() in current iproute2-next.

>  	};
>  	int ret;
>  
> @@ -713,6 +714,10 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
>  		if (s->rx_compressed)
>  			print_u64(PRINT_JSON,
>  				   "compressed", NULL, s->rx_compressed);
> +		if (s->rx_otherhost_dropped)
> +			print_u64(PRINT_JSON,
> +				   "otherhost_dropped",
> +				   NULL, s->rx_otherhost_dropped);
>  
>  		/* RX error stats */
>  		if (show_stats > 1) {
> @@ -795,11 +800,15 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
>  				     rta_getattr_u32(carrier_changes) : 0);
>  
>  		/* RX stats */
> -		fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%s",
> +		fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%*s%s",
>  			cols[0] - 4, "bytes", cols[1], "packets",
>  			cols[2], "errors", cols[3], "dropped",
>  			cols[4], "missed", cols[5], "mcast",
> -			cols[6], s->rx_compressed ? "compressed" : "", _SL_);
> +			s->rx_compressed ? cols[6] : 0,
> +			s->rx_compressed ? "compressed " : "",
> +			s->rx_otherhost_dropped ? cols[7] : 0,
> +			s->rx_otherhost_dropped ? "otherhost_dropped" : "",
> +			_SL_);
>  
>  		fprintf(fp, "    ");
>  		print_num(fp, cols[0], s->rx_bytes);
> @@ -810,6 +819,8 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
>  		print_num(fp, cols[5], s->multicast);
>  		if (s->rx_compressed)
>  			print_num(fp, cols[6], s->rx_compressed);
> +		if (s->rx_otherhost_dropped)
> +			print_num(fp, cols[7], s->rx_otherhost_dropped);
>  
>  		/* RX error stats */
>  		if (show_stats > 1) {
> -- 
> 2.36.0.512.ge40c2bad7a-goog
> 

Re: [PATCH net-next] show rx_otherhost_dropped stat in ip link show

[Jeffrey Ji]

I thought we wanted to avoid otherhost_dropped being counted as an
error, I recall Jakub saying something about not wanting users to call
for help when they see error in the counter name.


On Mon, May 9, 2022 at 9:09 PM Ido Schimmel <idosch@idosch.org> wrote:
>
> On Mon, May 09, 2022 at 07:18:10PM +0000, Jeffrey Ji wrote:
> > From: Jeffrey Ji <jeffreyji@google.com>
> >
> > This stat was added in commit 794c24e9921f ("net-core: rx_otherhost_dropped to core_stats")
> >
> > Tested: sent packet with wrong MAC address from 1
> > network namespace to another, verified that counter showed "1" in
> > `ip -s -s link sh` and `ip -s -s -j link sh`
> >
> > Signed-off-by: Jeffrey Ji <jeffreyji@google.com>
> > ---
> >  include/uapi/linux/if_link.h |  2 ++
> >  ip/ipaddress.c               | 15 +++++++++++++--
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> > index 22e21e57afc9..50477985bfea 100644
> > --- a/include/uapi/linux/if_link.h
> > +++ b/include/uapi/linux/if_link.h
> > @@ -243,6 +243,8 @@ struct rtnl_link_stats64 {
> >       __u64   rx_compressed;
> >       __u64   tx_compressed;
> >       __u64   rx_nohandler;
> > +
> > +     __u64   rx_otherhost_dropped;
>
> I believe you need to rebase against current iproute2-next. The kernel
> headers are already updated there. This tree:
> https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git
>
> >  };
> >
> >  /* Subset of link stats useful for in-HW collection. Meaning of the fields is as
> > diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> > index a80996efdc28..9d6af56e2a72 100644
> > --- a/ip/ipaddress.c
> > +++ b/ip/ipaddress.c
> > @@ -692,6 +692,7 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
> >               strlen("heartbt"),
> >               strlen("overrun"),
> >               strlen("compressed"),
> > +             strlen("otherhost_dropped"),
>
> There were a lot of changes in this area as part of the "ip stats"
> work. See print_stats64() in current iproute2-next.
>
> >       };
> >       int ret;
> >
> > @@ -713,6 +714,10 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
> >               if (s->rx_compressed)
> >                       print_u64(PRINT_JSON,
> >                                  "compressed", NULL, s->rx_compressed);
> > +             if (s->rx_otherhost_dropped)
> > +                     print_u64(PRINT_JSON,
> > +                                "otherhost_dropped",
> > +                                NULL, s->rx_otherhost_dropped);
> >
> >               /* RX error stats */
> >               if (show_stats > 1) {
> > @@ -795,11 +800,15 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
> >                                    rta_getattr_u32(carrier_changes) : 0);
> >
> >               /* RX stats */
> > -             fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%s",
> > +             fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%*s%s",
> >                       cols[0] - 4, "bytes", cols[1], "packets",
> >                       cols[2], "errors", cols[3], "dropped",
> >                       cols[4], "missed", cols[5], "mcast",
> > -                     cols[6], s->rx_compressed ? "compressed" : "", _SL_);
> > +                     s->rx_compressed ? cols[6] : 0,
> > +                     s->rx_compressed ? "compressed " : "",
> > +                     s->rx_otherhost_dropped ? cols[7] : 0,
> > +                     s->rx_otherhost_dropped ? "otherhost_dropped" : "",
> > +                     _SL_);
> >
> >               fprintf(fp, "    ");
> >               print_num(fp, cols[0], s->rx_bytes);
> > @@ -810,6 +819,8 @@ static void __print_link_stats(FILE *fp, struct rtattr *tb[])
> >               print_num(fp, cols[5], s->multicast);
> >               if (s->rx_compressed)
> >                       print_num(fp, cols[6], s->rx_compressed);
> > +             if (s->rx_otherhost_dropped)
> > +                     print_num(fp, cols[7], s->rx_otherhost_dropped);
> >
> >               /* RX error stats */
> >               if (show_stats > 1) {
> > --
> > 2.36.0.512.ge40c2bad7a-goog
> >

Re: [PATCH net-next] show rx_otherhost_dropped stat in ip link show

[Jakub Kicinski]

On Wed, 18 May 2022 07:29:08 -1000 Jeffrey Ji wrote:
> I thought we wanted to avoid otherhost_dropped being counted as an
> error, I recall Jakub saying something about not wanting users to call
> for help when they see error in the counter name.

You should probably set up a normal email client if you want to
continue working on the kernel, you replied to the wrong message :S

Yes, displaying the otherhost as an error could be too alarming.
That said the split between the main RX: line and RX errors: in
ip -s -s is somewhat arbitrary so no strong preference here.

Re: [PATCH net-next] show rx_otherhost_dropped stat in ip link show

[Stephen Hemminger]

On Wed, 18 May 2022 07:29:08 -1000
Jeffrey Ji <jeffreyjilinux@gmail.com> wrote:

> > >               /* RX stats */
> > > -             fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%s",
> > > +             fprintf(fp, "    RX: %*s %*s %*s %*s %*s %*s %*s%*s%s",
> > >                       cols[0] - 4, "bytes", cols[1], "packets",
> > >                       cols[2], "errors", cols[3], "dropped",
> > >                       cols[4], "missed", cols[5], "mcast",
> > > -                     cols[6], s->rx_compressed ? "compressed" : "", _SL_);
> > > +                     s->rx_compressed ? cols[6] : 0,
> > > +                     s->rx_compressed ? "compressed " : "",
> > > +                     s->rx_otherhost_dropped ? cols[7] : 0,
> > > +                     s->rx_otherhost_dropped ? "otherhost_dropped" : "",

This belongs in the detail part not in the common stats.

Look where nohandler etc errors are.