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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAKgT0Uf-fDHD_g75PSO591WVHdtHuUJ+L=aWBWoiM3vHyzxRtw@mail.gmail.com>
Date:   Wed, 21 Sep 2022 14:44:22 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, 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, Sep 21, 2022 at 1:52 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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.

Why do you need to duplicate it? I thought you would either be going
with nc->page or nc->page_small depending on the size so either way
you are accessing nc. Once you know you aren't going to be using the
slab cache you could basically fetch that and do the setting of
__GFP_MEMALLOC before you would even need to look at branching based
on the length. The branch on size would then assign the
page_frag_cache pointer, update the length, fetch the page frag, and
then resume the normal path.