Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

[Jesper Dangaard Brouer]

On Sun, 16 Feb 2020 22:07:32 +0100
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> @@ -2033,6 +2050,7 @@ 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++;
>  	u64_stats_update_end(&stats->syncp);

I find it confusing that this ethtool stats is named "xdp_tx".
Because you use it as an "xmit" counter and not for the action XDP_TX.

Both XDP_TX and XDP_REDIRECT out this device will increment this
"xdp_tx" counter.  I don't think end-users will comprehend this...

What about naming it "xdp_xmit" ?


>  	mvneta_txq_inc_put(txq);
> @@ -2114,6 +2132,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
>  
>  	switch (act) {
>  	case XDP_PASS:
> +		stats->xdp_pass++;
>  		return MVNETA_XDP_PASS;
>  	case XDP_REDIRECT: {
>  		int err;
> @@ -2126,6 +2145,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
>  					     len, true);
>  		} else {
>  			ret = MVNETA_XDP_REDIR;
> +			stats->xdp_redirect++;
>  		}
>  		break;
>  	}
> @@ -2147,6 +2167,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
>  				     virt_to_head_page(xdp->data),
>  				     len, true);
>  		ret = MVNETA_XDP_DROPPED;
> +		stats->xdp_drop++;
>  		break;
>  	}


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

Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

[Lorenzo Bianconi]

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

> On Sun, 16 Feb 2020 22:07:32 +0100
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> > @@ -2033,6 +2050,7 @@ 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++;
> >  	u64_stats_update_end(&stats->syncp);
> 
> I find it confusing that this ethtool stats is named "xdp_tx".
> Because you use it as an "xmit" counter and not for the action XDP_TX.
> 
> Both XDP_TX and XDP_REDIRECT out this device will increment this
> "xdp_tx" counter.  I don't think end-users will comprehend this...
> 
> What about naming it "xdp_xmit" ?

Hi Jesper,

yes, I think it is definitely better. So to follow up:
- rename current "xdp_tx" counter in "xdp_xmit" and increment it for
  XDP_TX verdict and for ndo_xdp_xmit
- introduce a new "xdp_tx" counter only for XDP_TX verdict.

If we agree I can post a follow-up patch.

Regards,
Lorenzo

> 
> 
> >  	mvneta_txq_inc_put(txq);
> > @@ -2114,6 +2132,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
> >  
> >  	switch (act) {
> >  	case XDP_PASS:
> > +		stats->xdp_pass++;
> >  		return MVNETA_XDP_PASS;
> >  	case XDP_REDIRECT: {
> >  		int err;
> > @@ -2126,6 +2145,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
> >  					     len, true);
> >  		} else {
> >  			ret = MVNETA_XDP_REDIR;
> > +			stats->xdp_redirect++;
> >  		}
> >  		break;
> >  	}
> > @@ -2147,6 +2167,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
> >  				     virt_to_head_page(xdp->data),
> >  				     len, true);
> >  		ret = MVNETA_XDP_DROPPED;
> > +		stats->xdp_drop++;
> >  		break;
> >  	}
> 
> 
> -- 
> 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: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

[Jesper Dangaard Brouer]

On Mon, 17 Feb 2020 11:25:50 +0100
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:

> > On Sun, 16 Feb 2020 22:07:32 +0100
> > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >   
> > > @@ -2033,6 +2050,7 @@ 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++;
> > >  	u64_stats_update_end(&stats->syncp);  
> > 
> > I find it confusing that this ethtool stats is named "xdp_tx".
> > Because you use it as an "xmit" counter and not for the action XDP_TX.
> > 
> > Both XDP_TX and XDP_REDIRECT out this device will increment this
> > "xdp_tx" counter.  I don't think end-users will comprehend this...
> > 
> > What about naming it "xdp_xmit" ?  
> 
> Hi Jesper,
> 
> yes, I think it is definitely better. So to follow up:
> - rename current "xdp_tx" counter in "xdp_xmit" and increment it for
>   XDP_TX verdict and for ndo_xdp_xmit
> - introduce a new "xdp_tx" counter only for XDP_TX verdict.
> 
> If we agree I can post a follow-up patch.

