The cleanup helper __free(free_cpumask_var) works only if the function
returns after alloc_cpumask_var, this can complicate the code in case
there are multiple cpumasks using the cleanup helper.
Define a cpumask initialiser that is NULL if the cpumask is a pointer
and {} if it's on stack. This allows users of the cleanup helper to use
it freely on initialised cpumasks as the actual free will be called only
if the mask is not NULL (and of course if it's a pointer).
This solution was first used in [1], dropped as eventually a single mask
was sufficient.
[1] - https://lore.kernel.org/lkml/20240120025053.684838-8-yury.norov@gmail.com
Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
---
include/linux/cpumask.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index ff8f41ab7ce6..5fb9c3fe4256 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -1005,6 +1005,7 @@ static __always_inline unsigned int cpumask_size(void)
#define this_cpu_cpumask_var_ptr(x) this_cpu_read(x)
#define __cpumask_var_read_mostly __read_mostly
+#define CPUMASK_NULL NULL
bool alloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node);
@@ -1051,6 +1052,7 @@ static __always_inline bool cpumask_available(cpumask_var_t mask)
#define this_cpu_cpumask_var_ptr(x) this_cpu_ptr(x)
#define __cpumask_var_read_mostly
+#define CPUMASK_NULL {}
static __always_inline bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags)
{
--
2.51.0On Mon, Sep 15, 2025 at 04:59:29PM +0200, Gabriele Monaco wrote:
> The cleanup helper __free(free_cpumask_var) works only if the function
> returns after alloc_cpumask_var, this can complicate the code in case
> there are multiple cpumasks using the cleanup helper.
>
> Define a cpumask initialiser that is NULL if the cpumask is a pointer
> and {} if it's on stack. This allows users of the cleanup helper to use
> it freely on initialised cpumasks as the actual free will be called only
> if the mask is not NULL (and of course if it's a pointer).
>
> This solution was first used in [1], dropped as eventually a single mask
> was sufficient.
>
> [1] - https://lore.kernel.org/lkml/20240120025053.684838-8-yury.norov@gmail.com
So why don't you pick the original patch instead?
> Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
> ---
> include/linux/cpumask.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index ff8f41ab7ce6..5fb9c3fe4256 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -1005,6 +1005,7 @@ static __always_inline unsigned int cpumask_size(void)
>
> #define this_cpu_cpumask_var_ptr(x) this_cpu_read(x)
> #define __cpumask_var_read_mostly __read_mostly
> +#define CPUMASK_NULL NULL
>
> bool alloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node);
>
> @@ -1051,6 +1052,7 @@ static __always_inline bool cpumask_available(cpumask_var_t mask)
>
> #define this_cpu_cpumask_var_ptr(x) this_cpu_ptr(x)
> #define __cpumask_var_read_mostly
> +#define CPUMASK_NULL {}
>
> static __always_inline bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags)
> {
> --
> 2.51.0
2025-09-15T16:04:53Z Yury Norov <yury.norov@gmail.com>: > So why don't you pick the original patch instead? In my opinion, the /juice/ of that patch was included with [1], here I'm just adding part of it. If you prefer I can pick that patch and adapt the commit message to reflect only the part included here. [1] - https://lore.kernel.org/lkml/1706509267-17754-3-git-send-email-schakrabarti@linux.microsoft.com/
On Mon, Sep 15, 2025 at 05:02:45PM +0000, Gabriele Monaco wrote: > 2025-09-15T16:04:53Z Yury Norov <yury.norov@gmail.com>: > > So why don't you pick the original patch instead? > > In my opinion, the /juice/ of that patch was included with [1], here I'm just adding part of it. > If you prefer I can pick that patch and adapt the commit message to reflect only the part included here. > > [1] - https://lore.kernel.org/lkml/1706509267-17754-3-git-send-email-schakrabarti@linux.microsoft.com/ Yes please.
Le Mon, Sep 15, 2025 at 02:35:12PM -0400, Yury Norov a écrit : > On Mon, Sep 15, 2025 at 05:02:45PM +0000, Gabriele Monaco wrote: > > 2025-09-15T16:04:53Z Yury Norov <yury.norov@gmail.com>: > > > So why don't you pick the original patch instead? > > > > In my opinion, the /juice/ of that patch was included with [1], here I'm just adding part of it. > > If you prefer I can pick that patch and adapt the commit message to reflect only the part included here. > > > > [1] - https://lore.kernel.org/lkml/1706509267-17754-3-git-send-email-schakrabarti@linux.microsoft.com/ > > Yes please. And can we rename CPUMASK_NULL to CPUMASK_VAR_NULL to avoid accidents/confusion with real cpumasks initializations? Thanks. -- Frederic Weisbecker SUSE Labs
On Tue, 2025-09-16 at 13:27 +0200, Frederic Weisbecker wrote:
> Le Mon, Sep 15, 2025 at 02:35:12PM -0400, Yury Norov a écrit :
> > On Mon, Sep 15, 2025 at 05:02:45PM +0000, Gabriele Monaco wrote:
> > > 2025-09-15T16:04:53Z Yury Norov <yury.norov@gmail.com>:
> > > > So why don't you pick the original patch instead?
> > >
> > > In my opinion, the /juice/ of that patch was included with [1],
> > > here I'm just adding part of it.
> > > If you prefer I can pick that patch and adapt the commit message
> > > to reflect only the part included here.
> > >
> > > [1] -
> > > https://lore.kernel.org/lkml/1706509267-17754-3-git-send-email-schakrabarti@linux.microsoft.com/
> >
> > Yes please.
Alright, will use your commit in v13 while changing the macro name to
CPUMASK_VAR_NULL as suggested.
> And can we rename CPUMASK_NULL to CPUMASK_VAR_NULL to avoid
> accidents/confusion with real
> cpumasks initializations?
Note that in the linked commit message, you have what I believe is an
incorrect assumption:
> So define a CPUMASK_VAR_NULL macro, ... and effectively a no-op
> when CPUMASK_OFFSTACK is disabled.
According to what I can understand from the standard, the C list
initialisation sets to the default value (e.g. 0) all elements not
present in the initialiser. Since in {} no element is present, {} is
not a no-op but it initialises the entire cpumask to 0.
Am I missing your original intent here?
It doesn't look like a big price to pay, but I'd still reword the
sentence to something like:
"and a valid struct initializer when CPUMASK_OFFSTACK is disabled."
Thanks,
Gabriele
On Wed, Sep 17, 2025 at 09:51:47AM +0200, Gabriele Monaco wrote:
>
>
> On Tue, 2025-09-16 at 13:27 +0200, Frederic Weisbecker wrote:
> > Le Mon, Sep 15, 2025 at 02:35:12PM -0400, Yury Norov a écrit :
> > > On Mon, Sep 15, 2025 at 05:02:45PM +0000, Gabriele Monaco wrote:
> > > > 2025-09-15T16:04:53Z Yury Norov <yury.norov@gmail.com>:
> > > > > So why don't you pick the original patch instead?
> > > >
> > > > In my opinion, the /juice/ of that patch was included with [1],
> > > > here I'm just adding part of it.
> > > > If you prefer I can pick that patch and adapt the commit message
> > > > to reflect only the part included here.
> > > >
> > > > [1] -
> > > > https://lore.kernel.org/lkml/1706509267-17754-3-git-send-email-schakrabarti@linux.microsoft.com/
> > >
> > > Yes please.
>
> Alright, will use your commit in v13 while changing the macro name to
> CPUMASK_VAR_NULL as suggested.
>
> > And can we rename CPUMASK_NULL to CPUMASK_VAR_NULL to avoid
> > accidents/confusion with real
> > cpumasks initializations?
>
> Note that in the linked commit message, you have what I believe is an
> incorrect assumption:
>
> > So define a CPUMASK_VAR_NULL macro, ... and effectively a no-op
> > when CPUMASK_OFFSTACK is disabled.
>
> According to what I can understand from the standard, the C list
> initialisation sets to the default value (e.g. 0) all elements not
> present in the initialiser. Since in {} no element is present, {} is
> not a no-op but it initialises the entire cpumask to 0.
>
> Am I missing your original intent here?
> It doesn't look like a big price to pay, but I'd still reword the
> sentence to something like:
> "and a valid struct initializer when CPUMASK_OFFSTACK is disabled."
The full quote is:
So define a CPUMASK_NULL macro, which allows to init struct cpumask
pointer with NULL when CPUMASK_OFFSTACK is enabled, and effectively
a no-op when CPUMASK_OFFSTACK is disabled.
If you read the 'which allows' part, it makes more sense, isn't?
On Wed, 2025-09-17 at 07:38 -0400, Yury Norov wrote:
> On Wed, Sep 17, 2025 at 09:51:47AM +0200, Gabriele Monaco wrote:
> > According to what I can understand from the standard, the C list
> > initialisation sets to the default value (e.g. 0) all elements not
> > present in the initialiser. Since in {} no element is present, {}
> > is not a no-op but it initialises the entire cpumask to 0.
> >
> > Am I missing your original intent here?
> > It doesn't look like a big price to pay, but I'd still reword the
> > sentence to something like:
> > "and a valid struct initializer when CPUMASK_OFFSTACK is disabled."
>
> The full quote is:
>
> So define a CPUMASK_NULL macro, which allows to init struct cpumask
> pointer with NULL when CPUMASK_OFFSTACK is enabled, and effectively
> a no-op when CPUMASK_OFFSTACK is disabled.
>
> If you read the 'which allows' part, it makes more sense, isn't?
Alright, my bad for trimming the sentence, what I wanted to highlight
is that with !CPUMASK_OFFSTACK this CPUMASK_NULL becomes something like
struct cpumask mask[1] = {};
which, to me, doesn't look like a no-op as the description suggests,
but an initialisation of the entire array.
Now I'm not sure if the compiler would be smart enough to optimise this
assignment out, but it doesn't look obvious to me.
Unless you were meaning the __free() becomes a no-op (which is true but
out of scope in this version of the patch), I would avoid mentioning
the no-op altogether.
Am I missing something and that initialisation is proven to be compiled
out?
On Wed, Sep 17, 2025 at 02:08:06PM +0200, Gabriele Monaco wrote:
> On Wed, 2025-09-17 at 07:38 -0400, Yury Norov wrote:
> > On Wed, Sep 17, 2025 at 09:51:47AM +0200, Gabriele Monaco wrote:
> > > According to what I can understand from the standard, the C list
> > > initialisation sets to the default value (e.g. 0) all elements not
> > > present in the initialiser. Since in {} no element is present, {}
> > > is not a no-op but it initialises the entire cpumask to 0.
> > >
> > > Am I missing your original intent here?
> > > It doesn't look like a big price to pay, but I'd still reword the
> > > sentence to something like:
> > > "and a valid struct initializer when CPUMASK_OFFSTACK is disabled."
> >
> > The full quote is:
> >
> > So define a CPUMASK_NULL macro, which allows to init struct cpumask
> > pointer with NULL when CPUMASK_OFFSTACK is enabled, and effectively
> > a no-op when CPUMASK_OFFSTACK is disabled.
> >
> > If you read the 'which allows' part, it makes more sense, isn't?
>
> Alright, my bad for trimming the sentence, what I wanted to highlight
> is that with !CPUMASK_OFFSTACK this CPUMASK_NULL becomes something like
>
> struct cpumask mask[1] = {};
>
> which, to me, doesn't look like a no-op as the description suggests,
> but an initialisation of the entire array.
>
> Now I'm not sure if the compiler would be smart enough to optimise this
> assignment out, but it doesn't look obvious to me.
>
> Unless you were meaning the __free() becomes a no-op (which is true but
> out of scope in this version of the patch), I would avoid mentioning
> the no-op altogether.
>
> Am I missing something and that initialisation is proven to be compiled
> out?
When you create a non-initialized variable on stack, compiler does
nothing about it, except for adjusting an argument to brk() emitted in
the function prologue.
In this case, non-initialized struct cpumask is already on stack, and
switching from
struct cpumask mask[1];
to
struct cpumask mask[1] = {};
is really a no-op.
Thanks,
Yury
On Wed, Sep 17, 2025 at 08:34:54AM -0400, Yury Norov wrote:
> On Wed, Sep 17, 2025 at 02:08:06PM +0200, Gabriele Monaco wrote:
> > On Wed, 2025-09-17 at 07:38 -0400, Yury Norov wrote:
> > > On Wed, Sep 17, 2025 at 09:51:47AM +0200, Gabriele Monaco wrote:
> > > > According to what I can understand from the standard, the C list
> > > > initialisation sets to the default value (e.g. 0) all elements not
> > > > present in the initialiser. Since in {} no element is present, {}
> > > > is not a no-op but it initialises the entire cpumask to 0.
> > > >
> > > > Am I missing your original intent here?
> > > > It doesn't look like a big price to pay, but I'd still reword the
> > > > sentence to something like:
> > > > "and a valid struct initializer when CPUMASK_OFFSTACK is disabled."
> > >
> > > The full quote is:
> > >
> > > So define a CPUMASK_NULL macro, which allows to init struct cpumask
> > > pointer with NULL when CPUMASK_OFFSTACK is enabled, and effectively
> > > a no-op when CPUMASK_OFFSTACK is disabled.
> > >
> > > If you read the 'which allows' part, it makes more sense, isn't?
> >
> > Alright, my bad for trimming the sentence, what I wanted to highlight
> > is that with !CPUMASK_OFFSTACK this CPUMASK_NULL becomes something like
> >
> > struct cpumask mask[1] = {};
> >
> > which, to me, doesn't look like a no-op as the description suggests,
> > but an initialisation of the entire array.
> >
> > Now I'm not sure if the compiler would be smart enough to optimise this
> > assignment out, but it doesn't look obvious to me.
> >
> > Unless you were meaning the __free() becomes a no-op (which is true but
> > out of scope in this version of the patch), I would avoid mentioning
> > the no-op altogether.
> >
> > Am I missing something and that initialisation is proven to be compiled
> > out?
>
> When you create a non-initialized variable on stack, compiler does
> nothing about it, except for adjusting an argument to brk() emitted in
> the function prologue.
>
> In this case, non-initialized struct cpumask is already on stack, and
> switching from
>
> struct cpumask mask[1];
>
> to
>
> struct cpumask mask[1] = {};
>
> is really a no-op.
Alright... The above is correct for optimization levels > 0.
With -O0, 2nd version really makes GCC to initialize the array.
https://godbolt.org/z/e1zG4K7M8
This is not relevant for the kernel because -O2 is our default
optimization level (I'm not even sure that -O0 is buildable).
But you may want to mention that in commit message.
Thanks,
Yury
2025-09-19T14:58:18Z Yury Norov <yury.norov@gmail.com>:
>> When you create a non-initialized variable on stack, compiler does
>> nothing about it, except for adjusting an argument to brk() emitted in
>> the function prologue.
>>
>> In this case, non-initialized struct cpumask is already on stack, and
>> switching from
>>
>> struct cpumask mask[1];
>>
>> to
>>
>> struct cpumask mask[1] = {};
>>
>> is really a no-op.
>
> Alright... The above is correct for optimization levels > 0.
> With -O0, 2nd version really makes GCC to initialize the array.
>
> https://godbolt.org/z/e1zG4K7M8
>
> This is not relevant for the kernel because -O2 is our default
> optimization level (I'm not even sure that -O0 is buildable).
> But you may want to mention that in commit message.
Thanks for testing that!
Makes totally sense. Although if it's something really unreachable in the kernel it may not make too much sense to mention in the commit message.
Also I'm not planning on sending another version after V13 [1], so we may let it slide.
Thanks,
Gabriele
[1] - https://lore.kernel.org/lkml/20250917161958.178925-1-gmonaco@redhat.com
© 2016 - 2025 Red Hat, Inc.