Re: add an iqdrops view to systat to show interface queue drops

[Claudio Jeker]

On Thu, Nov 15, 2018 at 11:32:23AM +1000, David Gwynne wrote:
> On Fri, Nov 17, 2017 at 01:11:52PM -, Christian Weisgerber wrote:
> > On 2017-11-17, David Gwynne  wrote:
> > 
> > > can we have modified displays within a view?
> > 
> > We already have this for the ifstat view.
> > 
> >  The character B changes the counter view between bytes and
> >  bits.  Pressing b displays statistics as calculated from boot
> >  time.  r changes the counters to show their totals as
> >  calculated between display refreshes.  t changes the counters
> >  to show the average per second over the display refresh
> >  interval; this is the default.
> 
> So how about 'd' to show drops, and 'e' to show errors again? Or should
> I make a toggle like 'B' to switch between them?

As mentioned to you I think it is useful to see the drop counters.
I would not mind to have systat show the sum of all drops (ierrs +
iqdrops) for example. Having netstat -i show all counters would good.
The idea is you can use systat to see that packets are lost and netstat to
see why exactly they are lost.
I doubt people switch modes in systat (at least I never remember all the
magic key combos).
 
> Index: engine.h
> ===
> RCS file: /cvs/src/usr.bin/systat/engine.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 engine.h
> --- engine.h  8 Feb 2018 07:00:33 -   1.9
> +++ engine.h  15 Nov 2018 01:30:41 -
> @@ -53,7 +53,7 @@
>  
>  
>  typedef struct {
> - char *title;
> + const char *title;
>   int norm_width;
>   int max_width;
>   int increment;
> Index: if.c
> ===
> RCS file: /cvs/src/usr.bin/systat/if.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 if.c
> --- if.c  16 Jan 2015 00:03:37 -  1.23
> +++ if.c  15 Nov 2018 01:30:42 -
> @@ -56,6 +56,12 @@ static void showifstat(struct ifstat *);
>  static void showtotal(void);
>  static void rt_getaddrinfo(struct sockaddr *, int, struct sockaddr **);
>  
> +static const char ierrs[] = "IERRS";
> +static const char oerrs[] = "OERRS";
> +static const char iqdrops[] = "IQDROPS";
> +static const char oqdrops[] = "OQDROPS";
> +
> +static int show_errs = 1;
>  
>  /* Define fields */
>  field_def fields_if[] = {
> @@ -63,10 +69,10 @@ field_def fields_if[] = {
>   {"STATE", 4, 6, 1, FLD_ALIGN_LEFT, -1, 0, 0, 0},
>   {"IPKTS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
>   {"IBYTES", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
> - {"IERRS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
> + {ierrs, 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
>   {"OPKTS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
>   {"OBYTES", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
> - {"OERRS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
> + {oerrs, 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
>   {"COLLS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
>   {"DESC", 14, 64, 1, FLD_ALIGN_LEFT, -1, 0, 0, 0},
>  };
> @@ -264,9 +270,11 @@ fetchifstat(void)
>   UPDATE(ifc_ip, ifm_data.ifi_ipackets);
>   UPDATE(ifc_ib, ifm_data.ifi_ibytes);
>   UPDATE(ifc_ie, ifm_data.ifi_ierrors);
> + UPDATE(ifc_iq, ifm_data.ifi_iqdrops);
>   UPDATE(ifc_op, ifm_data.ifi_opackets);
>   UPDATE(ifc_ob, ifm_data.ifi_obytes);
>   UPDATE(ifc_oe, ifm_data.ifi_oerrors);
> + UPDATE(ifc_oq, ifm_data.ifi_oqdrops);
>   UPDATE(ifc_co, ifm_data.ifi_collisions);
>   ifs->ifs_cur.ifc_flags = ifm.ifm_flags;
>   ifs->ifs_cur.ifc_state = ifm.ifm_data.ifi_link_state;
> @@ -315,11 +323,13 @@ showifstat(struct ifstat *ifs)
>  
>   print_fld_sdiv(FLD_IF_IBYTES, ifs->ifs_cur.ifc_ib * conv, div);
>   print_fld_size(FLD_IF_IPKTS, ifs->ifs_cur.ifc_ip);
> - print_fld_size(FLD_IF_IERRS, ifs->ifs_cur.ifc_ie);
> + print_fld_size(FLD_IF_IERRS, show_errs ?
> + ifs->ifs_cur.ifc_ie : ifs->ifs_cur.ifc_iq);
>  
>   print_fld_sdiv(FLD_IF_OBYTES, ifs->ifs_cur.ifc_ob * conv, div);
>   print_fld_size(FLD_IF_OPKTS, ifs->ifs_cur.ifc_op);
> - print_fld_size(FLD_IF_OERRS, ifs->ifs_cur.ifc_oe);
> + print_fld_size(FLD_IF_OERRS, show_errs ?
> + ifs->ifs_cur.ifc_oe : ifs->ifs_cur.ifc_oq);
>  
>   print_fld_size(FLD_IF_COLLS, ifs->ifs_cur.ifc_co);
>  
> @@ -336,11 +346,11 @@ showtotal(void)
>  
>   print_fld_sdiv(FLD_IF_IBYTES, sum.ifc_ib * conv, div);
>   print_fld_size(FLD_IF_IPKTS, sum.ifc_ip);
> - print_fld_size(FLD_IF_IERRS, sum.ifc_ie);
> + print_fld_size(FLD_IF_IERRS, show_errs ? sum.ifc_ie : sum.ifc_iq);
>  
>   print_fld_sdiv(FLD_IF_OBYTES, sum.ifc_ob * conv, div);
>   print_fld_size(FLD_IF_OPKTS, sum.ifc_op);
> - print_fld_size(FLD_IF_OERRS, sum.ifc_oe);
> + print_fld_size(FLD_IF_OERRS, show_errs ? sum.ifc_oe : sum.ifc_oq);
>  
>   

Re: add an iqdrops view to systat to show interface queue drops

[David Gwynne]

On Fri, Nov 17, 2017 at 01:11:52PM -, Christian Weisgerber wrote:
> On 2017-11-17, David Gwynne  wrote:
> 
> > can we have modified displays within a view?
> 
> We already have this for the ifstat view.
> 
>  The character B changes the counter view between bytes and
>  bits.  Pressing b displays statistics as calculated from boot
>  time.  r changes the counters to show their totals as
>  calculated between display refreshes.  t changes the counters
>  to show the average per second over the display refresh
>  interval; this is the default.

So how about 'd' to show drops, and 'e' to show errors again? Or should
I make a toggle like 'B' to switch between them?

Index: engine.h
===
RCS file: /cvs/src/usr.bin/systat/engine.h,v
retrieving revision 1.9
diff -u -p -r1.9 engine.h
--- engine.h8 Feb 2018 07:00:33 -   1.9
+++ engine.h15 Nov 2018 01:30:41 -
@@ -53,7 +53,7 @@
 
 
 typedef struct {
-   char *title;
+   const char *title;
int norm_width;
int max_width;
int increment;
Index: if.c
===
RCS file: /cvs/src/usr.bin/systat/if.c,v
retrieving revision 1.23
diff -u -p -r1.23 if.c
--- if.c16 Jan 2015 00:03:37 -  1.23
+++ if.c15 Nov 2018 01:30:42 -
@@ -56,6 +56,12 @@ static void showifstat(struct ifstat *);
 static void showtotal(void);
 static void rt_getaddrinfo(struct sockaddr *, int, struct sockaddr **);
 
+static const char ierrs[] = "IERRS";
+static const char oerrs[] = "OERRS";
+static const char iqdrops[] = "IQDROPS";
+static const char oqdrops[] = "OQDROPS";
+
+static int show_errs = 1;
 
 /* Define fields */
 field_def fields_if[] = {
@@ -63,10 +69,10 @@ field_def fields_if[] = {
{"STATE", 4, 6, 1, FLD_ALIGN_LEFT, -1, 0, 0, 0},
{"IPKTS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
{"IBYTES", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
-   {"IERRS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
+   {ierrs, 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
{"OPKTS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
{"OBYTES", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
-   {"OERRS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
+   {oerrs, 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
{"COLLS", 5, 8, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
{"DESC", 14, 64, 1, FLD_ALIGN_LEFT, -1, 0, 0, 0},
 };
@@ -264,9 +270,11 @@ fetchifstat(void)
UPDATE(ifc_ip, ifm_data.ifi_ipackets);
UPDATE(ifc_ib, ifm_data.ifi_ibytes);
UPDATE(ifc_ie, ifm_data.ifi_ierrors);
+   UPDATE(ifc_iq, ifm_data.ifi_iqdrops);
UPDATE(ifc_op, ifm_data.ifi_opackets);
UPDATE(ifc_ob, ifm_data.ifi_obytes);
UPDATE(ifc_oe, ifm_data.ifi_oerrors);
+   UPDATE(ifc_oq, ifm_data.ifi_oqdrops);
UPDATE(ifc_co, ifm_data.ifi_collisions);
ifs->ifs_cur.ifc_flags = ifm.ifm_flags;
ifs->ifs_cur.ifc_state = ifm.ifm_data.ifi_link_state;
@@ -315,11 +323,13 @@ showifstat(struct ifstat *ifs)
 
print_fld_sdiv(FLD_IF_IBYTES, ifs->ifs_cur.ifc_ib * conv, div);
print_fld_size(FLD_IF_IPKTS, ifs->ifs_cur.ifc_ip);
-   print_fld_size(FLD_IF_IERRS, ifs->ifs_cur.ifc_ie);
+   print_fld_size(FLD_IF_IERRS, show_errs ?
+   ifs->ifs_cur.ifc_ie : ifs->ifs_cur.ifc_iq);
 
print_fld_sdiv(FLD_IF_OBYTES, ifs->ifs_cur.ifc_ob * conv, div);
print_fld_size(FLD_IF_OPKTS, ifs->ifs_cur.ifc_op);
-   print_fld_size(FLD_IF_OERRS, ifs->ifs_cur.ifc_oe);
+   print_fld_size(FLD_IF_OERRS, show_errs ?
+   ifs->ifs_cur.ifc_oe : ifs->ifs_cur.ifc_oq);
 
print_fld_size(FLD_IF_COLLS, ifs->ifs_cur.ifc_co);
 
@@ -336,11 +346,11 @@ showtotal(void)
 
print_fld_sdiv(FLD_IF_IBYTES, sum.ifc_ib * conv, div);
print_fld_size(FLD_IF_IPKTS, sum.ifc_ip);
-   print_fld_size(FLD_IF_IERRS, sum.ifc_ie);
+   print_fld_size(FLD_IF_IERRS, show_errs ? sum.ifc_ie : sum.ifc_iq);
 
print_fld_sdiv(FLD_IF_OBYTES, sum.ifc_ob * conv, div);
print_fld_size(FLD_IF_OPKTS, sum.ifc_op);
-   print_fld_size(FLD_IF_OERRS, sum.ifc_oe);
+   print_fld_size(FLD_IF_OERRS, show_errs ? sum.ifc_oe : sum.ifc_oq);
 
print_fld_size(FLD_IF_COLLS, sum.ifc_co);
 
@@ -354,6 +364,19 @@ if_keyboard_callback(int ch)
struct ifstat *ifs;
 
switch (ch) {
+   case 'd':
+   show_errs = 1;
+   FLD_IF_IERRS->title = iqdrops;
+   FLD_IF_OERRS->title = oqdrops;
+   gotsig_alarm = 1;
+   break;
+   case 'e':
+   show_errs = 0;
+   FLD_IF_IERRS->title = ierrs;
+   FLD_IF_OERRS->title = oerrs;
+   gotsig_alarm = 1;
+   break;
+
case 'r':
for (ifs = ifstats; 

Re: add an iqdrops view to systat to show interface queue drops

[Christian Weisgerber]

On 2017-11-17, David Gwynne  wrote:

> can we have modified displays within a view?

We already have this for the ifstat view.

 The character B changes the counter view between bytes and
 bits.  Pressing b displays statistics as calculated from boot
 time.  r changes the counters to show their totals as
 calculated between display refreshes.  t changes the counters
 to show the average per second over the display refresh
 interval; this is the default.

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Re: add an iqdrops view to systat to show interface queue drops

[David Gwynne]

> On 17 Nov 2017, at 05:39, Claudio Jeker  wrote:
> 
> On Thu, Nov 16, 2017 at 03:21:20PM +0100, Alexander Bluhm wrote:
>> On Wed, Nov 15, 2017 at 01:29:42PM +1000, David Gwynne wrote:
>>> im adding numbers to input and output qdrops in the kernel, so im
>>> aware that they exist now. however, i don't really see these values
>>> in userland. it seems netstat and systat think errors are more
>>> important.
>>> 
>>> i tried adding qdrops to the ifstat view, but it got too cluttered.
>>> so i made a new view called iqdrops that shows qdrops instead of
>>> errors. i used iqdrops instead of ifqdrops so "if" on its own is
>>> still not ambiguous.
>> 
>> Now ifstat and iqdrops show almost the same information.  Just two
>> commns are different, eight culumns are redundant.  I think one
>> page would be better.  Do we need DESC?  It takes a lot of space
>> that could be used for output of dynamic counters.

can we have modified displays within a view? kind of like how some views change 
their ordering.

i agree that everything should be on the ifstat page, but there really isnt 
enough space. if we can have tweaked displays, id like the main one to 
aggregate the qdrops and errs values into a single column, and then have 
separate displays within that view to show errors and drops as separate values.

> Would it make sense to extend the mbuf page instead? It has the ring size
> and so it may make sense to show drops there.

i dont think that would make sense for interfaces like vlan.

Re: add an iqdrops view to systat to show interface queue drops

[David Gwynne]

On Thu, Nov 16, 2017 at 07:22:48AM -0700, Theo de Raadt wrote:
> > Now ifstat and iqdrops show almost the same information.  Just two
> > commns are different, eight culumns are redundant.  I think one
> > page would be better.  Do we need DESC?  It takes a lot of space
> > that could be used for output of dynamic counters.
> 
> I use DESC every single day.

me too.

Re: add an iqdrops view to systat to show interface queue drops

[Claudio Jeker]

On Thu, Nov 16, 2017 at 03:21:20PM +0100, Alexander Bluhm wrote:
> On Wed, Nov 15, 2017 at 01:29:42PM +1000, David Gwynne wrote:
> > im adding numbers to input and output qdrops in the kernel, so im
> > aware that they exist now. however, i don't really see these values
> > in userland. it seems netstat and systat think errors are more
> > important.
> > 
> > i tried adding qdrops to the ifstat view, but it got too cluttered.
> > so i made a new view called iqdrops that shows qdrops instead of
> > errors. i used iqdrops instead of ifqdrops so "if" on its own is
> > still not ambiguous.
> 
> Now ifstat and iqdrops show almost the same information.  Just two
> commns are different, eight culumns are redundant.  I think one
> page would be better.  Do we need DESC?  It takes a lot of space
> that could be used for output of dynamic counters.
> 

Would it make sense to extend the mbuf page instead? It has the ring size
and so it may make sense to show drops there.

-- 
:wq Claudio

Re: add an iqdrops view to systat to show interface queue drops

[Theo de Raadt]

> Now ifstat and iqdrops show almost the same information.  Just two
> commns are different, eight culumns are redundant.  I think one
> page would be better.  Do we need DESC?  It takes a lot of space
> that could be used for output of dynamic counters.

I use DESC every single day.

Re: add an iqdrops view to systat to show interface queue drops

[Alexander Bluhm]

On Wed, Nov 15, 2017 at 01:29:42PM +1000, David Gwynne wrote:
> im adding numbers to input and output qdrops in the kernel, so im
> aware that they exist now. however, i don't really see these values
> in userland. it seems netstat and systat think errors are more
> important.
> 
> i tried adding qdrops to the ifstat view, but it got too cluttered.
> so i made a new view called iqdrops that shows qdrops instead of
> errors. i used iqdrops instead of ifqdrops so "if" on its own is
> still not ambiguous.

Now ifstat and iqdrops show almost the same information.  Just two
commns are different, eight culumns are redundant.  I think one
page would be better.  Do we need DESC?  It takes a lot of space
that could be used for output of dynamic counters.

bluhm