netdev - Re: [net-next PATCH v2 4/5] virtio_net: add dedicated XDP transmit queues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5832DE60.4000200@iogearbox.net>
Date:   Mon, 21 Nov 2016 12:45:36 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>, eric.dumazet@...il.com,
        mst@...hat.com, kubakici@...pl, shm@...ulusnetworks.com,
        davem@...emloft.net, alexei.starovoitov@...il.com
CC:     netdev@...r.kernel.org, bblanco@...mgrid.com,
        john.r.fastabend@...el.com, brouer@...hat.com, tgraf@...g.ch
Subject: Re: [net-next PATCH v2 4/5] virtio_net: add dedicated XDP transmit
 queues

On 11/20/2016 03:51 AM, John Fastabend wrote:
> XDP requires using isolated transmit queues to avoid interference
> with normal networking stack (BQL, NETDEV_TX_BUSY, etc). This patch
> adds a XDP queue per cpu when a XDP program is loaded and does not
> expose the queues to the OS via the normal API call to
> netif_set_real_num_tx_queues(). This way the stack will never push
> an skb to these queues.
>
> However virtio/vhost/qemu implementation only allows for creating
> TX/RX queue pairs at this time so creating only TX queues was not
> possible. And because the associated RX queues are being created I
> went ahead and exposed these to the stack and let the backend use
> them. This creates more RX queues visible to the network stack than
> TX queues which is worth mentioning but does not cause any issues as
> far as I can tell.
>
> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
> ---
>   drivers/net/virtio_net.c |   32 +++++++++++++++++++++++++++++---
>   1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8f99a53..80a426c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -114,6 +114,9 @@ struct virtnet_info {
>   	/* # of queue pairs currently used by the driver */
>   	u16 curr_queue_pairs;
>
> +	/* # of XDP queue pairs currently used by the driver */
> +	u16 xdp_queue_pairs;
> +
>   	/* I like... big packets and I cannot lie! */
>   	bool big_packets;
>
> @@ -1525,7 +1528,8 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
>   	struct bpf_prog *old_prog;
> -	int i;
> +	u16 xdp_qp = 0, curr_qp;
> +	int err, i;
>
>   	if ((dev->features & NETIF_F_LRO) && prog) {
>   		netdev_warn(dev, "can't set XDP while LRO is on, disable LRO first\n");
> @@ -1542,12 +1546,34 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>   		return -EINVAL;
>   	}
>
> +	curr_qp = vi->curr_queue_pairs - vi->xdp_queue_pairs;
> +	if (prog)
> +		xdp_qp = nr_cpu_ids;
> +
> +	/* XDP requires extra queues for XDP_TX */
> +	if (curr_qp + xdp_qp > vi->max_queue_pairs) {
> +		netdev_warn(dev, "request %i queues but max is %i\n",
> +			    curr_qp + xdp_qp, vi->max_queue_pairs);
> +		return -ENOMEM;
> +	}
> +
> +	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
> +	if (err) {
> +		dev_warn(&dev->dev, "XDP Device queue allocation failure.\n");
> +		return err;
> +	}
> +
>   	if (prog) {
> -		prog = bpf_prog_add(prog, vi->max_queue_pairs - 1);
> -		if (IS_ERR(prog))
> +		prog = bpf_prog_add(prog, vi->max_queue_pairs);

I think this change is not correct, it would be off by one now.
The previous 'vi->max_queue_pairs - 1' was actually correct here.
dev_change_xdp_fd() already gives you a reference (see the doc on
enum xdp_netdev_command in netdevice.h).

> +		if (IS_ERR(prog)) {
> +			virtnet_set_queues(vi, curr_qp);
>   			return PTR_ERR(prog);
> +		}
>   	}
>
> +	vi->xdp_queue_pairs = xdp_qp;
> +	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> +
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
>   		old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>   		rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>