I agree, that sounds like an improvement to this patchset.


I suspect David Ahern have some opinions about more general stats for
XDP, but that it is a more general discussion, that it outside this
patchset, but we should also have that discussion.

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

Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

[Andrew Lunn]

On Mon, Feb 17, 2020 at 11:32:09AM +0100, Jesper Dangaard Brouer wrote:
> On Mon, 17 Feb 2020 11:25:50 +0100
> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> 
> > > On Sun, 16 Feb 2020 22:07:32 +0100
> > > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > >   
> > > > @@ -2033,6 +2050,7 @@ 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++;
> > > >  	u64_stats_update_end(&stats->syncp);  
> > > 
> > > I find it confusing that this ethtool stats is named "xdp_tx".
> > > Because you use it as an "xmit" counter and not for the action XDP_TX.
> > > 
> > > Both XDP_TX and XDP_REDIRECT out this device will increment this
> > > "xdp_tx" counter.  I don't think end-users will comprehend this...
> > > 
> > > What about naming it "xdp_xmit" ?  
> > 
> > Hi Jesper,
> > 
> > yes, I think it is definitely better. So to follow up:
> > - rename current "xdp_tx" counter in "xdp_xmit" and increment it for
> >   XDP_TX verdict and for ndo_xdp_xmit
> > - introduce a new "xdp_tx" counter only for XDP_TX verdict.
> > 
> > If we agree I can post a follow-up patch.
> 
> I agree, that sounds like an improvement to this patchset.
> 
> 
> I suspect David Ahern have some opinions about more general stats for
> XDP, but that it is a more general discussion, that it outside this
> patchset, but we should also have that discussion.

Hi Jesper

I've not been following XDP too much, but xdp_xmit seems pretty
generic. It would be nice if all drivers used the same statistics
names. Less user confusion that way. So why is this outside of the
discussion?

	Andrew

Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

[Jesper Dangaard Brouer]

On Mon, 17 Feb 2020 14:05:15 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Feb 17, 2020 at 11:32:09AM +0100, Jesper Dangaard Brouer wrote:
> > On Mon, 17 Feb 2020 11:25:50 +0100
> > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> >   
[...]
> > > 
> > > yes, I think it is definitely better. So to follow up:
> > > - rename current "xdp_tx" counter in "xdp_xmit" and increment it for
> > >   XDP_TX verdict and for ndo_xdp_xmit
> > > - introduce a new "xdp_tx" counter only for XDP_TX verdict.
> > > 
> > > If we agree I can post a follow-up patch.  
> > 
> > I agree, that sounds like an improvement to this patchset.
> > 
> > 
> > I suspect David Ahern have some opinions about more general stats for
> > XDP, but that it is a more general discussion, that it outside this
> > patchset, but we should also have that discussion.  
> 
> Hi Jesper
> 
> I've not been following XDP too much, but xdp_xmit seems pretty
> generic. It would be nice if all drivers used the same statistics
> names. Less user confusion that way. So why is this outside of the
> discussion?

I do want to have this discussion, please.

I had hoped this patchset sparked this that discussion... maybe we can
have it despite this patchset already got applied?

My only request is that, if we don't revert, we fixup the "xdp_tx"
counter name.  It would make it easier for us[1] if we can keep them
applied, as we are preparing (asciinema) demos for [1].

That said, I think it is rather important to standardize on same
statistics names across drivers... which is an assignment that Lorenzo
have already signed up for [2].


[1] https://netdevconf.info/0x14/session.html?tutorial-add-XDP-support-to-a-NIC-driver
[2] https://github.com/xdp-project/xdp-project/blob/master/planning.org#consistency-for-statistics-with-xdp
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

[David Ahern]

