Re: [RFC net-next] net: mvneta: align xdp stats naming scheme to mlx5 driver

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Tue, 18 Feb 2020 18:17:16 +0100
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > On Tue, 18 Feb 2020 01:14:29 +0100
> > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >   
> > > Introduce "rx" prefix in the name scheme for xdp counters
> > > on rx path.
> > > Differentiate between XDP_TX and ndo_xdp_xmit counters
> > > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/net/ethernet/marvell/mvneta.c | 22 +++++++++++++++++-----
> > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > > index b7045b6a15c2..6223700dc3df 100644
> > > --- a/drivers/net/ethernet/marvell/mvneta.c
> > > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > > @@ -344,6 +344,7 @@ enum {
> > >  	ETHTOOL_XDP_REDIRECT,
> > >  	ETHTOOL_XDP_PASS,
> > >  	ETHTOOL_XDP_DROP,
> > > +	ETHTOOL_XDP_XMIT,
> > >  	ETHTOOL_XDP_TX,
> > >  	ETHTOOL_MAX_STATS,
> > >  };
> > > @@ -399,10 +400,11 @@ static const struct mvneta_statistic mvneta_statistics[] = {
> > >  	{ ETHTOOL_STAT_EEE_WAKEUP, T_SW, "eee_wakeup_errors", },
> > >  	{ ETHTOOL_STAT_SKB_ALLOC_ERR, T_SW, "skb_alloc_errors", },
> > >  	{ ETHTOOL_STAT_REFILL_ERR, T_SW, "refill_errors", },
> > > -	{ ETHTOOL_XDP_REDIRECT, T_SW, "xdp_redirect", },
> > > -	{ ETHTOOL_XDP_PASS, T_SW, "xdp_pass", },
> > > -	{ ETHTOOL_XDP_DROP, T_SW, "xdp_drop", },
> > > -	{ ETHTOOL_XDP_TX, T_SW, "xdp_tx", },
> > > +	{ ETHTOOL_XDP_REDIRECT, T_SW, "rx_xdp_redirect", },
> > > +	{ ETHTOOL_XDP_PASS, T_SW, "rx_xdp_pass", },
> > > +	{ ETHTOOL_XDP_DROP, T_SW, "rx_xdp_drop", },
> > > +	{ ETHTOOL_XDP_TX, T_SW, "rx_xdp_tx_xmit", },  
> > 
> > Hmmm... "rx_xdp_tx_xmit", I expected this to be named "rx_xdp_tx" to
> > count the XDP_TX actions, but I guess this means something else.  
> 
> just reused mlx5 naming scheme here :)

Well, IMHO the naming in mlx5 should not automatically be seen as the
correct way ;-)
 
> >   
> > > +	{ ETHTOOL_XDP_XMIT, T_SW, "tx_xdp_xmit", },  
> > 
> > Okay, maybe.  I guess, this will still be valid for when we add an
> > XDP egress/TX-hook point.  
> 
> same here
> 
> >   
> > >  };
> > >  
> > >  struct mvneta_stats {
> > > @@ -414,6 +416,7 @@ struct mvneta_stats {
> > >  	u64	xdp_redirect;
> > >  	u64	xdp_pass;
> > >  	u64	xdp_drop;
> > > +	u64	xdp_xmit;
> > >  	u64	xdp_tx;
> > >  };
> > >  
> > > @@ -2050,7 +2053,10 @@ mvneta_xdp_submit_frame(struct mvneta_port
> > > *pp, struct mvneta_tx_queue *txq,
> > > u64_stats_update_begin(&stats->syncp); stats->es.ps.tx_bytes +=
> > > xdpf->len; stats->es.ps.tx_packets++;
> > > -	stats->es.ps.xdp_tx++;
> > > +	if (buf->type == MVNETA_TYPE_XDP_NDO)
> > > +		stats->es.ps.xdp_xmit++;
> > > +	else
> > > +		stats->es.ps.xdp_tx++;  
> > 
> > I don't like that you add a branch (if-statement) in this fast-path
> > code.
> > 
> > Do we really need to account in the xmit frame function, if this
> > was a XDP_REDIRECT or XDP_TX that started the xmit?  I mean we
> > already have action counters for XDP_REDIRECT and XDP_TX (but I
> > guess you skipped the XDP_TX action counter).   
> 
> ack, good point..I think we can move the code in
> mvneta_xdp_xmit_back/mvneta_xdp_xmit in order to avoid the if()
> condition. Moreover we can move it out the for loop in
> mvneta_xdp_xmit().

Sure, but I want the "xmit" counter (or what every we call it) to only
increment if the Ethernet frame was successfully queued. For me that is
an important property of the counter.  As I want a sysadm to be able to
use this counter to see if frames are getting dropped due to TX-queue
overflow by comparing/correlating counters.

This also begs the question: Should we have a counter for TX-queue
overflows?  That will make it even easier to diagnose problems from a
sysadm perspective?


> I will fix in a formal patch


> >   
> > >  	u64_stats_update_end(&stats->syncp);
> > >  
> > >  	mvneta_txq_inc_put(txq);
> > > @@ -4484,6 +4490,7 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
> > >  		u64 xdp_redirect;
> > >  		u64 xdp_pass;
> > >  		u64 xdp_drop;
> > > +		u64 xdp_xmit;
> > >  		u64 xdp_tx;
> > >  
> > >  		stats = per_cpu_ptr(pp->stats, cpu);
> > > @@ -4494,6 +4501,7 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
> > >  			xdp_redirect = stats->es.ps.xdp_redirect;
> > >  			xdp_pass = stats->es.ps.xdp_pass;
> > >  			xdp_drop = stats->es.ps.xdp_drop;
> > > +			xdp_xmit = stats->es.ps.xdp_xmit;
> > >  			xdp_tx = stats->es.ps.xdp_tx;
> > >  		} while (u64_stats_fetch_retry_irq(&stats->syncp, start));
> > >  
> > > @@ -4502,6 +4510,7 @@ mvneta_ethtool_update_pcpu_stats(struct mvneta_port *pp,
> > >  		es->ps.xdp_redirect += xdp_redirect;
> > >  		es->ps.xdp_pass += xdp_pass;
> > >  		es->ps.xdp_drop += xdp_drop;
> > > +		es->ps.xdp_xmit += xdp_xmit;
> > >  		es->ps.xdp_tx += xdp_tx;
> > >  	}
> > >  }
> > > @@ -4555,6 +4564,9 @@ static void mvneta_ethtool_update_stats(struct mvneta_port *pp)
> > >  			case ETHTOOL_XDP_TX:
> > >  				pp->ethtool_stats[i] = stats.ps.xdp_tx;
> > >  				break;
> > > +			case ETHTOOL_XDP_XMIT:
> > > +				pp->ethtool_stats[i] = stats.ps.xdp_xmit;
> > > +				break;
> > >  			}
> > >  			break;
> > >  		}  
> > 
> > It doesn't look like you have an action counter for XDP_TX, but we have
> > one for XDP_REDIRECT?  
> 
> I did not get you here sorry, I guess they should be accounted in two
> separated counters.

Checking code that got applied, you have xdp "action" counters for:
 - XDP_PASS     => stats->xdp_pass++;
 - XDP_REDIRECT => stats->xdp_redirect++ (on xdp_do_redirect == 0)
 - XDP_TX       => no-counter
 - XDP_ABORTED  => fall-through (to stats->xdp_drop++);
 - XDP_DROP     => stats->xdp_drop++

Notice the action XDP_TX is not accounted, that was my point.  While
all other XDP "actions" have a counter.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer