Subject: Stats for XDP actions (was: Re: [PATCH net] virtio_net: Account for tx bytes and packets on sending xdp_frames)

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Thu, 18 Apr 2019 15:24:45 +0100

David Ahern <[email-masked]> writes:

> On 2/4/19 3:53 AM, Jesper Dangaard Brouer wrote:
>> On Sat, 2 Feb 2019 14:27:26 -0700
>> David Ahern <[email-masked]> wrote:
>> 
>>> On 1/31/19 1:15 PM, Jesper Dangaard Brouer wrote:
>>>>>
>>>>> David, Jesper, care to chime in where we ended up in that last thread
>>>>> discussion this?  
>>>>
>>>> IHMO packets RX and TX on a device need to be accounted, in standard
>>>> counters, regardless of XDP.  For XDP RX the packet is counted as RX,
>>>> regardless if XDP choose to XDP_DROP.  On XDP TX which is via
>>>> XDP_REDIRECT or XDP_TX, the driver that transmit the packet need to
>>>> account the packet in a TX counter (this if often delayed to DMA TX
>>>> completion handling).  We cannot break the expectation that RX and TX
>>>> counter are visible to userspace stats tools. XDP should not make these
>>>> packets invisible.  
>>>
>>> Agreed. What I was pushing on that last thread was Rx, Tx and dropped
>>> are all accounted by the driver in standard stats. Basically if the
>>> driver touched it, the driver's counters should indicate that.
>> 
>> Sound like we all agree (except with the dropped counter, see below).
>> 
>> Do notice that mlx5 driver doesn't do this.  It is actually rather
>> confusing to use XDP on mlx5, as when XDP "consume" which include
>> XDP_DROP, XDP_REDIRECT or XDP_TX, then the driver standard stats are
>> not incremented... the packet is invisible to "ifconfig" stat based
>> tools.
>
> mlx5 needs some work. As I recall it still has the bug/panic removing
> xdp programs - at least I don't recall seeing a patch for it.
>
>> 
>> 
>>> The push back was on dropped packets and whether that counter should be
>>> bumped on XDP_DROP.
>> 
>> My opinion is the XDP_DROP action should NOT increment the drivers drop
>> counter.  First of all the "dropped" counter is also use for other
>> stuff, which will confuse that this counter express.  Second, choosing
>> XDP_DROP is a policy choice, it still means it was RX-ed at the driver
>> level.
>> 
>
> Understood. Hopefully in March I will get some time to come back to this
> and propose an idea on what I would like to see - namely, the admin has
> a config option at load time to enable driver counters versus custom map
> counters. (meaning the operator of the node chooses standard stats over
> strict performance.) But of course that means the drivers have the code
> to collect those stats.

Hi David

I don't recall seeing any follow-up on this. Did you have a chance to
formulate your ideas? :)

-Toke