netdev - Re: [PATCH net-next] net: skb: introduce and use a single page frag cache

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 21 Sep 2022 22:51:58 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Alexander H Duyck <alexander.duyck@...il.com>,
        netdev@...r.kernel.org
Cc:     Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] net: skb: introduce and use a single page frag
 cache

On Wed, 2022-09-21 at 13:23 -0700, Alexander H Duyck wrote:
> On Wed, 2022-09-21 at 21:33 +0200, Paolo Abeni wrote:
> > Nice! I'll use that in v2, with page_ref_add(page, offset / SZ_1K - 1);
> > or we will leak the page.
> 
> No, the offset already takes care of the -1 via the "- SZ_1K". What we
> are adding is references for the unused offset.

You are right. For some reasons I keep reading PAGE_SIZE instead of
'offset'.

> > > 
> It occurs to me that I think you are missing the check for the gfp_mask
> and the reclaim and DMA flags values as a result with your change. I
> think we will need to perform that check before we can do the direct
> page allocation based on size.

Yes, the gtp_mask checks are required (it just stuck me a few moments
ago ;). I will move the code as you originally suggested.

> > > 
> > Why? in the end we will still use an ancillary variable and the
> > napi_alloc_cache struct will be bigger (probaly not very relevant, but
> > for no gain at all).
> 
> It was mostly just about reducing instructions. The thought is we could
> get rid of the storage of the napi cache entirely since the only thing
> used is the page member, so if we just passed that around instead it
> would save us the trouble and not really be another variable. Basically
> we would be passing a frag cache pointer instead of a napi_alloc_cache.

In that case we will still duplicate a bit of code  -
this_cpu_ptr(&napi_alloc_cache) on both branches. gcc 11.3.1 here says
that the generated code is smaller without this change.

Cheers,

Paolo