diff mbox series

[iproute2-next] ip: promote missed packets to the -s row

Message ID 20200916194249.505389-1-kuba@kernel.org
State New
Headers show
Series [iproute2-next] ip: promote missed packets to the -s row | expand

Commit Message

Jakub Kicinski Sept. 16, 2020, 7:42 p.m. UTC
missed_packet_errors are much more commonly reported:

linux$ git grep -c '[.>]rx_missed_errors ' -- drivers/ | wc -l
64
linux$ git grep -c '[.>]rx_over_errors ' -- drivers/ | wc -l
37

Plus those drivers are generally more modern than those
using rx_over_errors.

Since recently merged kernel documentation makes this
preference official, let's make ip -s output more informative
and let rx_missed_errors take the place of rx_over_errors.

Before:

2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
    RX: bytes  packets  errors  dropped overrun mcast
    6.04T      4.67G    0       0       0       67.7M
    RX errors: length   crc     frame   fifo    missed
               0        0       0       0       7
    TX: bytes  packets  errors  dropped carrier collsns
    3.13T      2.76G    0       0       0       0
    TX errors: aborted  fifo   window heartbeat transns
               0        0       0       0       6

After:

2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
    RX: bytes  packets  errors  dropped missed  mcast
    6.04T      4.67G    0       0       7       67.7M
    RX errors: length   crc     frame   fifo    overrun
               0        0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    3.13T      2.76G    0       0       0       0
    TX errors: aborted  fifo   window heartbeat transns
               0        0       0       0       6

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 ip/ipaddress.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

David Ahern Sept. 18, 2020, 3:44 p.m. UTC | #1
On 9/16/20 1:42 PM, Jakub Kicinski wrote:
> missed_packet_errors are much more commonly reported:
> 
> linux$ git grep -c '[.>]rx_missed_errors ' -- drivers/ | wc -l
> 64
> linux$ git grep -c '[.>]rx_over_errors ' -- drivers/ | wc -l
> 37
> 
> Plus those drivers are generally more modern than those
> using rx_over_errors.
> 
> Since recently merged kernel documentation makes this
> preference official, let's make ip -s output more informative
> and let rx_missed_errors take the place of rx_over_errors.
> 
> Before:
> 
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
>     RX: bytes  packets  errors  dropped overrun mcast
>     6.04T      4.67G    0       0       0       67.7M
>     RX errors: length   crc     frame   fifo    missed
>                0        0       0       0       7
>     TX: bytes  packets  errors  dropped carrier collsns
>     3.13T      2.76G    0       0       0       0
>     TX errors: aborted  fifo   window heartbeat transns
>                0        0       0       0       6
> 
> After:
> 
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
>     RX: bytes  packets  errors  dropped missed  mcast
>     6.04T      4.67G    0       0       7       67.7M
>     RX errors: length   crc     frame   fifo    overrun
>                0        0       0       0       0
>     TX: bytes  packets  errors  dropped carrier collsns
>     3.13T      2.76G    0       0       0       0
>     TX errors: aborted  fifo   window heartbeat transns
>                0        0       0       0       6

changes to ip output are usually not allowed.
Jakub Kicinski Sept. 18, 2020, 3:48 p.m. UTC | #2
On Fri, 18 Sep 2020 09:44:35 -0600 David Ahern wrote:
> On 9/16/20 1:42 PM, Jakub Kicinski wrote:
> > 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> >     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
> >     RX: bytes  packets  errors  dropped overrun mcast
> >     6.04T      4.67G    0       0       0       67.7M
> >     RX errors: length   crc     frame   fifo    missed
> >                0        0       0       0       7
> >     TX: bytes  packets  errors  dropped carrier collsns
> >     3.13T      2.76G    0       0       0       0
> >     TX errors: aborted  fifo   window heartbeat transns
> >                0        0       0       0       6
> > 
> > After:
> > 
> > 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> >     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
> >     RX: bytes  packets  errors  dropped missed  mcast
> >     6.04T      4.67G    0       0       7       67.7M
> >     RX errors: length   crc     frame   fifo    overrun
> >                0        0       0       0       0
> >     TX: bytes  packets  errors  dropped carrier collsns
> >     3.13T      2.76G    0       0       0       0
> >     TX errors: aborted  fifo   window heartbeat transns
> >                0        0       0       0       6  
> 
> changes to ip output are usually not allowed.

