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
| ||
|
Date: Tue, 23 Aug 2022 15:29:01 +0800 From: Pingfan Liu <kernelfans@...il.com> To: Yury Norov <yury.norov@...il.com> Cc: linux-kernel@...r.kernel.org, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Rasmus Villemoes <linux@...musvillemoes.dk>, Thomas Gleixner <tglx@...utronix.de>, Steven Price <steven.price@....com>, Mark Rutland <mark.rutland@....com>, "Jason A. Donenfeld" <Jason@...c4.com>, Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com> Subject: Re: [RFC 07/10] lib/cpumask: Introduce cpumask_not_dying_but() On Mon, Aug 22, 2022 at 07:15:45AM -0700, Yury Norov wrote: > On Mon, Aug 22, 2022 at 10:15:17AM +0800, Pingfan Liu wrote: > > During cpu hot-removing, the dying cpus are still in cpu_online_mask. > > On the other hand, A subsystem will migrate its broker from the dying > > cpu to a online cpu in its teardown cpuhp_step. > > > > After enabling the teardown of cpus in parallel, cpu_online_mask can not > > tell those dying from the real online. > > > > Introducing a function cpumask_not_dying_but() to pick a real online > > cpu. > > > > Signed-off-by: Pingfan Liu <kernelfans@...il.com> > > Cc: Yury Norov <yury.norov@...il.com> > > Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com> > > Cc: Rasmus Villemoes <linux@...musvillemoes.dk> > > Cc: Thomas Gleixner <tglx@...utronix.de> > > Cc: Steven Price <steven.price@....com> > > Cc: Mark Rutland <mark.rutland@....com> > > Cc: "Jason A. Donenfeld" <Jason@...c4.com> > > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com> > > To: linux-kernel@...r.kernel.org > > --- > > include/linux/cpumask.h | 3 +++ > > kernel/cpu.c | 3 +++ > > lib/cpumask.c | 18 ++++++++++++++++++ > > 3 files changed, 24 insertions(+) > > > > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h > > index 0d435d0edbcb..d2033a239a07 100644 > > --- a/include/linux/cpumask.h > > +++ b/include/linux/cpumask.h > > @@ -317,6 +317,9 @@ unsigned int cpumask_any_but(const struct cpumask *mask, unsigned int cpu) > > return i; > > } > > > > +/* for parallel kexec reboot */ > > +int cpumask_not_dying_but(const struct cpumask *mask, unsigned int cpu); > > + > > #define CPU_BITS_NONE \ > > { \ > > [0 ... BITS_TO_LONGS(NR_CPUS)-1] = 0UL \ > > diff --git a/kernel/cpu.c b/kernel/cpu.c > > index 90debbe28e85..771e344f8ff9 100644 > > --- a/kernel/cpu.c > > +++ b/kernel/cpu.c > > @@ -1282,6 +1282,9 @@ static void cpus_down_no_rollback(struct cpumask *cpus) > > struct cpuhp_cpu_state *st; > > unsigned int cpu; > > > > + for_each_cpu(cpu, cpus) > > + set_cpu_dying(cpu, true); > > + > > /* launch ap work one by one, but not wait for completion */ > > for_each_cpu(cpu, cpus) { > > st = per_cpu_ptr(&cpuhp_state, cpu); > > diff --git a/lib/cpumask.c b/lib/cpumask.c > > index 8baeb37e23d3..6474f07ed87a 100644 > > --- a/lib/cpumask.c > > +++ b/lib/cpumask.c > > @@ -7,6 +7,24 @@ > > #include <linux/memblock.h> > > #include <linux/numa.h> > > > > +/* Used in parallel kexec-reboot cpuhp callbacks */ > > +int cpumask_not_dying_but(const struct cpumask *mask, > > + unsigned int cpu) > > +{ > > + unsigned int i; > > + > > + if (CONFIG_SHUTDOWN_NONBOOT_CPUS) { > > Hmm... Would it even work? Anyways, the documentation says: > Within code, where possible, use the IS_ENABLED macro to convert a Kconfig > symbol into a C boolean expression, and use it in a normal C conditional: > > .. code-block:: c > > if (IS_ENABLED(CONFIG_SOMETHING)) { > ... > } > Yes, it shall be like you pointed out. I changed the code from "#ifdef" style to "if (IS_ENABLED()" style just before sending out the series. Sorry for the haste without compiling check again. > > > + cpumask_check(cpu); > > + for_each_cpu(i, mask) > > + if (i != cpu && !cpumask_test_cpu(i, cpu_dying_mask)) > > + break; > > + return i; > > + } else { > > + return cpumask_any_but(mask, cpu); > > + } > > +} > > +EXPORT_SYMBOL(cpumask_not_dying_but); > > I don't like how you create a dedicated function for a random > mask. Dying mask is nothing special, right? What you really Yes, I agree. > need is probably this: > cpumask_andnot_any_but(mask, cpu_dying_mask, cpu); > That is it. > Now, if you still think it's worth that, you can add a trivial wrapper > for cpu_dying_mask. (But please pick some other name, because > 'not dying but' sounds like a hangover description. :) ) > I think that since even if !IS_ENABLED(CONFIG_SHUTDOWN_NONBOOT_CPUS), cpumask_andnot_any_but(mask, cpu_dying_mask, cpu) can work properly, so replacing the callsite with "cpumask_andnot() + cpumask_any_but()" will be a choice. Appreciate for your help. Thanks, Pingfan > Thanks, > Yury > > > + > > /** > > * cpumask_next_wrap - helper to implement for_each_cpu_wrap > > * @n: the cpu prior to the place to search > > -- > > 2.31.1
Powered by blists - more mailing lists