netdev - Re: [PATCH net-next v2] xen-netfront: add basic XDP support

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]

Message-ID: <20200305153929.186819a8@carbon>
Date:   Thu, 5 Mar 2020 15:39:41 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Ilias Apalodimas <ilias.apalodimas@...aro.org>
Cc:     brouer@...hat.com, Denis Kirjanov <kirjanov@...il.com>,
        Denis Kirjanov <kda@...ux-powerpc.org>, netdev@...r.kernel.org,
        jgross@...e.com
Subject: Re: [PATCH net-next v2] xen-netfront: add basic XDP support

On Thu, 5 Mar 2020 15:31:14 +0200
Ilias Apalodimas <ilias.apalodimas@...aro.org> wrote:

> On Thu, Mar 05, 2020 at 04:23:31PM +0300, Denis Kirjanov wrote:
> > On 3/5/20, Ilias Apalodimas <ilias.apalodimas@...aro.org> wrote:  
> > > Hi Denis,
> > >
> > > There's a bunch of things still missing from my remarks on V1.
> > > XDP is not supposed to allocate and free pages constantly as that's one of
> > > the things that's making it fast.  
> > 
> > Hi Ilias,
> > 
> > I've removed the copying to an allocated page so there is no page
> > allocation/free logic added.
> >   
> 
> Yea that has been removed. I am not familiar with the driver though, so i'll
> give you an example. 
> Let's say the BPF program says the packet must be dropped. What will happen to
> the page with the packet payload?
> Ideally on XDP we want that page recycled back into the device descriptors, so
> the driver won't have to allocate and map a fresh page.

I agree.  The main point with XDP is that we can do something
faster-than the normal network stack.  Especially in case of XDP_DROP,
we do driver specific recycling tricks, to avoid any allocations and
reinsert the RX-frame in RX-ring, and avoid overhead of SKB allocations.

Looking closer at your patch it seem you run XDP after the SKB alloc?!?

> >   
> > >
> > > You are also missing proper support for XDP_REDIRECT, ndo_xdp_xmit. We
> > > usually require the whole functionality to merge the driver.  

I agree, we have unfortunately seen drivers not getting completed if we
don't require full-XDP feature set.

> > 
> > I wanted to minimize changes and send follow-up patches
> >   
> 
> Adding XDP_REDIRECT is pretty trivial and the ndo_xdp_xmit should be very
> similar to XDP_TX. So assuming you'll fix XDP_TX adding the .ndo one will be
> relatively small amount of code.

You can have a patchset with more patches, if you prefer splitting this
up in multiple patches.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer