netdev - Re: [PATCH v2] net: cache for same cpu skb_attempt_defer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 13 Feb 2024 19:13:41 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
 davem@...emloft.net, dsahern@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH v2] net: cache for same cpu skb_attempt_defer_free

On Tue, 13 Feb 2024 14:07:24 +0000 Pavel Begunkov wrote:
> >> +       local_bh_disable();
> >> +       skb_release_all(skb, SKB_DROP_REASON_NOT_SPECIFIED, false);  
> > 
> > I am trying to understand why we use false instead of true here ?
> > Or if you prefer:
> > local_bh_disable();
> > __napi_kfree_skb(skb, SKB_DROP_REASON_NOT_SPECIFIED);
> > local_bh_enable();  

FWIW I had the same reaction. napi_safe = false followed by
napi_skb_cache_put() looks sus. No argument that naming is bad,
not the first time it comes up :(

> Maybe it's my misunderstanding but disabled bh != "napi safe",
> e.g. the napi_struct we're interested in might be scheduled for
> another CPU. Which is also why "napi" prefix in percpu
> napi_alloc_cache sounds a bit misleading to me.

FWIW the skb recycling is called napi_* to hint to driver authors that
if they are in NAPI context this is a better function to call.

The connection to a particular NAPI instance matters only for the page
pool recycling, but that's handled. The conditions you actually
need to look out for are hardware IRQs and whatever async paths which
can trigger trigger while NAPI is half way thru touching the cache of
the local CPU.

> The second reason is that it shouldn't change anything
> performance wise
> 
> napi_pp_put_page(napi_safe) {
>      ...
>      if (napi_safe || in_softirq()) { ... }
> }