| Message ID | 20220509191810.2157940-1-jeffreyjilinux@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [net-next] show rx_otherhost_dropped stat in ip link show | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
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 >
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 >
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 > >
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.
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.
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) {