On 2/17/20 7:51 AM, Jesper Dangaard Brouer wrote:
> On Mon, 17 Feb 2020 14:05:15 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>> On Mon, Feb 17, 2020 at 11:32:09AM +0100, Jesper Dangaard Brouer wrote:
>>> On Mon, 17 Feb 2020 11:25:50 +0100
>>> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
>>>   
> [...]
>>>>
>>>> yes, I think it is definitely better. So to follow up:
>>>> - rename current "xdp_tx" counter in "xdp_xmit" and increment it for
>>>>   XDP_TX verdict and for ndo_xdp_xmit
>>>> - introduce a new "xdp_tx" counter only for XDP_TX verdict.
>>>>
>>>> If we agree I can post a follow-up patch.  
>>>
>>> I agree, that sounds like an improvement to this patchset.
>>>
>>>
>>> I suspect David Ahern have some opinions about more general stats for
>>> XDP, but that it is a more general discussion, that it outside this
>>> patchset, but we should also have that discussion.  
>>
>> Hi Jesper
>>
>> I've not been following XDP too much, but xdp_xmit seems pretty
>> generic. It would be nice if all drivers used the same statistics
>> names. Less user confusion that way. So why is this outside of the
>> discussion?

Hi Andrew: I brought this up over a year ago - the need for some
consistency in XDP stats (names and meaning) across drivers:

https://lore.kernel.org/netdev/1d9a6548-4d1d-6624-e808-6ab0460a8655@gmail.com/

I don't have strong preferences on which driver is right in the current
naming, only that we have consistency. There has not been much progress
in the past 15 months, so I am glad to see someone take this on.

> 
> I do want to have this discussion, please.
> 
> I had hoped this patchset sparked this that discussion... maybe we can
> have it despite this patchset already got applied?
> 
> My only request is that, if we don't revert, we fixup the "xdp_tx"
> counter name.  It would make it easier for us[1] if we can keep them
> applied, as we are preparing (asciinema) demos for [1].

Jesper: what about the mlx5 naming scheme:

     rx_xdp_drop: 86468350180
     rx_xdp_redirect: 18860584
     rx_xdp_tx_xmit: 0

The rx prefix shows the xdp action is in the Rx path, and then the Tx
path has tx_xdp_xmit.

i40e seems to have something similar for the Rx path:
     rx-0.xdp.pass: 0
     rx-0.xdp.drop: 0
     rx-0.xdp.tx: 0
     rx-0.xdp.unknown: 0
     rx-0.xdp.redirect: 0
     rx-0.xdp.redirect_fail: 0

I don't see any Tx stats for xdp, but this is an older kernel so not
sure what 5.x has.

Looks like sfc has a similar naming scheme:
     rx_xdp_drops: 0
     rx_xdp_bad_drops: 0
     rx_xdp_tx: 0
     rx_xdp_redirect: 0

So if mvneta follows these 3, the names just need rx_ prepended.

Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

[David Miller]

From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date: Mon, 17 Feb 2020 11:25:50 +0100

> yes, I think it is definitely better. So to follow up:
> - rename current "xdp_tx" counter in "xdp_xmit" and increment it for
>   XDP_TX verdict and for ndo_xdp_xmit
> - introduce a new "xdp_tx" counter only for XDP_TX verdict.
> 
> If we agree I can post a follow-up patch.

What names do other drivers use?  Consistency is important.  I noticed
while reviewing these patches that mellanox drivers export similar
statistics in the exact same way.

Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to ethtool

[Lorenzo Bianconi]

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

> From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Date: Mon, 17 Feb 2020 11:25:50 +0100
> 
> > yes, I think it is definitely better. So to follow up:
> > - rename current "xdp_tx" counter in "xdp_xmit" and increment it for
> >   XDP_TX verdict and for ndo_xdp_xmit
> > - introduce a new "xdp_tx" counter only for XDP_TX verdict.
> > 
> > If we agree I can post a follow-up patch.
> 
> What names do other drivers use?  Consistency is important.  I noticed
> while reviewing these patches that mellanox drivers export similar
> statistics in the exact same way.

According to David's suggestion, I am following mellanox implementation
adding "rx_" prefix for xdp actions on rx path and, based on Jesper's comment,
I am differentiating between XDP_TX and ndo_xdp_xmit.
So, to follow up:

XDP_TX: rx_xdp_tx_xmit
XDP_DROP: rx_xdp_drop
XDP_PASS: rx_xdp_pass
XDP_REDIRECT: rx_xdp_redirect
ndo_xdp_xmit: tx_xdp_xmit

I will post a RFC patch soon.

Regards,
Lorenzo

> 

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