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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Feb 2020 11:17:56 -0700
From:   David Ahern <dsahern@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
        ilias.apalodimas@...aro.org, davem@...emloft.net,
        David Ahern <dsahern@...nel.org>,
        BPF-dev-list <bpf@...r.kernel.org>
Subject: Re: [PATCH net-next 4/5] net: mvneta: introduce xdp counters to
 ethtool

On 2/17/20 7:51 AM, Jesper Dangaard Brouer wrote:
> On Mon, 17 Feb 2020 14:05:15 +0100
> Andrew Lunn <andrew@...n.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@...hat.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.