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

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

[Lorenzo Bianconi]

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", },
+	{ ETHTOOL_XDP_XMIT, T_SW, "tx_xdp_xmit", },
 };
 
 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++;
 	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;
 		}
-- 
2.24.1

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

[David Ahern]

On 2/17/20 5:14 PM, Lorenzo Bianconi wrote:
> @@ -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", },
> +	{ ETHTOOL_XDP_XMIT, T_SW, "tx_xdp_xmit", },
>  };
>  

LGTM

Acked-by: David Ahern <dsahern@gmail.com>

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

[Jesper Dangaard Brouer]

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.

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

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


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

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

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

[Lorenzo Bianconi]

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

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

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

Regards,
Lorenzo

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

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

[Jesper Dangaard Brouer]

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

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

[Lorenzo Bianconi]

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

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

[Jakub Kicinski]

On Tue, 18 Feb 2020 01:14:29 +0100 Lorenzo Bianconi 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>

Sorry for coming in late.

I thought the ability to attach a BPF program to a fexit of another BPF
program will put an end to these unnecessary statistics. IOW I maintain
my position that there should be no ethtool stats for XDP.

As discussed before real life BPF progs will maintain their own stats
at the granularity of their choosing, so we're just wasting datapath
cycles.

The previous argument that the BPF prog stats are out of admin control
is no longer true with the fexit option (IIUC how that works).

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

[Toke Høiland-Jørgensen]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 18 Feb 2020 01:14:29 +0100 Lorenzo Bianconi 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>
>
> Sorry for coming in late.
>
> I thought the ability to attach a BPF program to a fexit of another BPF
> program will put an end to these unnecessary statistics. IOW I maintain
> my position that there should be no ethtool stats for XDP.
>
> As discussed before real life BPF progs will maintain their own stats
> at the granularity of their choosing, so we're just wasting datapath
> cycles.
>
> The previous argument that the BPF prog stats are out of admin control
> is no longer true with the fexit option (IIUC how that works).

So you're proposing an admin that wants to keep track of XDP has to
(permantently?) attach an fexit program to every running XDP program and
use that to keep statistics? But presumably he'd first need to discover
that XDP is enabled; which the ethtool stats is a good hint for :)

-Toke

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

[Daniel Borkmann]

On 2/18/20 11:23 PM, Toke Høiland-Jørgensen wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
>> On Tue, 18 Feb 2020 01:14:29 +0100 Lorenzo Bianconi 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>
>>
>> Sorry for coming in late.
>>
>> I thought the ability to attach a BPF program to a fexit of another BPF
>> program will put an end to these unnecessary statistics. IOW I maintain
>> my position that there should be no ethtool stats for XDP.
>>
>> As discussed before real life BPF progs will maintain their own stats
>> at the granularity of their choosing, so we're just wasting datapath
>> cycles.

+1

>> The previous argument that the BPF prog stats are out of admin control
>> is no longer true with the fexit option (IIUC how that works).
> 
> So you're proposing an admin that wants to keep track of XDP has to
> (permantently?) attach an fexit program to every running XDP program and
> use that to keep statistics? But presumably he'd first need to discover
> that XDP is enabled; which the ethtool stats is a good hint for :)

Doesn't iproute2 clearly show that already via `ip l` that XDP is attached?

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

[David Ahern]

On 2/18/20 4:19 PM, Daniel Borkmann wrote:
> On 2/18/20 11:23 PM, Toke Høiland-Jørgensen wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>>> On Tue, 18 Feb 2020 01:14:29 +0100 Lorenzo Bianconi 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>
>>>
>>> Sorry for coming in late.
>>>
>>> I thought the ability to attach a BPF program to a fexit of another BPF
>>> program will put an end to these unnecessary statistics. IOW I maintain
>>> my position that there should be no ethtool stats for XDP.
>>>
>>> As discussed before real life BPF progs will maintain their own stats
>>> at the granularity of their choosing, so we're just wasting datapath
>>> cycles.
> 
> +1

There is value in having something as essential as stats available
through standard APIs and tooling. mlx5, i40e, sfc, etc, etc already
provide stats via ethtool. This patch is making mvneta consistent with
existing stats from these other drivers.

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

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Tue, 18 Feb 2020 16:24:13 -0700

> There is value in having something as essential as stats available
> through standard APIs and tooling. mlx5, i40e, sfc, etc, etc already
> provide stats via ethtool. This patch is making mvneta consistent with
> existing stats from these other drivers.

I completely agree.

It is the right way to go and we have precedence.

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

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 19 Feb 2020 00:19:36 +0100

> On 2/18/20 11:23 PM, Toke Høiland-Jørgensen wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>>> On Tue, 18 Feb 2020 01:14:29 +0100 Lorenzo Bianconi 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>
>>>
>>> Sorry for coming in late.
>>>
>>> I thought the ability to attach a BPF program to a fexit of another
>>> BPF
>>> program will put an end to these unnecessary statistics. IOW I
>>> maintain
>>> my position that there should be no ethtool stats for XDP.
>>>
>>> As discussed before real life BPF progs will maintain their own stats
>>> at the granularity of their choosing, so we're just wasting datapath
>>> cycles.
> 
> +1

Bugs in bpf programs leading to lack of fundamental statistics.

