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

[Jon Kohler <jon@nutanix.com>]

> 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