[PATCH net-next 1/4] tun: rcu_deference xdp_prog only once per batch

[PATCH net-next 1/4] tun: rcu_deference xdp_prog only once per batch

[Jon Kohler]

Hoist rcu_dereference(tun->xdp_prog) out of tun_xdp_one, so that
rcu_deference is called once during batch processing.

No functional change intended.

Signed-off-by: Jon Kohler <jon@nutanix.com>
---
 drivers/net/tun.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 7babd1e9a378..87fc51916fce 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2353,12 +2353,12 @@ static void tun_put_page(struct tun_page *tpage)
 static int tun_xdp_one(struct tun_struct *tun,
 		       struct tun_file *tfile,
 		       struct xdp_buff *xdp, int *flush,
-		       struct tun_page *tpage)
+		       struct tun_page *tpage,
+		       struct bpf_prog *xdp_prog)
 {
 	unsigned int datasize = xdp->data_end - xdp->data;
 	struct tun_xdp_hdr *hdr = xdp->data_hard_start;
 	struct virtio_net_hdr *gso = &hdr->gso;
-	struct bpf_prog *xdp_prog;
 	struct sk_buff *skb = NULL;
 	struct sk_buff_head *queue;
 	u32 rxhash = 0, act;
@@ -2371,7 +2371,6 @@ static int tun_xdp_one(struct tun_struct *tun,
 	if (unlikely(datasize < ETH_HLEN))
 		return -EINVAL;
 
-	xdp_prog = rcu_dereference(tun->xdp_prog);
 	if (xdp_prog) {
 		if (gso->gso_type) {
 			skb_xdp = true;
@@ -2494,6 +2493,7 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
 	if (m->msg_controllen == sizeof(struct tun_msg_ctl) &&
 	    ctl && ctl->type == TUN_MSG_PTR) {
 		struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx;
+		struct bpf_prog *xdp_prog;
 		struct tun_page tpage;
 		int n = ctl->num;
 		int flush = 0, queued = 0;
@@ -2503,10 +2503,12 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
 		local_bh_disable();
 		rcu_read_lock();
 		bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
+		xdp_prog = rcu_dereference(tun->xdp_prog);
 
 		for (i = 0; i < n; i++) {
 			xdp = &((struct xdp_buff *)ctl->ptr)[i];
-			ret = tun_xdp_one(tun, tfile, xdp, &flush, &tpage);
+			ret = tun_xdp_one(tun, tfile, xdp, &flush, &tpage,
+					  xdp_prog);
 			if (ret > 0)
 				queued += ret;
 		}
-- 
2.43.0

[PATCH net-next 2/4] tun: optimize skb allocation in tun_xdp_one

[Jon Kohler]

Enhance TUN_MSG_PTR batch processing by leveraging bulk allocation from
the per-CPU NAPI cache via napi_skb_cache_get_bulk. This improves
efficiency by reducing allocation overhead and is especially useful
when using IFF_NAPI and GRO is able to feed the cache entries back.

Handle scenarios where full preallocation of SKBs is not possible by
gracefully dropping only the uncovered portion of the batch payload.

Cc: Alexander Lobakin <aleksander.lobakin@intel.com>
Signed-off-by: Jon Kohler <jon@nutanix.com>
---
 drivers/net/tun.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 87fc51916fce..f7f7490e78dc 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2354,12 +2354,12 @@ static int tun_xdp_one(struct tun_struct *tun,
 		       struct tun_file *tfile,
 		       struct xdp_buff *xdp, int *flush,
 		       struct tun_page *tpage,
-		       struct bpf_prog *xdp_prog)
+		       struct bpf_prog *xdp_prog,
+		       struct sk_buff *skb)
 {
 	unsigned int datasize = xdp->data_end - xdp->data;
 	struct tun_xdp_hdr *hdr = xdp->data_hard_start;
 	struct virtio_net_hdr *gso = &hdr->gso;
-	struct sk_buff *skb = NULL;
 	struct sk_buff_head *queue;
 	u32 rxhash = 0, act;
 	int buflen = hdr->buflen;
@@ -2381,16 +2381,15 @@ static int tun_xdp_one(struct tun_struct *tun,
 
 		act = bpf_prog_run_xdp(xdp_prog, xdp);
 		ret = tun_xdp_act(tun, xdp_prog, xdp, act);
-		if (ret < 0) {
-			put_page(virt_to_head_page(xdp->data));
+		if (ret < 0)
 			return ret;
-		}
 
 		switch (ret) {
 		case XDP_REDIRECT:
 			*flush = true;
 			fallthrough;
 		case XDP_TX:
+			napi_consume_skb(skb, 1);
 			return 0;
 		case XDP_PASS:
 			break;
@@ -2403,13 +2402,14 @@ static int tun_xdp_one(struct tun_struct *tun,
 				tpage->page = page;
 				tpage->count = 1;
 			}
+			napi_consume_skb(skb, 1);
 			return 0;
 		}
 	}
 
 build:
-	skb = build_skb(xdp->data_hard_start, buflen);
-	if (!skb) {
+	skb = build_skb_around(skb, xdp->data_hard_start, buflen);
+	if (unlikely(!skb)) {
 		ret = -ENOMEM;
 		goto out;
 	}
@@ -2427,7 +2427,6 @@ static int tun_xdp_one(struct tun_struct *tun,
 
 	if (tun_vnet_hdr_to_skb(tun->flags, skb, gso)) {
 		atomic_long_inc(&tun->rx_frame_errors);
-		kfree_skb(skb);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -2455,7 +2454,6 @@ static int tun_xdp_one(struct tun_struct *tun,
 
 		if (unlikely(tfile->detached)) {
 			spin_unlock(&queue->lock);
-			kfree_skb(skb);
 			return -EBUSY;
 		}
 
@@ -2496,7 +2494,9 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
 		struct bpf_prog *xdp_prog;
 		struct tun_page tpage;
 		int n = ctl->num;
-		int flush = 0, queued = 0;
+		int flush = 0, queued = 0, num_skbs = 0;
+		/* Max size of VHOST_NET_BATCH */
+		void *skbs[64];
 
 		memset(&tpage, 0, sizeof(tpage));
 
@@ -2505,12 +2505,27 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
 		bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
 		xdp_prog = rcu_dereference(tun->xdp_prog);
 
-		for (i = 0; i < n; i++) {
+		num_skbs = napi_skb_cache_get_bulk(skbs, n);
+
+		for (i = 0; i < num_skbs; i++) {
+			struct sk_buff *skb = skbs[i];
 			xdp = &((struct xdp_buff *)ctl->ptr)[i];
 			ret = tun_xdp_one(tun, tfile, xdp, &flush, &tpage,
-					  xdp_prog);
+					  xdp_prog, skb);
 			if (ret > 0)
 				queued += ret;
+			else if (ret < 0) {
+				dev_core_stats_rx_dropped_inc(tun->dev);
+				napi_consume_skb(skb, 1);
+				put_page(virt_to_head_page(xdp->data));
+			}
+		}
+
+		/* Handle remaining xdp_buff entries if num_skbs < ctl->num */
+		for (i = num_skbs; i < ctl->num; i++) {
+			xdp = &((struct xdp_buff *)ctl->ptr)[i];
+			dev_core_stats_rx_dropped_inc(tun->dev);
+			put_page(virt_to_head_page(xdp->data));
 		}
 
 		if (flush)
-- 
2.43.0

[PATCH net-next 3/4] tun: use napi_build_skb in __tun_build_skb

[Jon Kohler]

Use napi_build_skb for small payload SKBs that end up using the
tun_build_skb path.

Signed-off-by: Jon Kohler <jon@nutanix.com>
---
 drivers/net/tun.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index f7f7490e78dc..7b13d4bf5374 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1538,7 +1538,11 @@ static struct sk_buff *__tun_build_skb(struct tun_file *tfile,
 				       int buflen, int len, int pad,
 				       int metasize)
 {
-	struct sk_buff *skb = build_skb(buf, buflen);
+	struct sk_buff *skb;
+
+	local_bh_disable();
+	skb = napi_build_skb(buf, buflen);
+	local_bh_enable();
 
 	if (!skb)
 		return ERR_PTR(-ENOMEM);
-- 
2.43.0

[PATCH net-next 4/4] tun: use napi_consume_skb in tun_do_read

[Jon Kohler]

Now that we have the build_skb paths using local NAPI cache, use
napi_consume_skb in tun_do_read, so that the local cache gets refilled
on read. This is especially useful in the vhost worker use case where
the RX and TX paths are running on the same worker kthread.

Signed-off-by: Jon Kohler <jon@nutanix.com>
---
 drivers/net/tun.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 7b13d4bf5374..f85115383667 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2163,10 +2163,13 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
 		struct sk_buff *skb = ptr;
 
 		ret = tun_put_user(tun, tfile, skb, to);
-		if (unlikely(ret < 0))
+		if (ret >= 0) {
+			local_bh_disable();
+			napi_consume_skb(skb, 1);
+			local_bh_enable();
+		} else {
 			kfree_skb(skb);
-		else
-			consume_skb(skb);
+		}
 	}
 
 	return ret;
-- 
2.43.0

Re: [PATCH net-next 1/4] tun: rcu_deference xdp_prog only once per batch

[Willem de Bruijn]

Jon Kohler wrote:
> Hoist rcu_dereference(tun->xdp_prog) out of tun_xdp_one, so that
> rcu_deference is called once during batch processing.

I'm skeptical that this does anything.

The compiler can inline tun_xdp_one and indeed seems to do so. And
then it can cache the read in a register if that is the best use of
a register.

> 
> No functional change intended.
> 
> Signed-off-by: Jon Kohler <jon@nutanix.com>

Re: [PATCH net-next 3/4] tun: use napi_build_skb in __tun_build_skb

[Willem de Bruijn]

Jon Kohler wrote:
> Use napi_build_skb for small payload SKBs that end up using the
> tun_build_skb path.
> 
> Signed-off-by: Jon Kohler <jon@nutanix.com>
> ---
>  drivers/net/tun.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index f7f7490e78dc..7b13d4bf5374 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1538,7 +1538,11 @@ static struct sk_buff *__tun_build_skb(struct tun_file *tfile,
>  				       int buflen, int len, int pad,
>  				       int metasize)
>  {
> -	struct sk_buff *skb = build_skb(buf, buflen);
> +	struct sk_buff *skb;
> +
> +	local_bh_disable();
> +	skb = napi_build_skb(buf, buflen);
> +	local_bh_enable();

The goal of this whole series seems to be to use the percpu skb cache
for bulk alloc.

As all these helpers' prefix indicates, they are meant to be used with
NAPI. Not sure using them on a tun write() datapath is deemed
acceptable. Or even correct. Perhaps the infrastructure authors have
an opinion.

From commit 795bb1c00dd3 ("net: bulk free infrastructure for NAPI
context, use napi_consume_skb") it does appear that technically all
that is needed is to be called in softirq context.

Re: [PATCH net-next 2/4] tun: optimize skb allocation in tun_xdp_one

[Willem de Bruijn]

Jon Kohler wrote:
> Enhance TUN_MSG_PTR batch processing by leveraging bulk allocation from
> the per-CPU NAPI cache via napi_skb_cache_get_bulk. This improves
> efficiency by reducing allocation overhead and is especially useful
> when using IFF_NAPI and GRO is able to feed the cache entries back.
> 
> Handle scenarios where full preallocation of SKBs is not possible by
> gracefully dropping only the uncovered portion of the batch payload.
> 
> Cc: Alexander Lobakin <aleksander.lobakin@intel.com>
> Signed-off-by: Jon Kohler <jon@nutanix.com>
> ---
>  drivers/net/tun.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 87fc51916fce..f7f7490e78dc 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2354,12 +2354,12 @@ static int tun_xdp_one(struct tun_struct *tun,
>  		       struct tun_file *tfile,
>  		       struct xdp_buff *xdp, int *flush,
>  		       struct tun_page *tpage,
> -		       struct bpf_prog *xdp_prog)
> +		       struct bpf_prog *xdp_prog,
> +		       struct sk_buff *skb)
>  {
>  	unsigned int datasize = xdp->data_end - xdp->data;
>  	struct tun_xdp_hdr *hdr = xdp->data_hard_start;
>  	struct virtio_net_hdr *gso = &hdr->gso;
> -	struct sk_buff *skb = NULL;
>  	struct sk_buff_head *queue;
>  	u32 rxhash = 0, act;
>  	int buflen = hdr->buflen;
> @@ -2381,16 +2381,15 @@ static int tun_xdp_one(struct tun_struct *tun,
>  
>  		act = bpf_prog_run_xdp(xdp_prog, xdp);
>  		ret = tun_xdp_act(tun, xdp_prog, xdp, act);
> -		if (ret < 0) {
> -			put_page(virt_to_head_page(xdp->data));
> +		if (ret < 0)
>  			return ret;
> -		}
>  
>  		switch (ret) {
>  		case XDP_REDIRECT:
>  			*flush = true;
>  			fallthrough;
>  		case XDP_TX:
> +			napi_consume_skb(skb, 1);
>  			return 0;
>  		case XDP_PASS:
>  			break;
> @@ -2403,13 +2402,14 @@ static int tun_xdp_one(struct tun_struct *tun,
>  				tpage->page = page;
>  				tpage->count = 1;
>  			}
> +			napi_consume_skb(skb, 1);
>  			return 0;
>  		}
>  	}
>  
>  build:
> -	skb = build_skb(xdp->data_hard_start, buflen);
> -	if (!skb) {
> +	skb = build_skb_around(skb, xdp->data_hard_start, buflen);
> +	if (unlikely(!skb)) {
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> @@ -2427,7 +2427,6 @@ static int tun_xdp_one(struct tun_struct *tun,
>  
>  	if (tun_vnet_hdr_to_skb(tun->flags, skb, gso)) {
>  		atomic_long_inc(&tun->rx_frame_errors);
> -		kfree_skb(skb);
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -2455,7 +2454,6 @@ static int tun_xdp_one(struct tun_struct *tun,
>  
>  		if (unlikely(tfile->detached)) {
>  			spin_unlock(&queue->lock);
> -			kfree_skb(skb);
>  			return -EBUSY;
>  		}
>  
> @@ -2496,7 +2494,9 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>  		struct bpf_prog *xdp_prog;
>  		struct tun_page tpage;
>  		int n = ctl->num;
> -		int flush = 0, queued = 0;
> +		int flush = 0, queued = 0, num_skbs = 0;
> +		/* Max size of VHOST_NET_BATCH */
> +		void *skbs[64];
>  
>  		memset(&tpage, 0, sizeof(tpage));
>  
> @@ -2505,12 +2505,27 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>  		bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
>  		xdp_prog = rcu_dereference(tun->xdp_prog);
>  
> -		for (i = 0; i < n; i++) {
> +		num_skbs = napi_skb_cache_get_bulk(skbs, n);
> +
> +		for (i = 0; i < num_skbs; i++) {
> +			struct sk_buff *skb = skbs[i];
>  			xdp = &((struct xdp_buff *)ctl->ptr)[i];
>  			ret = tun_xdp_one(tun, tfile, xdp, &flush, &tpage,
> -					  xdp_prog);
> +					  xdp_prog, skb);
>  			if (ret > 0)
>  				queued += ret;
> +			else if (ret < 0) {
> +				dev_core_stats_rx_dropped_inc(tun->dev);
> +				napi_consume_skb(skb, 1);
> +				put_page(virt_to_head_page(xdp->data));
> +			}
> +		}
> +
> +		/* Handle remaining xdp_buff entries if num_skbs < ctl->num */
> +		for (i = num_skbs; i < ctl->num; i++) {
> +			xdp = &((struct xdp_buff *)ctl->ptr)[i];
> +			dev_core_stats_rx_dropped_inc(tun->dev);
> +			put_page(virt_to_head_page(xdp->data));

The code should attempt to send out packets rather than drop them.

Re: [PATCH net-next 2/4] tun: optimize skb allocation in tun_xdp_one

[Jon Kohler]

> On May 7, 2025, at 4:50 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> 
> !-------------------------------------------------------------------|
>  CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
> Jon Kohler wrote:
>> Enhance TUN_MSG_PTR batch processing by leveraging bulk allocation from
>> the per-CPU NAPI cache via napi_skb_cache_get_bulk. This improves
>> efficiency by reducing allocation overhead and is especially useful
>> when using IFF_NAPI and GRO is able to feed the cache entries back.
>> 
>> Handle scenarios where full preallocation of SKBs is not possible by
>> gracefully dropping only the uncovered portion of the batch payload.
>> 
>> Cc: Alexander Lobakin <aleksander.lobakin@intel.com>
>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>> ---
>> drivers/net/tun.c | 39 +++++++++++++++++++++++++++------------
>> 1 file changed, 27 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 87fc51916fce..f7f7490e78dc 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -2354,12 +2354,12 @@ static int tun_xdp_one(struct tun_struct *tun,
>>       struct tun_file *tfile,
>>       struct xdp_buff *xdp, int *flush,
>>       struct tun_page *tpage,
>> -       struct bpf_prog *xdp_prog)
>> +       struct bpf_prog *xdp_prog,
>> +       struct sk_buff *skb)
>> {
>> unsigned int datasize = xdp->data_end - xdp->data;
>> struct tun_xdp_hdr *hdr = xdp->data_hard_start;
>> struct virtio_net_hdr *gso = &hdr->gso;
>> - struct sk_buff *skb = NULL;
>> struct sk_buff_head *queue;
>> u32 rxhash = 0, act;
>> int buflen = hdr->buflen;
>> @@ -2381,16 +2381,15 @@ static int tun_xdp_one(struct tun_struct *tun,
>> 
>> act = bpf_prog_run_xdp(xdp_prog, xdp);
>> ret = tun_xdp_act(tun, xdp_prog, xdp, act);
>> - if (ret < 0) {
>> - put_page(virt_to_head_page(xdp->data));
>> + if (ret < 0)
>> return ret;
>> - }
>> 
>> switch (ret) {
>> case XDP_REDIRECT:
>> *flush = true;
>> fallthrough;
>> case XDP_TX:
>> + napi_consume_skb(skb, 1);
>> return 0;
>> case XDP_PASS:
>> break;
>> @@ -2403,13 +2402,14 @@ static int tun_xdp_one(struct tun_struct *tun,
>> tpage->page = page;
>> tpage->count = 1;
>> }
>> + napi_consume_skb(skb, 1);
>> return 0;
>> }
>> }
>> 
>> build:
>> - skb = build_skb(xdp->data_hard_start, buflen);
>> - if (!skb) {
>> + skb = build_skb_around(skb, xdp->data_hard_start, buflen);
>> + if (unlikely(!skb)) {
>> ret = -ENOMEM;
>> goto out;
>> }
>> @@ -2427,7 +2427,6 @@ static int tun_xdp_one(struct tun_struct *tun,
>> 
>> if (tun_vnet_hdr_to_skb(tun->flags, skb, gso)) {
>> atomic_long_inc(&tun->rx_frame_errors);
>> - kfree_skb(skb);
>> ret = -EINVAL;
>> goto out;
>> }
>> @@ -2455,7 +2454,6 @@ static int tun_xdp_one(struct tun_struct *tun,
>> 
>> if (unlikely(tfile->detached)) {
>> spin_unlock(&queue->lock);
>> - kfree_skb(skb);
>> return -EBUSY;
>> }
>> 
>> @@ -2496,7 +2494,9 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>> struct bpf_prog *xdp_prog;
>> struct tun_page tpage;
>> int n = ctl->num;
>> - int flush = 0, queued = 0;
>> + int flush = 0, queued = 0, num_skbs = 0;
>> + /* Max size of VHOST_NET_BATCH */
>> + void *skbs[64];
>> 
>> memset(&tpage, 0, sizeof(tpage));
>> 
>> @@ -2505,12 +2505,27 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>> bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
>> xdp_prog = rcu_dereference(tun->xdp_prog);
>> 
>> - for (i = 0; i < n; i++) {
>> + num_skbs = napi_skb_cache_get_bulk(skbs, n);
>> +
>> + for (i = 0; i < num_skbs; i++) {
>> + struct sk_buff *skb = skbs[i];
>> xdp = &((struct xdp_buff *)ctl->ptr)[i];
>> ret = tun_xdp_one(tun, tfile, xdp, &flush, &tpage,
>> -  xdp_prog);
>> +  xdp_prog, skb);
>> if (ret > 0)
>> queued += ret;
>> + else if (ret < 0) {
>> + dev_core_stats_rx_dropped_inc(tun->dev);
>> + napi_consume_skb(skb, 1);
>> + put_page(virt_to_head_page(xdp->data));
>> + }
>> + }
>> +
>> + /* Handle remaining xdp_buff entries if num_skbs < ctl->num */
>> + for (i = num_skbs; i < ctl->num; i++) {
>> + xdp = &((struct xdp_buff *)ctl->ptr)[i];
>> + dev_core_stats_rx_dropped_inc(tun->dev);
>> + put_page(virt_to_head_page(xdp->data));
> 
> The code should attempt to send out packets rather than drop them.
> 

I took a hint from the other two places that currently do bulk SKB
allocation drop at least some of their payloads on allocation failure.
See the other call sites for napi_skb_cache_get_bulk for reference.

Also, this is similar to what the code already does today, because
if build_skb fails today, tun_xdp_one just ENOMEM’s and doesn’t go
any further.

This code is at least somewhat better than today, because this code
will increment the drop counters, vs silently dropped today.

Happy to take a pointer if there is a suggested retry mechanism of
sorts?

Re: [PATCH net-next 3/4] tun: use napi_build_skb in __tun_build_skb

[Jon Kohler]

> On May 7, 2025, at 4:50 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> 
> !-------------------------------------------------------------------|
>  CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
> Jon Kohler wrote:
>> Use napi_build_skb for small payload SKBs that end up using the
>> tun_build_skb path.
>> 
>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>> ---
>> drivers/net/tun.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index f7f7490e78dc..7b13d4bf5374 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1538,7 +1538,11 @@ static struct sk_buff *__tun_build_skb(struct tun_file *tfile,
>>       int buflen, int len, int pad,
>>       int metasize)
>> {
>> - struct sk_buff *skb = build_skb(buf, buflen);
>> + struct sk_buff *skb;
>> +
>> + local_bh_disable();
>> + skb = napi_build_skb(buf, buflen);
>> + local_bh_enable();
> 
> The goal of this whole series seems to be to use the percpu skb cache
> for bulk alloc.

Yes

> 
> As all these helpers' prefix indicates, they are meant to be used with
> NAPI. Not sure using them on a tun write() datapath is deemed
> acceptable. Or even correct. Perhaps the infrastructure authors have
> an opinion.

@Alexsander: thoughts on this one? Following in the footsteps of
cpu_map_kthread_run, that appears to be its own things, non-NAPI,
and "bpf: cpumap: switch to napi_skb_cache_get_bulk()” simply wrapped
that whole area with local_bh_disable

> 
> From commit 795bb1c00dd3 ("net: bulk free infrastructure for NAPI
> context, use napi_consume_skb") it does appear that technically all
> that is needed is to be called in softirq context.

I saw the same thing, it appears that it isn’t NAPI constrained (like say
napi_alloc_skb, which takes napi arg), but rather softirq and that’s that.

My initial read on all of this was that the napi_ prefix was the original
intent and it morphed out of that over time (to just be softirq protected).
Always happy to learn more if I’ve misread the situation

Re: [PATCH net-next 1/4] tun: rcu_deference xdp_prog only once per batch

[Jon Kohler]

> On May 7, 2025, at 4:43 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> 
> !-------------------------------------------------------------------|
>  CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
> Jon Kohler wrote:
>> Hoist rcu_dereference(tun->xdp_prog) out of tun_xdp_one, so that
>> rcu_deference is called once during batch processing.
> 
> I'm skeptical that this does anything.
> 
> The compiler can inline tun_xdp_one and indeed seems to do so. And
> then it can cache the read in a register if that is the best use of
> a register.

The thought here is that if a compiler decided to not-inline tun_xdp_one
(perhaps it grew to big, or the compiler was being sassy), that the intent
would simply be that this wants to be called once-and-only-once. This
change just makes that intent more clear, and is a nice little cleanup.

I’ve got a series that stacks on top of this that enables multi-buffer support
and I can keep an eye on if that gets inlined or not.

> 
>> 
>> No functional change intended.
>> 
>> Signed-off-by: Jon Kohler <jon@nutanix.com>

Re: [PATCH net-next 1/4] tun: rcu_deference xdp_prog only once per batch

[Willem de Bruijn]

Jon Kohler wrote:
> 
> 
> > On May 7, 2025, at 4:43 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> > 
> > !-------------------------------------------------------------------|
> >  CAUTION: External Email
> > 
> > |-------------------------------------------------------------------!
> > 
> > Jon Kohler wrote:
> >> Hoist rcu_dereference(tun->xdp_prog) out of tun_xdp_one, so that
> >> rcu_deference is called once during batch processing.
> > 
> > I'm skeptical that this does anything.
> > 
> > The compiler can inline tun_xdp_one and indeed seems to do so. And
> > then it can cache the read in a register if that is the best use of
> > a register.
> 
> The thought here is that if a compiler decided to not-inline tun_xdp_one
> (perhaps it grew to big, or the compiler was being sassy), that the intent
> would simply be that this wants to be called once-and-only-once. This
> change just makes that intent more clear, and is a nice little cleanup.
> 
> I’ve got a series that stacks on top of this that enables multi-buffer support
> and I can keep an eye on if that gets inlined or not.

That will only give you one outcome with a specific compiler, platform
and build configuration.

I would just drop this and let the compiler handle such optimizations.

Re: [PATCH net-next 1/4] tun: rcu_deference xdp_prog only once per batch

[Jon Kohler]

> On May 8, 2025, at 9:31 AM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> 
> !-------------------------------------------------------------------|
>  CAUTION: External Email
> 
> |-------------------------------------------------------------------!
> 
> Jon Kohler wrote:
>> 
>> 
>>> On May 7, 2025, at 4:43 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
>>> 
>>> !-------------------------------------------------------------------|
>>> CAUTION: External Email
>>> 
>>> |-------------------------------------------------------------------!
>>> 
>>> Jon Kohler wrote:
>>>> Hoist rcu_dereference(tun->xdp_prog) out of tun_xdp_one, so that
>>>> rcu_deference is called once during batch processing.
>>> 
>>> I'm skeptical that this does anything.
>>> 
>>> The compiler can inline tun_xdp_one and indeed seems to do so. And
>>> then it can cache the read in a register if that is the best use of
>>> a register.
>> 
>> The thought here is that if a compiler decided to not-inline tun_xdp_one
>> (perhaps it grew to big, or the compiler was being sassy), that the intent
>> would simply be that this wants to be called once-and-only-once. This
>> change just makes that intent more clear, and is a nice little cleanup.
>> 
>> I’ve got a series that stacks on top of this that enables multi-buffer support
>> and I can keep an eye on if that gets inlined or not.
> 
> That will only give you one outcome with a specific compiler, platform
> and build configuration.
> 
> I would just drop this and let the compiler handle such optimizations.

Ok, thanks for the feedback, will do