Does that mean "no" or "you need to be more convincing"? :)

JSON output is not changed. I don't think we care about screen
scrapers. If we cared about people how interpret values based 
on their position in the output we would break that with every
release, no?
David Ahern Sept. 19, 2020, 4:52 a.m. UTC | #3
On 9/18/20 9:48 AM, Jakub Kicinski wrote:
> On Fri, 18 Sep 2020 09:44:35 -0600 David Ahern wrote:
>> On 9/16/20 1:42 PM, Jakub Kicinski wrote:
>>> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>>>     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
>>>     RX: bytes  packets  errors  dropped overrun mcast
>>>     6.04T      4.67G    0       0       0       67.7M
>>>     RX errors: length   crc     frame   fifo    missed
>>>                0        0       0       0       7
>>>     TX: bytes  packets  errors  dropped carrier collsns
>>>     3.13T      2.76G    0       0       0       0
>>>     TX errors: aborted  fifo   window heartbeat transns
>>>                0        0       0       0       6
>>>
>>> After:
>>>
>>> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>>>     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
>>>     RX: bytes  packets  errors  dropped missed  mcast
>>>     6.04T      4.67G    0       0       7       67.7M
>>>     RX errors: length   crc     frame   fifo    overrun
>>>                0        0       0       0       0
>>>     TX: bytes  packets  errors  dropped carrier collsns
>>>     3.13T      2.76G    0       0       0       0
>>>     TX errors: aborted  fifo   window heartbeat transns
>>>                0        0       0       0       6  
>>
>> changes to ip output are usually not allowed.
> 
> Does that mean "no" or "you need to be more convincing"? :)
> 
> JSON output is not changed. I don't think we care about screen
> scrapers. If we cared about people how interpret values based 
> on their position in the output we would break that with every
> release, no?
> 

In this case you are not adding or inserting a new column, you are
changing the meaning of an existing column.

It's an 'error' stat so probably not as sensitive. I do not have a
strong religion on it since it seems to be making the error stat more up
to date.
Stephen Hemminger Sept. 19, 2020, 4:06 p.m. UTC | #4
On Fri, 18 Sep 2020 22:52:56 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 9/18/20 9:48 AM, Jakub Kicinski wrote:
> > On Fri, 18 Sep 2020 09:44:35 -0600 David Ahern wrote:  
> >> On 9/16/20 1:42 PM, Jakub Kicinski wrote:  
> >>> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> >>>     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
> >>>     RX: bytes  packets  errors  dropped overrun mcast
> >>>     6.04T      4.67G    0       0       0       67.7M
> >>>     RX errors: length   crc     frame   fifo    missed
> >>>                0        0       0       0       7
> >>>     TX: bytes  packets  errors  dropped carrier collsns
> >>>     3.13T      2.76G    0       0       0       0
> >>>     TX errors: aborted  fifo   window heartbeat transns
> >>>                0        0       0       0       6
> >>>
> >>> After:
> >>>
> >>> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> >>>     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff
> >>>     RX: bytes  packets  errors  dropped missed  mcast
> >>>     6.04T      4.67G    0       0       7       67.7M
> >>>     RX errors: length   crc     frame   fifo    overrun
> >>>                0        0       0       0       0
> >>>     TX: bytes  packets  errors  dropped carrier collsns
> >>>     3.13T      2.76G    0       0       0       0
> >>>     TX errors: aborted  fifo   window heartbeat transns
> >>>                0        0       0       0       6    
> >>
> >> changes to ip output are usually not allowed.  
> > 
> > Does that mean "no" or "you need to be more convincing"? :)
> > 
> > JSON output is not changed. I don't think we care about screen
> > scrapers. If we cared about people how interpret values based 
> > on their position in the output we would break that with every
> > release, no?
> >   
> 
> In this case you are not adding or inserting a new column, you are
> changing the meaning of an existing column.
> 
> It's an 'error' stat so probably not as sensitive. I do not have a
> strong religion on it since it seems to be making the error stat more up
> to date.

