Re: [PATCH net-next 3/3] tap: XDP support

[Jason Wang]

On 2017å07æ28æ 11:13, Jakub Kicinski wrote:
> On Thu, 27 Jul 2017 17:25:33 +0800, Jason Wang wrote:
> > This patch tries to implement XDP for tun. The implementation was
> > split into two parts:
> >
> > - fast path: small and no gso packet. We try to do XDP at page level
> >    before build_skb(). For XDP_TX, since creating/destroying queues
> >    were completely under control of userspace, it was implemented
> >    through generic XDP helper after skb has been built. This could be
> >    optimized in the future.
> > - slow path: big or gso packet. We try to do it after skb was created
> >    through generic XDP helpers.
> >
> > XDP_REDIRECT was not implemented, it could be done on top.
> >
> > xdp1 test shows 47.6% improvement:
> >
> > Before: ~2.1Mpps
> > After:  ~3.1Mpps
> >
> > Suggested-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > @@ -1008,6 +1016,56 @@ tun_net_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
> >   	stats->tx_dropped = tx_dropped;
> >   }
> >   
> > +static int tun_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > +		       struct netlink_ext_ack *extack)
> > +{
> > +	struct tun_struct *tun = netdev_priv(dev);
> > +	struct bpf_prog *old_prog;
> > +
> > +	/* We will shift the packet that can't be handled to generic
> > +	 * XDP layer.
> > +	 */
> > +
> > +	old_prog = rtnl_dereference(tun->xdp_prog);
> > +	if (old_prog)
> > +		bpf_prog_put(old_prog);
> > +	rcu_assign_pointer(tun->xdp_prog, prog);
> Is this OK?  Could this lead to the program getting freed and then
> datapath accessing a stale pointer?  I mean in the scenario where the
> process gets pre-empted between the bpf_prog_put() and
> rcu_assign_pointer()?

Will call bpf_prog_put() after rcu_assign_pointer().

> > +	if (prog) {
> > +		prog = bpf_prog_add(prog, 1);
> > +		if (IS_ERR(prog))
> > +			return PTR_ERR(prog);
> > +	}
> I don't think you need this extra reference here.  dev_change_xdp_fd()
> will call bpf_prog_get_type() which means driver gets the program with
> a reference already taken, drivers does have to free that reference when
> program is removed (or device is freed, as you correctly do).

I see, will drop this in next version.

Thanks.

> > +	return 0;
> > +}
> > +