linux-kernel - Re: [PATCH net-next v15 06/13] mm: page_frag: reuse existing space for 'size' and 'pfmemalloc'

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: <CAKgT0UcEuYanVEaRViuJ5v8F7EXKJLr4_yP=ZkiMdamznt0FoQ@mail.gmail.com>
Date: Tue, 27 Aug 2024 11:16:40 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org
Subject: Re: [PATCH net-next v15 06/13] mm: page_frag: reuse existing space
 for 'size' and 'pfmemalloc'

On Tue, Aug 27, 2024 at 5:06 AM Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> On 2024/8/27 0:46, Alexander Duyck wrote:
> > On Mon, Aug 26, 2024 at 5:46 AM Yunsheng Lin <linyunsheng@...wei.com> wrote:
> >>
> >> Currently there is one 'struct page_frag' for every 'struct
> >> sock' and 'struct task_struct', we are about to replace the
> >> 'struct page_frag' with 'struct page_frag_cache' for them.
> >> Before begin the replacing, we need to ensure the size of
> >> 'struct page_frag_cache' is not bigger than the size of
> >> 'struct page_frag', as there may be tens of thousands of
> >> 'struct sock' and 'struct task_struct' instances in the
> >> system.
> >>
> >> By or'ing the page order & pfmemalloc with lower bits of
> >> 'va' instead of using 'u16' or 'u32' for page size and 'u8'
> >> for pfmemalloc, we are able to avoid 3 or 5 bytes space waste.
> >> And page address & pfmemalloc & order is unchanged for the
> >> same page in the same 'page_frag_cache' instance, it makes
> >> sense to fit them together.
> >>
> >> After this patch, the size of 'struct page_frag_cache' should be
> >> the same as the size of 'struct page_frag'.
> >>
> >> CC: Alexander Duyck <alexander.duyck@...il.com>
> >> Signed-off-by: Yunsheng Lin <linyunsheng@...wei.com>
> >> ---
> >>  include/linux/mm_types_task.h   | 19 ++++++-----
> >>  include/linux/page_frag_cache.h | 60 +++++++++++++++++++++++++++++++--
> >>  mm/page_frag_cache.c            | 51 +++++++++++++++-------------
> >>  3 files changed, 97 insertions(+), 33 deletions(-)
> >>

...

> >>  void page_frag_cache_drain(struct page_frag_cache *nc);
> >
> > So how many of these additions are actually needed outside of the
> > page_frag_cache.c file? I'm just wondering if we could look at moving
>
> At least page_frag_cache_is_pfmemalloc(), page_frag_encoded_page_order(),
> page_frag_encoded_page_ptr(), page_frag_encoded_page_address() are needed
> out of the page_frag_cache.c file for now, which are used mostly in
> __page_frag_cache_commit() and __page_frag_alloc_refill_probe_align() for
> debugging and performance reason, see patch 7 & 10.

As far as the __page_frag_cache_commit I might say that could be moved
to page_frag_cache.c, but admittedly I don't know how much that would
impact the performance.

> The only left one is page_frag_encode_page(), I am not sure if it makes
> much sense to move it to page_frag_cache.c while the rest of them are in
> .h file.

I would move it. There is no point in exposing internals more than
necessary. Also since you are carrying a BUILD_BUG_ON it would make
sense to keep that internal to your implementation.