Re: [PATCH net-next 1/2] virtio-net: re enable XDP_REDIRECT for mergeable buffer

[Jason Wang]

On 2018å03æ01æ 16:41, Jesper Dangaard Brouer wrote:
> On Thu,  1 Mar 2018 11:19:04 +0800 Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> > XDP_REDIRECT support for mergeable buffer was removed since commit
> > 7324f5399b06 ("virtio_net: disable XDP_REDIRECT in receive_mergeable()
> > case"). This is because we don't reserve enough tailroom for struct
> > skb_shared_info which breaks XDP assumption. Other complaints are, the
> > complex linearize logic and EWMA estimation may increase the
> > possibility of linearizing.
> This patch also have the intermixing issues, I mentioned for patch 2/2.
>
> On Thu, 1 Mar 2018 09:02:06 +0100
> Jesper Dangaard Brouer <brouer@xxxxxxxxxx> wrote:
>
> > 1. XDP generic is not feature complete, e.g. cpumap will drop these
> >     packets. It might not be possible to implement some features, think
> >     of (AF_XDP) zero-copy.
> >
> > 2. This can easily cause out-of-order packets.
> >
> > 3. It makes it harder to troubleshoot, when diagnosing issues
> >     around #1, we have a hard time determining what path an XDP packet
> >     took (the xdp tracepoints doesn't know).
>
> It is slightly better, as it is consistent in calling XDP-generic in
> the XDP_REDIRECT action, which an action under heavy development, here
> we want the freedom to develop in different code tempi.

Looks not, this patch did:

+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ case XDP_REDIRECT:
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ err = xdp_do_redirect(dev, &xdp, xdp_prog);
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (err)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto err_xdp;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ *xdp_xmit = true;
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rcu_read_unlock();
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto xdp_xmit;

So native version is used for most cases. XDP-generic will be only used 
for the packets of insufficient headroom which should be rare (just 
first few packets).

>    And some
> features might never be available in XDP-generic. Thus, when a feature
> is missing/broken it will be consistent for the user.

This sounds bad.

> The remaining question is how will a user know that XDP "mode" she is
> using?

So for this patch, skb mode will only happen for the first several 
packets. So mostly native mode. And it's even not clear to me if user 
need to know about this since the performance should be the same.

>    The user clearly loaded an XDP-native program, and expect the
> associated performance, but XDP_REDIRECT will be using the slow
> XDP-generic code path...

As I replied above, XDP_REDIRECT use native mode except for the first 
few packets.

> > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > ---
> >   drivers/net/virtio_net.c | 107 +++++++++++++++++++++++++++++------------------
> >   1 file changed, 67 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 9bb9e56..81190ba 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> [...]
> > @@ -689,31 +709,30 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >   	struct bpf_prog *xdp_prog;
> >   	unsigned int truesize;
> >   	unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> > -	bool sent;
> > +	bool sent, skb_xdp = false;
> > +	int err;
> >   
> >   	head_skb = NULL;
> >   
> >   	rcu_read_lock();
> >   	xdp_prog = rcu_dereference(rq->xdp_prog);
> >   	if (xdp_prog) {
> > -		struct page *xdp_page;
> >   		struct xdp_buff xdp;
> >   		void *data;
> >   		u32 act;
> >   
> > -		/* This happens when rx buffer size is underestimated */
> > +		/* This happens when rx buffer size is underestimated
> > +		 * or headroom is not enough because of the buffer
> > +		 * was refilled before XDP is set. In both cases,
> > +		 * for simplicity, we will offload them to generic
> > +		 * XDP routine. This should only happen for the first
> > +		 * several packets, so we don't care much about its
> > +		 * performance.
> > +		 */
> >   		if (unlikely(num_buf > 1 ||
> >   			     headroom < virtnet_get_headroom(vi))) {
> I think you also need to check the tailroom here? (AFAIK this is hidden
> in the len_to_ctx as the "truesize").

add_recvbuf_mergeable() did this:

ÂÂÂ unsigned int headroom = virtnet_get_headroom(vi);
ÂÂÂ unsigned int tailroom = headroom ? sizeof(struct skb_shared_info) : 0;

So if headroom is not zero, it implies tailroom here.

Thanks

> > -			/* linearize data for XDP */
> > -			xdp_page = xdp_linearize_page(rq, &num_buf,
> > -						      page, offset,
> > -						      VIRTIO_XDP_HEADROOM,
> > -						      &len);
> > -			if (!xdp_page)
> > -				goto err_xdp;
> > -			offset = VIRTIO_XDP_HEADROOM;
> > -		} else {
> > -			xdp_page = page;
> > +			skb_xdp = true;
> > +			goto skb_xdp;
> >   		}
> >   
> >   		/* Transient failure which in theory could occur if