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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Thu, Mar 05, 2020 at 04:23:31PM +0300, Denis Kirjanov wrote:
> On 3/5/20, Ilias Apalodimas <ilias.apalodimas@linaro.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.
> 
i
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.

> 
> >
> > You are also missing proper support for XDP_REDIRECT, ndo_xdp_xmit. We
> > usually
> > require the whole functionality to merge the driver.
> 
> 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.

> >
> >
> > On Mon, Mar 02, 2020 at 05:21:14PM +0300, Denis Kirjanov wrote:
> >>
> > [...]
> >> +u32 xennet_run_xdp(struct netfront_queue *queue, struct page *pdata,
> >> +		   struct xen_netif_rx_response *rx, struct bpf_prog *prog,
> >> +		   struct xdp_buff *xdp)
> >> +{
> >> +	u32 len = rx->status;
> >> +	u32 act = XDP_PASS;
> >> +
> >> +	xdp->data_hard_start = page_address(pdata);
> >> +	xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
> >> +	xdp_set_data_meta_invalid(xdp);
> >> +	xdp->data_end = xdp->data + len;
> >> +	xdp->handle = 0;
> >> +
> >> +	act = bpf_prog_run_xdp(prog, xdp);
> >> +	switch (act) {
> >> +	case XDP_PASS:
> >> +	case XDP_TX:
> >> +	case XDP_DROP:
> >
> > Maybe i am missing something on the usage, but XDP_TX and XDROP are handled
> > similarly?
> > XDP_TX is supposed to sent the packet out of the interface it arrived.
> 
> Yep, you're right. I'll add it to the next version.
> 
> Thanks!
> 

Cheers
/Ilias