Is there any way to see the old error column at all?
Jakub Kicinski Sept. 21, 2020, 9:49 p.m. UTC | #5
On Sat, 19 Sep 2020 09:06:58 -0700 Stephen Hemminger wrote:
> > > Does that mean "no" or "you need to be more convincing"? :)
> > > 
> > > JSON output is not changed. I don't think we care about screen
> > > scrapers. If we cared about people how interpret values based 
> > > on their position in the output we would break that with every
> > > release, no?
> > 
> > In this case you are not adding or inserting a new column, you are
> > changing the meaning of an existing column.
> > 
> > It's an 'error' stat so probably not as sensitive. I do not have a
> > strong religion on it since it seems to be making the error stat more up
> > to date.  

I hope this change would encourage vendors to use the standard stats.
Because..

> Is there any way to see the old error column at all?

I had no idea how to even show error stats. I read the code multiple
times, only when I sat down to actually implement displaying did I
realize that -s -s (repeat the option) shows all the stats.
David Ahern Sept. 23, 2020, 2:24 a.m. UTC | #6
On 9/16/20 1:42 PM, Jakub Kicinski wrote:
> missed_packet_errors are much more commonly reported:

> 

> linux$ git grep -c '[.>]rx_missed_errors ' -- drivers/ | wc -l

> 64

> linux$ git grep -c '[.>]rx_over_errors ' -- drivers/ | wc -l

> 37

> 

> Plus those drivers are generally more modern than those

> using rx_over_errors.

> 

> Since recently merged kernel documentation makes this

> preference official, let's make ip -s output more informative

> and let rx_missed_errors take the place of rx_over_errors.

> 

> Before:

> 

> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000

>     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff

>     RX: bytes  packets  errors  dropped overrun mcast

>     6.04T      4.67G    0       0       0       67.7M

>     RX errors: length   crc     frame   fifo    missed

>                0        0       0       0       7

>     TX: bytes  packets  errors  dropped carrier collsns

>     3.13T      2.76G    0       0       0       0

>     TX errors: aborted  fifo   window heartbeat transns

>                0        0       0       0       6

> 

> After:

> 

> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000

>     link/ether 00:0a:f7:c1:4d:38 brd ff:ff:ff:ff:ff:ff

>     RX: bytes  packets  errors  dropped missed  mcast

>     6.04T      4.67G    0       0       7       67.7M

>     RX errors: length   crc     frame   fifo    overrun

>                0        0       0       0       0

>     TX: bytes  packets  errors  dropped carrier collsns

>     3.13T      2.76G    0       0       0       0

>     TX errors: aborted  fifo   window heartbeat transns

>                0        0       0       0       6

> 

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

> ---

>  ip/ipaddress.c | 8 ++++----

>  1 file changed, 4 insertions(+), 4 deletions(-)

> 


applied to iproute2-next
diff mbox series

Patch

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index ccf67d1dd55c..0822a8d2d792 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -744,7 +744,7 @@  static void __print_link_stats(FILE *fp, struct rtattr *tb[])
 		close_json_object();
 	} else {
 		/* RX stats */
-		fprintf(fp, "    RX: bytes  packets  errors  dropped overrun mcast   %s%s",
+		fprintf(fp, "    RX: bytes  packets  errors  dropped missed  mcast   %s%s",
 			s->rx_compressed ? "compressed" : "", _SL_);
 
 		fprintf(fp, "    ");
@@ -752,7 +752,7 @@  static void __print_link_stats(FILE *fp, struct rtattr *tb[])
 		print_num(fp, 8, s->rx_packets);
 		print_num(fp, 7, s->rx_errors);
 		print_num(fp, 7, s->rx_dropped);
-		print_num(fp, 7, s->rx_over_errors);
+		print_num(fp, 7, s->rx_missed_errors);
 		print_num(fp, 7, s->multicast);
 		if (s->rx_compressed)
 			print_num(fp, 7, s->rx_compressed);
@@ -760,14 +760,14 @@  static void __print_link_stats(FILE *fp, struct rtattr *tb[])
 		/* RX error stats */
 		if (show_stats > 1) {
 			fprintf(fp, "%s", _SL_);
-			fprintf(fp, "    RX errors: length   crc     frame   fifo    missed%s%s",
+			fprintf(fp, "    RX errors: length   crc     frame   fifo    overrun%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);
+			print_num(fp, 7, s->rx_over_errors);
 			if (s->rx_nohandler)
 				print_num(fp, 7, s->rx_nohandler);
 		}