Re: [PATCH v2 bpf 1/5] net: ethtool: add xdp properties flag set

[John Fastabend <john.fastabend@gmail.com>]

Jesper Dangaard Brouer wrote:
> On Fri, 4 Dec 2020 16:21:08 +0100
> Daniel Borkmann <daniel@iogearbox.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@gmail.com writes:  
> > >>> From: Marek Majtyka <marekx.majtyka@intel.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.

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

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.

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