[bpf-next,V1-RFC,10/14] tun: setup xdp_rxq_info

Driver hook points for xdp_rxq_info:
 * init+reg: tun_attach
 * unreg   : __tun_detach

I've done some manual testing of this tun driver, but I would
appriciate good review and someone else running their use-case tests,
as I'm not 100% sure I understand the tfile->detached semantics.

Cc: Jason Wang <jasowang@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 drivers/net/tun.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

On 2017年12月13日 19:20, Jesper Dangaard Brouer wrote:
> Driver hook points for xdp_rxq_info:
>   * init+reg: tun_attach
>   * unreg   : __tun_detach
>
> I've done some manual testing of this tun driver, but I would
> appriciate good review and someone else running their use-case tests,
> as I'm not 100% sure I understand the tfile->detached semantics.
>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>   drivers/net/tun.c |   25 ++++++++++++++++++++++++-
>   1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index e367d6310353..f1df08c2c541 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -180,6 +180,7 @@ struct tun_file {
>   	struct list_head next;
>   	struct tun_struct *detached;
>   	struct skb_array tx_array;
> +	struct xdp_rxq_info xdp_rxq;
>   };
>   
>   struct tun_flow_entry {
> @@ -687,8 +688,10 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>   			    tun->dev->reg_state == NETREG_REGISTERED)
>   				unregister_netdevice(tun->dev);
>   		}
> -		if (tun)
> +		if (tun) {
>   			skb_array_cleanup(&tfile->tx_array);
> +			xdp_rxq_info_unreg(&tfile->xdp_rxq);
> +		}
>   		sock_put(&tfile->sk);
>   	}
>   }
> @@ -728,11 +731,15 @@ static void tun_detach_all(struct net_device *dev)
>   		tun_napi_del(tun, tfile);
>   		/* Drop read queue */
>   		tun_queue_purge(tfile);
> +		skb_array_cleanup(&tfile->tx_array);

Looks like this is unnecessary, skb array will be cleaned up only when 
fd is closed otherwise there will be a double free.

Other looks good.

Thanks

> +		xdp_rxq_info_unreg(&tfile->xdp_rxq);
>   		sock_put(&tfile->sk);
>   	}
>   	list_for_each_entry_safe(tfile, tmp, &tun->disabled, next) {
>   		tun_enable_queue(tfile);
>   		tun_queue_purge(tfile);
> +		skb_array_cleanup(&tfile->tx_array);
> +		xdp_rxq_info_unreg(&tfile->xdp_rxq);
>   		sock_put(&tfile->sk);
>   	}
>   	BUG_ON(tun->numdisabled != 0);
> @@ -784,6 +791,21 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>   
>   	tfile->queue_index = tun->numqueues;
>   	tfile->socket.sk->sk_shutdown &= ~RCV_SHUTDOWN;
> +
> +	if (tfile->detached) {
> +		/* Re-attach detached tfile, updating XDP queue_index */
> +		WARN_ON(!xdp_rxq_info_is_reg(&tfile->xdp_rxq));
> +
> +		if (tfile->xdp_rxq.queue_index    != tfile->queue_index)
> +			tfile->xdp_rxq.queue_index = tfile->queue_index;
> +	} else {
> +		/* Setup XDP RX-queue info, for new tfile getting attached */
> +		xdp_rxq_info_init(&tfile->xdp_rxq);
> +		tfile->xdp_rxq.dev = tun->dev;
> +		tfile->xdp_rxq.queue_index = tfile->queue_index;
> +		xdp_rxq_info_reg(&tfile->xdp_rxq);
> +	}
> +
>   	rcu_assign_pointer(tfile->tun, tun);
>   	rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);
>   	tun->numqueues++;
> @@ -1508,6 +1530,7 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>   		xdp.data = buf + pad;
>   		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);
>   
>

On Wed, 20 Dec 2017 15:48:01 +0800
Jason Wang <jasowang@redhat.com> wrote:

> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index e367d6310353..f1df08c2c541 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -180,6 +180,7 @@ struct tun_file {
> >   	struct list_head next;
> >   	struct tun_struct *detached;
> >   	struct skb_array tx_array;
> > +	struct xdp_rxq_info xdp_rxq;
> >   };
> >   
> >   struct tun_flow_entry {
> > @@ -687,8 +688,10 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
> >   			    tun->dev->reg_state == NETREG_REGISTERED)
> >   				unregister_netdevice(tun->dev);
> >   		}
> > -		if (tun)
> > +		if (tun) {
> >   			skb_array_cleanup(&tfile->tx_array);
> > +			xdp_rxq_info_unreg(&tfile->xdp_rxq);
> > +		}
> >   		sock_put(&tfile->sk);
> >   	}
> >   }
> > @@ -728,11 +731,15 @@ static void tun_detach_all(struct net_device *dev)
> >   		tun_napi_del(tun, tfile);
> >   		/* Drop read queue */
> >   		tun_queue_purge(tfile);
> > +		skb_array_cleanup(&tfile->tx_array);  
> 
> Looks like this is unnecessary, skb array will be cleaned up only when 
> fd is closed otherwise there will be a double free.

What code path is called on "fd close" which call skb_array_cleanup() ?
(Is it __tun_detach()?)

Then, I guess I don't need below xdp_rxq_info_unreg() either, right?
 
> > +		xdp_rxq_info_unreg(&tfile->xdp_rxq);
> >   		sock_put(&tfile->sk);