[PATCH net-next 0/2] Introduce XDP_FLAGS_NO_TX flag

[PATCH net-next 0/2] Introduce XDP_FLAGS_NO_TX flag

[Charlie Somerville]

This patch series introduces a new flag XDP_FLAGS_NO_TX which prevents
the allocation of additional send queues for XDP programs.

Included in this patch series is an implementation of XDP_FLAGS_NO_TX
for the virtio_net driver. This flag is intended to be advisory - not
all drivers must implement support for it.

Many virtualised environments only provide enough virtio_net send queues
for the number of processors allocated to the VM:

# nproc
8
# ethtool --show-channels ens3
Channel parameters for ens3:
Pre-set maximums:
RX:     0
TX:     0
Other:      0
Combined:   8

In this configuration XDP is unusable because the virtio_net driver
always tries to allocate an extra send queue for each processor - even
if the XDP the program never uses the XDP_TX functionality.

While XDP_TX is still unavailable in these environments, this new flag
expands the set of XDP programs that can be used.

This is my first contribution to the kernel, so apologies if I've sent
this to the wrong list. I have tried to cc relevant maintainers but
it's possible I may have missed some people. I'm looking forward to
receiving feedback on this change.

Charlie Somerville (2):
  xdp: Add XDP_FLAGS_NO_TX flag
  virtio_net: Implement XDP_FLAGS_NO_TX support

 drivers/net/virtio_net.c     | 17 +++++++++++++----
 include/uapi/linux/if_link.h |  5 ++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.30.0

[PATCH net-next 1/2] xdp: Add XDP_FLAGS_NO_TX flag

[Charlie Somerville]

Some network interfaces must allocate additional hardware resources to
support XDP filters retransmitting packets with XDP_TX.

However not all XDP filters do use XDP_TX, and there may not be any
additional send queues available for use.

XDP filters can indicate that they will never transmit by setting the
XDP_FLAGS_NO_TX flag in the IFLA_XDP_FLAGS attribute. This flag is
only advisory - some network drivers may still allocate send queues.

Signed-off-by: Charlie Somerville <charlie@charlie.bz>
---
 include/uapi/linux/if_link.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 874cc12a34d9..b4ba4427cd98 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1168,11 +1168,14 @@ enum {
 #define XDP_FLAGS_DRV_MODE		(1U << 2)
 #define XDP_FLAGS_HW_MODE		(1U << 3)
 #define XDP_FLAGS_REPLACE		(1U << 4)
+#define XDP_FLAGS_NO_TX			(1U << 5)
 #define XDP_FLAGS_MODES			(XDP_FLAGS_SKB_MODE | \
 					 XDP_FLAGS_DRV_MODE | \
 					 XDP_FLAGS_HW_MODE)
 #define XDP_FLAGS_MASK			(XDP_FLAGS_UPDATE_IF_NOEXIST | \
-					 XDP_FLAGS_MODES | XDP_FLAGS_REPLACE)
+					 XDP_FLAGS_MODES | \
+					 XDP_FLAGS_REPLACE | \
+					 XDP_FLAGS_NO_TX)
 
 /* These are stored into IFLA_XDP_ATTACHED on dump. */
 enum {
-- 
2.30.0

[PATCH net-next 2/2] virtio_net: Implement XDP_FLAGS_NO_TX support

[Charlie Somerville]

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)) {
 			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;
 	}
-- 
2.30.0

Re: [PATCH net-next 2/2] virtio_net: Implement XDP_FLAGS_NO_TX support

[Shay Agroskin]

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)
...
>

Re: [PATCH net-next 2/2] virtio_net: Implement XDP_FLAGS_NO_TX support

[Charlie Somerville]

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);

Re: [PATCH net-next 2/2] virtio_net: Implement XDP_FLAGS_NO_TX support

[Shay Agroskin]

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

Re: [PATCH net-next 0/2] Introduce XDP_FLAGS_NO_TX flag

[Toke Høiland-Jørgensen]