I think that is absolutely the wrong tradeoff.

And if performance is a concern, we can have a knob to turn off
the xdp counter bumps.  This is something I totally support.

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

[David Miller]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Tue, 18 Feb 2020 23:23:22 +0100

> Jakub Kicinski <kuba@kernel.org> writes:
> 
>> On Tue, 18 Feb 2020 01:14:29 +0100 Lorenzo Bianconi 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>
>>
>> Sorry for coming in late.
>>
>> I thought the ability to attach a BPF program to a fexit of another BPF
>> program will put an end to these unnecessary statistics. IOW I maintain
>> my position that there should be no ethtool stats for XDP.
>>
>> As discussed before real life BPF progs will maintain their own stats
>> at the granularity of their choosing, so we're just wasting datapath
>> cycles.
>>
>> The previous argument that the BPF prog stats are out of admin control
>> is no longer true with the fexit option (IIUC how that works).
> 
> So you're proposing an admin that wants to keep track of XDP has to
> (permantently?) attach an fexit program to every running XDP program and
> use that to keep statistics? But presumably he'd first need to discover
> that XDP is enabled; which the ethtool stats is a good hint for :)

Really, mistakes happen and a poorly implemented or inserted fexit
module should not be a reason to not have access to accurate and
working statistics for fundamental events.

I am therefore totally against requiring fexit for this functionality.
If you want more sophisticated events or custome ones, sure, but not
for this baseline stuff.

I do, however, think we need a way to turn off these counter bumps if
the user wishes to do so for maximum performance.

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

[Jakub Kicinski]

On Tue, 18 Feb 2020 15:47:13 -0800 (PST) David Miller wrote:
> Really, mistakes happen and a poorly implemented or inserted fexit
> module should not be a reason to not have access to accurate and
> working statistics for fundamental events.
> 
> I am therefore totally against requiring fexit for this functionality.
> If you want more sophisticated events or custome ones, sure, but not
> for this baseline stuff.
> 
> I do, however, think we need a way to turn off these counter bumps if
> the user wishes to do so for maximum performance.

Yes, this point plus the precedence you mentioned elsewhere are quite
hard to contend with.

In an ideal world I was wondering if we could have the kernel install
the fexit hook, a'la what we do with drop monitor using tracepoints
from within the kernel.

Then have a proper netlink stats group for them, instead of the ethtool
free-form endlessly bikesheddable strings.

But I guess it could be hard to easily recover the source interface
pointer without digging through NAPI instances or such :S

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

[Jesper Dangaard Brouer]

On Tue, 18 Feb 2020 15:47:13 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> Date: Tue, 18 Feb 2020 23:23:22 +0100
> 
> > Jakub Kicinski <kuba@kernel.org> writes:
> >   
> >> On Tue, 18 Feb 2020 01:14:29 +0100 Lorenzo Bianconi 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>  
> >>
> >> Sorry for coming in late.
> >>
> >> I thought the ability to attach a BPF program to a fexit of another BPF
> >> program will put an end to these unnecessary statistics. IOW I maintain
> >> my position that there should be no ethtool stats for XDP.
> >>
> >> As discussed before real life BPF progs will maintain their own stats
> >> at the granularity of their choosing, so we're just wasting datapath
> >> cycles.

Well, in practice we see that real-life[1] BPF progs don't maintain
stats (as I agree they _should_), and an end-user of this showed up on
XDP-newbies list, and I helped out, going in the complete wrong
direction, when it was simply the XDP prog dropping these packets, due
to builtin rate limiter.  It would have been so much easier to identify
via a simple counter, that I could have asked for from the sysadm.

[1] https://gitlab.com/Dreae/compressor/

> >>
> >> The previous argument that the BPF prog stats are out of admin control
> >> is no longer true with the fexit option (IIUC how that works).  
> > 
> > So you're proposing an admin that wants to keep track of XDP has to
> > (permantently?) attach an fexit program to every running XDP program and
> > use that to keep statistics? But presumably he'd first need to discover
> > that XDP is enabled; which the ethtool stats is a good hint for :)  
> 
> Really, mistakes happen and a poorly implemented or inserted fexit
> module should not be a reason to not have access to accurate and
> working statistics for fundamental events.

Yes, exactly.  These statistics counters are only "basic" XDP events,
that e.g. don't count the bytes.  They are only the first level of
identifying what the system is doing.  When digging deeper we need
tracepoint and fexit.

> I am therefore totally against requiring fexit for this functionality.
> If you want more sophisticated events or custome ones, sure, but not
> for this baseline stuff.

I fully agree.

> I do, however, think we need a way to turn off these counter bumps if
> the user wishes to do so for maximum performance.

I sort of agree, but having a mechanism to turn on/off these "basic"
counters might cost more than just always having them always on.  Even
the static_key infra will create sub-optimal code, which can throw-off
the advantage.

Maybe it is worth pointing out, that Lorenzo's code is doing something
smart, which lowers the overhead.  The stats struct (mvneta_stats) that
is passed to mvneta_run_xdp is not global, it only counts events in
this NAPI cycle, and is first transferred to the global counters when
drivers NAPI functions end, calling mvneta_update_stats().  (We can
optimized this a bit more on this HW as it is not necessary to have u64
long counters for these temp/non-global stats).

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