Re: [PATCH net-next v2 5/9] tun: use bulk NAPI cache allocation in tun_xdp_one

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 02/12/2025 17.49, Jon Kohler wrote:
> 
> 
>> On Nov 27, 2025, at 10:02 PM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On Wed, Nov 26, 2025 at 3:19 AM Jon Kohler <jon@nutanix.com> wrote:
>>>
>>> Optimize TUN_MSG_PTR batch processing by allocating sk_buff structures
>>> in bulk from the per-CPU NAPI cache using napi_skb_cache_get_bulk.
>>> This reduces allocation overhead and improves efficiency, especially
>>> when IFF_NAPI is enabled and GRO is feeding entries back to the cache.
>>
>> Does this mean we should only enable this when NAPI is used?
> 
> No, it does not mean that at all, but I see what that would be confusing.
> I can clean up the commit msg on the next go around
> 
>>>
>>> If bulk allocation cannot fully satisfy the batch, gracefully drop only
>>> the uncovered portion, allowing the rest of the batch to proceed, which
>>> is what already happens in the previous case where build_skb() would
>>> fail and return -ENOMEM.
>>>
>>> Signed-off-by: Jon Kohler <jon@nutanix.com>
>>
>> Do we have any benchmark result for this?
> 
> Yes, it is in the cover letter:
> https://patchwork.kernel.org/project/netdevbpf/cover/20251125200041.1565663-1-jon@nutanix.com/
> 
>>> ---
>>> drivers/net/tun.c | 30 ++++++++++++++++++++++++------
>>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index 97f130bc5fed..64f944cce517 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
[...]
>>> @@ -2454,6 +2455,7 @@ static int tun_xdp_one(struct tun_struct *tun,
>>>                 ret = tun_xdp_act(tun, xdp_prog, xdp, act);
>>>                 if (ret < 0) {
>>>                         /* tun_xdp_act already handles drop statistics */
>>> +                       kfree_skb_reason(skb, SKB_DROP_REASON_XDP);
>>
>> This should belong to previous patches?
> 
> Well, not really, as we did not even have an SKB to free at this point
> in the previous code
>>
>>>                         put_page(virt_to_head_page(xdp->data));

This calling put_page() directly also looks dubious.

>>>                         return ret;
>>>                 }
>>> @@ -2463,6 +2465,7 @@ static int tun_xdp_one(struct tun_struct *tun,
>>>                         *flush = true;
>>>                         fallthrough;
>>>                 case XDP_TX:
>>> +                       napi_consume_skb(skb, 1);
>>>                         return 0;
>>>                 case XDP_PASS:
>>>                         break;
>>> @@ -2475,13 +2478,15 @@ static int tun_xdp_one(struct tun_struct *tun,
>>>                                 tpage->page = page;
>>>                                 tpage->count = 1;
>>>                         }
>>> +                       napi_consume_skb(skb, 1);
>>
>> I wonder if this would have any side effects since tun_xdp_one() is
>> not called by a NAPI.
> 
> As far as I can tell, this napi_consume_skb is really just an artifact of
> how it was named and how it was traditionally used.
> 
> Now this is really just a napi_consume_skb within a bh disable/enable
> section, which should meet the requirements of how that interface
> should be used (again, AFAICT)
> 

Yicks - this sounds super ugly.  Just wrapping napi_consume_skb() in bh
disable/enable section and then assuming you get the same protection as
NAPI is really dubious.

Cc Sebastian as he is trying to cleanup these kind of use-case, to make
kernel preemption work.


>>
>>>                         return 0;
>>>                 }
>>>         }
>>>
>>> build:
>>> -       skb = build_skb(xdp->data_hard_start, buflen);
>>> +       skb = build_skb_around(skb, xdp->data_hard_start, buflen);
>>>         if (!skb) {
>>> +               kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
> 
> Though to your point, I dont think this actually does anything given
> that if the skb was somehow nuked as part of build_skb_around, there
> would not be an skb to free. Doesn’t hurt though, from a self documenting
> code perspective tho?
> 
>>>                 dev_core_stats_rx_dropped_inc(tun->dev);
>>>                 return -ENOMEM;
>>>         }
>>> @@ -2566,9 +2571,11 @@ 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;
>>> +               int flush = 0, queued = 0, num_skbs = 0;
>>>                 struct tun_page tpage;
>>>                 int n = ctl->num;
>>> -               int flush = 0, queued = 0;
>>> +               /* Max size of VHOST_NET_BATCH */
>>> +               void *skbs[64];
>>
>> I think we need some tweaks
>>
>> 1) TUN is decoupled from vhost, so it should have its own value (a
>> macro is better)
> 
> Sure, I can make another constant that does a similar thing
> 
>> 2) Provide a way to fail or handle the case when more than 64
> 
> What if we simply assert that the maximum here is 64, which I think
> is what it actually is in practice?
> 
>>
>>>
>>>                 memset(&tpage, 0, sizeof(tpage));
>>>
>>> @@ -2576,13 +2583,24 @@ static int tun_sendmsg(struct socket *sock, struct msghdr *m, size_t total_len)
>>>                 rcu_read_lock();
>>>                 bpf_net_ctx = bpf_net_ctx_set(&__bpf_net_ctx);
>>>
>>> -               for (i = 0; i < n; i++) {
>>> +               num_skbs = napi_skb_cache_get_bulk(skbs, n);
>>
>> Its document said:
>>
>> """
>> * Must be called *only* from the BH context.
>> “"”
> We’re in a bh_disable section here, is that not good enough?

Again this feels very ugly and prone to painting ourselves into a
corner, assuming BH-disabled sections have same protection as NAPI.

(The napi_skb_cache_get/put function are operating on per CPU arrays
without any locking.)

--Jesper