Re: [PATCH v2] regset: use kvzalloc() for regset_get_alloc()

[Doug Anderson <dianders@chromium.org>]

Hi,

On Mon, Feb 5, 2024 at 9:27 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> While browsing through ChromeOS crash reports, I found one with an
> allocation failure that looked like this:
>
>   chrome: page allocation failure: order:7,
>           mode:0x40dc0(GFP_KERNEL|__GFP_COMP|__GFP_ZERO),
>           nodemask=(null),cpuset=urgent,mems_allowed=0
>   CPU: 7 PID: 3295 Comm: chrome Not tainted
>           5.15.133-20574-g8044615ac35c #1 (HASH:1162 1)
>   Hardware name: Google Lazor (rev3 - 8) with KB Backlight (DT)
>   Call trace:
>   ...
>   warn_alloc+0x104/0x174
>   __alloc_pages+0x5f0/0x6e4
>   kmalloc_order+0x44/0x98
>   kmalloc_order_trace+0x34/0x124
>   __kmalloc+0x228/0x36c
>   __regset_get+0x68/0xcc
>   regset_get_alloc+0x1c/0x28
>   elf_core_dump+0x3d8/0xd8c
>   do_coredump+0xeb8/0x1378
>   get_signal+0x14c/0x804
>   ...
>
> An order 7 allocation is (1 << 7) contiguous pages, or 512K. It's not
> a surprise that this allocation failed on a system that's been running
> for a while.
>
> More digging showed that it was fairly easy to see the order 7
> allocation by just sending a SIGQUIT to chrome (or other processes) to
> generate a core dump. The actual amount being allocated was 279,584
> bytes and it was for "core_note_type" NT_ARM_SVE.
>
> There was quite a bit of discussion [1] on the mailing lists in
> response to my v1 patch attempting to switch to vmalloc. The overall
> conclusion was that we could likely reduce the 279,584 byte allocation
> by quite a bit and Mark Brown has sent a patch to that effect [2].
> However even with the 279,584 byte allocation gone there are still
> 65,552 byte allocations. These are just barely more than the 65,536
> bytes and thus would require an order 5 allocation.
>
> An order 5 allocation is still something to avoid unless necessary and
> nothing needs the memory here to be contiguous. Change the allocation
> to kvzalloc() which should still be efficient for small allocations
> but doesn't force the memory subsystem to work hard (and maybe fail)
> at getting a large contiguous chunk.
>
> [1] https://lore.kernel.org/r/20240201171159.1.Id9ad163b60d21c9e56c2d686b0cc9083a8ba7924@changeid
> [2] https://lore.kernel.org/r/20240203-arm64-sve-ptrace-regset-size-v1-1-2c3ba1386b9e@kernel.org
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - Use kvzalloc() instead of vmalloc().
> - Update description based on v1 discussion.
>
>  fs/binfmt_elf.c | 2 +-
>  kernel/regset.c | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)

Just wanted to check in to see if there's anything else that I need to
do here. Mark's patch to avoid the order 7 allocations [1] has landed,
but we still want this kvzalloc() because the order 5 allocations
can't really be avoided. I'm happy to sit tight for longer but just
wanted to make sure it was clear that we still want my patch _in
addition_ to Mark's patch and to see if there was anything else you
needed me to do.

Thanks!

[1] https://lore.kernel.org/r/20240213-arm64-sve-ptrace-regset-size-v2-1-c7600ca74b9b@kernel.org