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

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6824 bytes --]

> 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 ;-)

sure, I have no prefernces actually :)

>  
> > >   
> > > > +	{ 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.

yes, it is just a matter of using "num_frame - drops" as counter in
mvneta_xdp_xmit()

> 
> 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?

not yet. Do you want to add it?

> 
> 
> > 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

nope, we have a counter for it...it is "rx_xdp_tx_xmit".
Moreover we have "tx_xdp_xmit" for ndo_xdp_xmit

- XDP_TX -> stats->xdp_tx++
- ndo_xdp_xmit -> stats->xdp_xmit++

Regards,
Lorenzo

>  - 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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]