Message-ID: <140ecbe1e25f54f90d859cc696c4119aa96bc6eb.camel@mellanox.com>
Date:   Wed, 6 Feb 2019 00:06:33 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "dsahern@...il.com" <dsahern@...il.com>,
        "brouer@...hat.com" <brouer@...hat.com>
CC:     "thoiland@...hat.com" <thoiland@...hat.com>,
        "hawk@...nel.org" <hawk@...nel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "borkmann@...earbox.net" <borkmann@...earbox.net>,
        Tariq Toukan <tariqt@...lanox.com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "mst@...hat.com" <mst@...hat.com>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "makita.toshiaki@....ntt.co.jp" <makita.toshiaki@....ntt.co.jp>
Subject: Re: [PATCH net] virtio_net: Account for tx bytes and packets on
 sending xdp_frames

On Mon, 2019-02-04 at 19:13 -0800, David Ahern wrote:
> On 2/4/19 3:53 AM, Jesper Dangaard Brouer wrote:
> > On Sat, 2 Feb 2019 14:27:26 -0700
> > David Ahern <dsahern@...il.com> 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.

Only when xdp_redirect to mlx5, and removing the program while redirect
is happening, this is actually due to a lack of synchronization means
between different drivers, we have some ideas to overcome this using a
standard XDP API, or just use a hack in mlx5 driver which i don't like:

https://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git/commit/?h=topic/xdp-redirect-fix&id=a3652d03cc35fd3ad62744986c8ccaca74c9f20c

I will be working on this towards the end of this week.

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

So bottom line:
1) Driver will count rx packets as rx-ed packets regardless of XDP
decision.

2) Driver should keep track of XDP decisions statistics, report them in
ethtool and in the new API suggested by David. track even (XDP_PASS) ?

Maybe instead of having all drivers track the statistics on their own,
we should move the responsibility to upper layer.

Idea: since we already have rxq_info structure per XDP ring (no false
sharing) and available per xdp_buff we can do:

+++ b/include/linux/filter.h
@@ -651,7 +651,9 @@ static __always_inline u32 bpf_prog_run_xdp(const
struct bpf_prog *prog,
         * already takes rcu_read_lock() when fetching the program, so
         * it's not necessary here anymore.
         */
-       return BPF_PROG_RUN(prog, xdp);
+       u32 ret = BPF_PROG_RUN(prog, xdp);
+       xdp->xdp_rxq_info.stats[ret]++
+       return ret;
 }

still we need a way (API) to report the rxq_info to whoever needs to
read current XDP stats 

3) Unrelated, In non XDP case, if skb allocation fails or driver fails
to pass the skb up to the stack for somereason, should the driver
increase rx packets ? IMHO the answer should be yes if we want to have
similar behavior between XDP and non XDP cases.

But this could result in netdev->stats.rx_packets + netdev-
>stats.rx_dropped to be more than the actual rx-ed packets, is this
acceptable ?

Powered by blists - more mailing lists