| Message ID | 20161120025104.19187.54400.stgit@john-Precision-Tower-5810 |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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@intel.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); >
On 16-11-21 03:45 AM, Daniel Borkmann wrote: > 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@intel.com> >> --- [...] >> } >> >> + 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). Right, this was an error thanks for checking it I'll send a v3. And maybe draft a test for XDP ref counting to test it in the future. .John
On Sat, Nov 19, 2016 at 06:51:04PM -0800, 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@intel.com> FYI what's supposed to happen is packets from the same flow going in the reverse direction will go on the same queue. This might come in handy when implementing RX XDP. > --- > 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); > + 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);
On 16-11-21 03:13 PM, Michael S. Tsirkin wrote: > On Sat, Nov 19, 2016 at 06:51:04PM -0800, 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@intel.com> > > FYI what's supposed to happen is packets from the same > flow going in the reverse direction will go on the > same queue. > > This might come in handy when implementing RX XDP. > Yeah but if its the first packet not part of a flow then presumably it can pick any queue but its worth keeping in mind certainly. .John
On Tue, Nov 22, 2016 at 12:17:40AM -0800, John Fastabend wrote: > On 16-11-21 03:13 PM, Michael S. Tsirkin wrote: > > On Sat, Nov 19, 2016 at 06:51:04PM -0800, 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@intel.com> > > > > FYI what's supposed to happen is packets from the same > > flow going in the reverse direction will go on the > > same queue. > > > > This might come in handy when implementing RX XDP. > > > > Yeah but if its the first packet not part of a flow then presumably it > can pick any queue but its worth keeping in mind certainly. > > .John Oh I agree, absolutely. This was just a FYI in case it comes useful as an optimization down the road.
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); + 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);
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@intel.com> --- drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)