Message-ID: <20201207230755.GB27205@ranger.igk.intel.com>
Date:   Tue, 8 Dec 2020 00:07:55 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Jesper Dangaard Brouer <jbrouer@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        alardam@...il.com, magnus.karlsson@...el.com,
        bjorn.topel@...el.com, andrii.nakryiko@...il.com, kuba@...nel.org,
        ast@...nel.org, netdev@...r.kernel.org, davem@...emloft.net,
        hawk@...nel.org, jonathan.lemon@...il.com, bpf@...r.kernel.org,
        jeffrey.t.kirsher@...el.com, maciejromanfijalkowski@...il.com,
        intel-wired-lan@...ts.osuosl.org,
        Marek Majtyka <marekx.majtyka@...el.com>
Subject: Re: [PATCH v2 bpf 1/5] net: ethtool: add xdp properties flag set

On Mon, Dec 07, 2020 at 12:52:22PM -0800, John Fastabend wrote:
> Jesper Dangaard Brouer wrote:
> > On Fri, 4 Dec 2020 16:21:08 +0100
> > Daniel Borkmann <daniel@...earbox.net> wrote:
> > 
> > > On 12/4/20 1:46 PM, Maciej Fijalkowski wrote:
> > > > On Fri, Dec 04, 2020 at 01:18:31PM +0100, Toke Høiland-Jørgensen wrote:  
> > > >> alardam@...il.com writes:  
> > > >>> From: Marek Majtyka <marekx.majtyka@...el.com>
> > > >>>
> > > >>> Implement support for checking what kind of xdp functionality a netdev
> > > >>> supports. Previously, there was no way to do this other than to try
> > > >>> to create an AF_XDP socket on the interface or load an XDP program and see
> > > >>> if it worked. This commit changes this by adding a new variable which
> > > >>> describes all xdp supported functions on pretty detailed level:  
> > > >>
> > > >> I like the direction this is going! :)
> > 
> > (Me too, don't get discouraged by our nitpicking, keep working on this! :-))
> > 
> > > >>  
> > > >>>   - aborted
> > > >>>   - drop
> > > >>>   - pass
> > > >>>   - tx  
> > > 
> > > I strongly think we should _not_ merge any native XDP driver patchset
> > > that does not support/implement the above return codes. 
> > 
> > I agree, with above statement.
> > 
> > > Could we instead group them together and call this something like
> > > XDP_BASE functionality to not give a wrong impression?
> > 
> > I disagree.  I can accept that XDP_BASE include aborted+drop+pass.
> > 
> > I think we need to keep XDP_TX action separate, because I think that
> > there are use-cases where the we want to disable XDP_TX due to end-user
> > policy or hardware limitations.
> 
> How about we discover this at load time though. Meaning if the program
> doesn't use XDP_TX then the hardware can skip resource allocations for
> it. I think we could have verifier or extra pass discover the use of
> XDP_TX and then pass a bit down to driver to enable/disable TX caps.

+1

> 
> > 
> > Use-case(1): Cloud-provider want to give customers (running VMs) ability
> > to load XDP program for DDoS protection (only), but don't want to allow
> > customer to use XDP_TX (that can implement LB or cheat their VM
> > isolation policy).
> 
> Not following. What interface do they want to allow loading on? If its
> the VM interface then I don't see how it matters. From outside the
> VM there should be no way to discover if its done in VM or in tc or
> some other stack.
> 
> If its doing some onloading/offloading I would assume they need to
> ensure the isolation, etc. is still maintained because you can't
> let one VMs program work on other VMs packets safely.
> 
> So what did I miss, above doesn't make sense to me.
> 
> > 
> > Use-case(2): Disable XDP_TX on a driver to save hardware TX-queue
> > resources, as the use-case is only DDoS.  Today we have this problem
> > with the ixgbe hardware, that cannot load XDP programs on systems with
> > more than 192 CPUs.
> 
> The ixgbe issues is just a bug or missing-feature in my opinion.

Not a bug, rather HW limitation?

> 
> I think we just document that XDP_TX consumes resources and if users
> care they shouldn't use XD_TX in programs and in that case hardware
> should via program discovery not allocate the resource. This seems
> cleaner in my opinion then more bits for features.

But what if I'm with some limited HW that actually has a support for XDP
and I would like to utilize XDP_TX?

Not all drivers that support XDP consume Tx resources. Recently igb got
support and it shares Tx queues between netstack and XDP.

I feel like we should have a sort-of best effort approach in case we
stumble upon the XDP_TX in prog being loaded and query the driver if it
would be able to provide the Tx resources on the current system, given
that normally we tend to have a queue per core.

In that case igb would say yes, ixgbe would say no and prog would be
rejected.

> 
> > 
> > 
> > > If this is properly documented that these are basic must-have
> > > _requirements_, then users and driver developers both know what the
> > > expectations are.
> > 
> > We can still document that XDP_TX is a must-have requirement, when a
> > driver implements XDP.
> 
> +1
> 
> > 
> > 
> > > >>>   - redirect  
> > > >>
> > 
> > 
> > -- 
> > Best regards,
> >   Jesper Dangaard Brouer
> >   MSc.CS, Principal Kernel Engineer at Red Hat
> >   LinkedIn: http://www.linkedin.com/in/brouer
> > 
> 
> 

Powered by blists - more mailing lists