Message-ID: <75c2b85a-da67-4037-b2b5-6a1dec838fa9@ancud.ru>
Date: Wed, 10 Apr 2024 21:00:06 +0300
From: Nikita Kiryushin <kiryushin@...ud.ru>
To: Ingo Molnar <mingo@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
 Peter Zijlstra <peterz@...radead.org>, Ashok Raj <ashok.raj@...el.com>,
 David Woodhouse <dwmw@...zon.co.uk>, linux-kernel@...r.kernel.org,
 lvc-project@...uxtesting.org
Subject: Re: [PATCH] x86/smpboot: Add map vars allocation check in
 smp_prepare_cpus_common

Thank you for the feedback!
I still have a couple of questions, if you could elaborate further?

>   - That doesn't really solve anything, nor does it propagate the error
>     further up.
This patch indeed is not fixing the problem, but tries to, at least
notify about it. Not sure, if it makes any sense in the context, as
noted that

> Plus memory allocation failures within __init functions for
>     key CPU data structures are invariably fatal.
If allocation errors in this case are indeed fatal, no one will be able to
get the printed warning (so it is indeed pointless).

But I do not understand, if all the possible allocation failures here are fatal?
For cpu 0 it is obvious (as set_cpu_sibling_map(0) right here dereferences
the allocated), but is it as fatal for the other cores?

> While there might be
>     more cores in the future - but there will be even more RAM. This error
>     condition will never be realistic.
In general case it is true. But there are some cases, for which we can not presume, that there will be enough resources: for example, building a new system with large number of cores and test-running it with one small stick of ram, or (more realistically) some quirky hardware/firmware fault. I myself had experience in the past with a buggy BIOS, that made memory resource available for the system usage so small, it led to allocation failures in places it was presumed impossible before.
>   - The canonical arch behavior for __init functions is to return -ENOMEM
>     and not printk anything.
Thank you for this observation! Is there any documentation I should read
to educate myself about conventional rules like that?
I was making my assumptions that printk is an OK solution in this case,
based on surrounding code (like arch_cpuhp_init_parallel_bringup,
disable_smp and native_smp_prepare_cpus, all of which do printk).
I guess it may be specific to this specific part of code that, as you
mentioned, are not meant to fail.
>   But that's not really an option for
>     smp_prepare_cpus_common(), which feeds back into the
>     ::smp_prepare_cpus() callback that doesn't really expect failure either.

> My suggestion would be to simply pass in __GFP_NOFAIL to document that
> there's no reasonable allocation failure policy here.
Thank you, seems like a more elegant solution, than adding new state
flags to the code!
But I have some questions considering __GFP_NOFAIL. It clearly shows, that allocation will not fail/is not expected to fail, does it not try making allocation until it succeeds? Would not it make the system hang in a problematic case? If all the allocations for all the cpus in this block are critical, we would be trading a possible crash for a possible hang (all in early startup), not sure which is better to debug. If some of the allocations are not that critical, with __GFP_NOFAIL the would still hang, is it OK?
> Also note that this code has changed in the latest x86 tree (tip:master).
Thank you for bringing that to my attention! Are there any guides to read
about the preferred workflow for patches for x86 subsystem? I was developing
the code on top of mainline kernel, not the x86 tree, and would like to know
if I should have done it differently for the future.

Powered by blists - more mailing lists