Re: [PATCH net-next 06/11] tuntap: split out XDP logic

[Jason Wang]

On 2018å09æ07æ 01:21, Michael S. Tsirkin wrote:
> On Thu, Sep 06, 2018 at 12:05:21PM +0800, Jason Wang wrote:
> > This patch split out XDP logic into a single function. This make it to
> > be reused by XDP batching path in the following patch.
> >
> > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > ---
> >   drivers/net/tun.c | 84 ++++++++++++++++++++++++++++-------------------
> >   1 file changed, 51 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 389aa0727cc6..21b125020b3b 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1635,6 +1635,44 @@ static bool tun_can_build_skb(struct tun_struct *tun, struct tun_file *tfile,
> >   	return true;
> >   }
> >   
> > +static u32 tun_do_xdp(struct tun_struct *tun,
> > +		      struct tun_file *tfile,
> > +		      struct bpf_prog *xdp_prog,
> > +		      struct xdp_buff *xdp,
> > +		      int *err)
> > +{
> > +	u32 act = bpf_prog_run_xdp(xdp_prog, xdp);
> > +
> > +	switch (act) {
> > +	case XDP_REDIRECT:
> > +		*err = xdp_do_redirect(tun->dev, xdp, xdp_prog);
> > +		xdp_do_flush_map();
> > +		if (*err)
> > +			break;
> > +		goto out;
> > +	case XDP_TX:
> > +		*err = tun_xdp_tx(tun->dev, xdp);
> > +		if (*err < 0)
> > +			break;
> > +		*err = 0;
> > +		goto out;
> > +	case XDP_PASS:
> > +		goto out;
> Do we need goto? why not just return?

I don't see any difference.

> > +	default:
> > +		bpf_warn_invalid_xdp_action(act);
> > +		/* fall through */
> > +	case XDP_ABORTED:
> > +		trace_xdp_exception(tun->dev, xdp_prog, act);
> > +		/* fall through */
> > +	case XDP_DROP:
> > +		break;
> > +	}
> > +
> > +	put_page(virt_to_head_page(xdp->data_hard_start));
> put here because caller does get_page :( Not pretty.
> I'd move this out to the caller.

Then we need a switch in the caller, not sure it's better.

> > +out:
> > +	return act;
> How about combining err and act? err is < 0 XDP_PASS is > 0.
> No need for pointers then.

Ok.

> > +}
> > +
> >   static struct sk_buff *tun_build_skb(struct tun_struct *tun,
> >   				     struct tun_file *tfile,
> >   				     struct iov_iter *from,
> > @@ -1645,10 +1683,10 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
> >   	struct sk_buff *skb = NULL;
> >   	struct bpf_prog *xdp_prog;
> >   	int buflen = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > -	unsigned int delta = 0;
> >   	char *buf;
> >   	size_t copied;
> > -	int err, pad = TUN_RX_PAD;
> > +	int pad = TUN_RX_PAD;
> > +	int err = 0;
> >   
> >   	rcu_read_lock();
> >   	xdp_prog = rcu_dereference(tun->xdp_prog);
> > @@ -1685,9 +1723,8 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
> >   	local_bh_disable();
> >   	rcu_read_lock();
> >   	xdp_prog = rcu_dereference(tun->xdp_prog);
> > -	if (xdp_prog && !*skb_xdp) {
> > +	if (xdp_prog) {
> >   		struct xdp_buff xdp;
> > -		void *orig_data;
> >   		u32 act;
> >   
> >   		xdp.data_hard_start = buf;
> > @@ -1695,33 +1732,14 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
> >   		xdp_set_data_meta_invalid(&xdp);
> >   		xdp.data_end = xdp.data + len;
> >   		xdp.rxq = &tfile->xdp_rxq;
> > -		orig_data = xdp.data;
> > -		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> > -
> > -		switch (act) {
> > -		case XDP_REDIRECT:
> > -			err = xdp_do_redirect(tun->dev, &xdp, xdp_prog);
> > -			xdp_do_flush_map();
> > -			if (err)
> > -				goto err_xdp;
> > -			goto out;
> > -		case XDP_TX:
> > -			if (tun_xdp_tx(tun->dev, &xdp) < 0)
> > -				goto err_xdp;
> > -			goto out;
> > -		case XDP_PASS:
> > -			delta = orig_data - xdp.data;
> > -			len = xdp.data_end - xdp.data;
> > -			break;
> > -		default:
> > -			bpf_warn_invalid_xdp_action(act);
> > -			/* fall through */
> > -		case XDP_ABORTED:
> > -			trace_xdp_exception(tun->dev, xdp_prog, act);
> > -			/* fall through */
> > -		case XDP_DROP:
> > +		act = tun_do_xdp(tun, tfile, xdp_prog, &xdp, &err);
> > +		if (err)
> >   			goto err_xdp;
> > -		}
> > +		if (act != XDP_PASS)
> > +			goto out;
> likely?

It depends on the XDP program, so I tend not to use it.

> > +
> > +		pad = xdp.data - xdp.data_hard_start;
> > +		len = xdp.data_end - xdp.data;
> >   	}
> >   	rcu_read_unlock();
> >   	local_bh_enable();
> > @@ -1729,18 +1747,18 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
> >   build:
> >   	skb = build_skb(buf, buflen);
> >   	if (!skb) {
> > +		put_page(alloc_frag->page);
> >   		skb = ERR_PTR(-ENOMEM);
> >   		goto out;
> >   	}
> >   
> > -	skb_reserve(skb, pad - delta);
> > +	skb_reserve(skb, pad);
> >   	skb_put(skb, len);
> >   
> >   	return skb;
> >   
> >   err_xdp:
> > -	alloc_frag->offset -= buflen;
> > -	put_page(alloc_frag->page);
> > +	this_cpu_inc(tun->pcpu_stats->rx_dropped);
>
> This fixes bug in previous patch which dropped it. OK :)

Yes, but let me move this to the buggy patch.

Thanks

> >   out:
> >   	rcu_read_unlock();
> >   	local_bh_enable();
> > --
> > 2.17.1