Charlie Somerville <charlie@charlie.bz> writes:

> This patch series introduces a new flag XDP_FLAGS_NO_TX which prevents
> the allocation of additional send queues for XDP programs.
>
> Included in this patch series is an implementation of XDP_FLAGS_NO_TX
> for the virtio_net driver. This flag is intended to be advisory - not
> all drivers must implement support for it.
>
> Many virtualised environments only provide enough virtio_net send queues
> for the number of processors allocated to the VM:
>
> # nproc
> 8
> # ethtool --show-channels ens3
> Channel parameters for ens3:
> Pre-set maximums:
> RX:     0
> TX:     0
> Other:      0
> Combined:   8
>
> In this configuration XDP is unusable because the virtio_net driver
> always tries to allocate an extra send queue for each processor - even
> if the XDP the program never uses the XDP_TX functionality.
>
> While XDP_TX is still unavailable in these environments, this new flag
> expands the set of XDP programs that can be used.

I don't think adding a new flag is a good idea. Why can't you just fix
the driver?

-Toke

Re: [PATCH net-next 0/2] Introduce XDP_FLAGS_NO_TX flag

[Jason Wang]

On 2021/1/9 上午10:49, Charlie Somerville wrote:
> This patch series introduces a new flag XDP_FLAGS_NO_TX which prevents
> the allocation of additional send queues for XDP programs.


This part I don't understand. Is such flag a must? I think the answer is 
probably not.

Why not simply do:

1) if we had sufficient TX queues, use dedicated TX queues for XDP_TX
2) if we don't, simple synchronize through spin_lock[1]

Thanks

[1] https://www.spinics.net/lists/bpf/msg32587.html


>
> Included in this patch series is an implementation of XDP_FLAGS_NO_TX
> for the virtio_net driver. This flag is intended to be advisory - not
> all drivers must implement support for it.
>
> Many virtualised environments only provide enough virtio_net send queues
> for the number of processors allocated to the VM:
>
> # nproc
> 8
> # ethtool --show-channels ens3
> Channel parameters for ens3:
> Pre-set maximums:
> RX:     0
> TX:     0
> Other:      0
> Combined:   8
>
> In this configuration XDP is unusable because the virtio_net driver
> always tries to allocate an extra send queue for each processor - even
> if the XDP the program never uses the XDP_TX functionality.
>
> While XDP_TX is still unavailable in these environments, this new flag
> expands the set of XDP programs that can be used.
>
> This is my first contribution to the kernel, so apologies if I've sent
> this to the wrong list. I have tried to cc relevant maintainers but
> it's possible I may have missed some people. I'm looking forward to
> receiving feedback on this change.
>
> Charlie Somerville (2):
>    xdp: Add XDP_FLAGS_NO_TX flag
>    virtio_net: Implement XDP_FLAGS_NO_TX support
>
>   drivers/net/virtio_net.c     | 17 +++++++++++++----
>   include/uapi/linux/if_link.h |  5 ++++-
>   2 files changed, 17 insertions(+), 5 deletions(-)
>

Re: [PATCH net-next 0/2] Introduce XDP_FLAGS_NO_TX flag

[Charlie Somerville]

On Tue, Jan 12, 2021, at 14:03, Jason Wang wrote:
> 
> On 2021/1/9 上午10:49, Charlie Somerville wrote:
> > This patch series introduces a new flag XDP_FLAGS_NO_TX which prevents
> > the allocation of additional send queues for XDP programs.
> 
> 
> This part I don't understand. Is such flag a must? I think the answer is 
> probably not.
> 
> Why not simply do:
> 
> 1) if we had sufficient TX queues, use dedicated TX queues for XDP_TX
> 2) if we don't, simple synchronize through spin_lock[1]
> 
> Thanks
> 
> [1] https://www.spinics.net/lists/bpf/msg32587.html

The patch from Xuan Zhuo looks like a much better approach. I am happy to close this out in favour of that one! Thanks for the link.