| Message ID | 20210109024950.4043819-3-charlie@charlie.bz (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce XDP_FLAGS_NO_TX flag | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 6 maintainers not CCed: ast@kernel.org daniel@iogearbox.net bpf@vger.kernel.org virtualization@lists.linux-foundation.org hawk@kernel.org john.fastabend@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 87 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Charlie Somerville <charlie@charlie.bz> writes: > No send queues will be allocated for XDP filters. Attempts to > transmit > packets when no XDP send queues exist will fail with EOPNOTSUPP. > > Signed-off-by: Charlie Somerville <charlie@charlie.bz> > --- > drivers/net/virtio_net.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 508408fbe78f..ed08998765e0 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -485,6 +485,10 @@ static struct send_queue > *virtnet_xdp_sq(struct virtnet_info *vi) > { > unsigned int qp; > > + /* If no queue pairs are allocated for XDP use, return > NULL */ > + if (vi->xdp_queue_pairs == 0) > + return NULL; > + > qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + > smp_processor_id(); > return &vi->sq[qp]; > } > @@ -514,6 +518,11 @@ static int virtnet_xdp_xmit(struct > net_device *dev, > > sq = virtnet_xdp_sq(vi); > > + /* No send queue exists if program was attached with > XDP_NO_TX */ > + if (unlikely(!sq)) { > + return -EOPNOTSUPP; > + } > + > if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) { > ret = -EINVAL; > drops = n; > @@ -1464,7 +1473,7 @@ static int virtnet_poll(struct napi_struct > *napi, int budget) > > if (xdp_xmit & VIRTIO_XDP_TX) { > sq = virtnet_xdp_sq(vi); > - if (virtqueue_kick_prepare(sq->vq) && > virtqueue_notify(sq->vq)) { > + if (sq && virtqueue_kick_prepare(sq->vq) && > virtqueue_notify(sq->vq)) { Is this addition needed ? Seems like we don't set VIRTIO_XDP_TX bit in case of virtnet_xdp_xmit() failure, so the surrounding 'if' won't be taken. > u64_stats_update_begin(&sq->stats.syncp); > sq->stats.kicks++; > u64_stats_update_end(&sq->stats.syncp); > @@ -2388,7 +2397,7 @@ static int > virtnet_restore_guest_offloads(struct virtnet_info *vi) ... >
On Mon, Jan 11, 2021, at 04:31, Shay Agroskin wrote: > Is this addition needed ? Seems like we don't set VIRTIO_XDP_TX > bit in case of virtnet_xdp_xmit() failure, so the surrounding 'if' > won't be taken. Good catch, it looks like you're right. I'm happy to remove that extra branch although I would like to add a comment explaining that assumption: diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index ed08998765e0..3ae7cd2f1e72 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1472,8 +1472,10 @@ static int virtnet_poll(struct napi_struct *napi, int budget) xdp_do_flush(); if (xdp_xmit & VIRTIO_XDP_TX) { + /* VIRTIO_XDP_TX only set on successful virtnet_xdp_xmit, + * implies sq != NULL */ sq = virtnet_xdp_sq(vi); - if (sq && virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) { + if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) { u64_stats_update_begin(&sq->stats.syncp); sq->stats.kicks++; u64_stats_update_end(&sq->stats.syncp);
Charlie Somerville <charlie@charlie.bz> writes: > On Mon, Jan 11, 2021, at 04:31, Shay Agroskin wrote: > >> Is this addition needed ? Seems like we don't set VIRTIO_XDP_TX >> bit in case of virtnet_xdp_xmit() failure, so the surrounding >> 'if' >> won't be taken. > > Good catch, it looks like you're right. I'm happy to remove that > extra branch although I would like to add a comment explaining > that assumption: > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index ed08998765e0..3ae7cd2f1e72 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1472,8 +1472,10 @@ static int virtnet_poll(struct > napi_struct *napi, int budget) > xdp_do_flush(); > > if (xdp_xmit & VIRTIO_XDP_TX) { > + /* VIRTIO_XDP_TX only set on successful > virtnet_xdp_xmit, > + * implies sq != NULL */ > sq = virtnet_xdp_sq(vi); > - if (sq && virtqueue_kick_prepare(sq->vq) && > virtqueue_notify(sq->vq)) { > + if (virtqueue_kick_prepare(sq->vq) && > virtqueue_notify(sq->vq)) { > u64_stats_update_begin(&sq->stats.syncp); > sq->stats.kicks++; > u64_stats_update_end(&sq->stats.syncp); The comment itself looks good. Note that the code style dictates that multi-line comments should end up with a line containing '*/' alone. See https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting for more information Also you'd probably need to send a new email containing the new patchset (see V# tag on emails in the mailing list) Shay
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 508408fbe78f..ed08998765e0 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -485,6 +485,10 @@ static struct send_queue *virtnet_xdp_sq(struct virtnet_info *vi) { unsigned int qp; + /* If no queue pairs are allocated for XDP use, return NULL */ + if (vi->xdp_queue_pairs == 0) + return NULL; + qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + smp_processor_id(); return &vi->sq[qp]; } @@ -514,6 +518,11 @@ static int virtnet_xdp_xmit(struct net_device *dev, sq = virtnet_xdp_sq(vi); + /* No send queue exists if program was attached with XDP_NO_TX */ + if (unlikely(!sq)) { + return -EOPNOTSUPP; + } + if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) { ret = -EINVAL; drops = n; @@ -1464,7 +1473,7 @@ static int virtnet_poll(struct napi_struct *napi, int budget) if (xdp_xmit & VIRTIO_XDP_TX) { sq = virtnet_xdp_sq(vi); - if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) { + if (sq && virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) { u64_stats_update_begin(&sq->stats.syncp); sq->stats.kicks++; u64_stats_update_end(&sq->stats.syncp); @@ -2388,7 +2397,7 @@ static int virtnet_restore_guest_offloads(struct virtnet_info *vi) } static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, - struct netlink_ext_ack *extack) + struct netlink_ext_ack *extack, u32 flags) { unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr); struct virtnet_info *vi = netdev_priv(dev); @@ -2418,7 +2427,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, } curr_qp = vi->curr_queue_pairs - vi->xdp_queue_pairs; - if (prog) + if (prog && !(flags & XDP_FLAGS_NO_TX)) xdp_qp = nr_cpu_ids; /* XDP requires extra queues for XDP_TX */ @@ -2502,7 +2511,7 @@ static int virtnet_xdp(struct net_device *dev, struct netdev_bpf *xdp) { switch (xdp->command) { case XDP_SETUP_PROG: - return virtnet_xdp_set(dev, xdp->prog, xdp->extack); + return virtnet_xdp_set(dev, xdp->prog, xdp->extack, xdp->flags); default: return -EINVAL; }
No send queues will be allocated for XDP filters. Attempts to transmit packets when no XDP send queues exist will fail with EOPNOTSUPP. Signed-off-by: Charlie Somerville <charlie@charlie.bz> --- drivers/net/virtio_net.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)