2024-04-07 13:43:25

by Klara Modin

[permalink] [raw]
Subject: [PATCH] mm/memprofiling: explicitly include irqflags.h in alloc_tag.h

linux/alloc_tag.h uses the macro this_cpu_inc which eventually expands to:

#define this_cpu_generic_to_op(pcp, val, op) \
do { \
unsigned long __flags; \
raw_local_irq_save(__flags); \
raw_cpu_generic_to_op(pcp, val, op); \
raw_local_irq_restore(__flags); \
} while (0)

The macros raw_local_irq_save and raw_local_irq_restore are defined in
linux/irqflags.h which is not included implicitly on all configs.
Therefore, include it explicitly.

Fixes: ac906a377c67 ("lib: add allocation tagging support for memory allocation profiling")
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Klara Modin <[email protected]>
---
include/linux/alloc_tag.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h
index e867461585ff..afc9e259a2d3 100644
--- a/include/linux/alloc_tag.h
+++ b/include/linux/alloc_tag.h
@@ -12,6 +12,7 @@
#include <asm/percpu.h>
#include <linux/cpumask.h>
#include <linux/static_key.h>
+#include <linux/irqflags.h>

struct alloc_tag_counters {
u64 bytes;

base-commit: f43b3aae94511d62174c3b29239da0dd22d0eeb3
--
2.44.0



2024-04-07 17:01:54

by Kent Overstreet

[permalink] [raw]
Subject: Re: [PATCH] mm/memprofiling: explicitly include irqflags.h in alloc_tag.h

On Sun, Apr 07, 2024 at 03:32:52PM +0200, Klara Modin wrote:
> linux/alloc_tag.h uses the macro this_cpu_inc which eventually expands to:
>
> #define this_cpu_generic_to_op(pcp, val, op) \
> do { \
> unsigned long __flags; \
> raw_local_irq_save(__flags); \
> raw_cpu_generic_to_op(pcp, val, op); \
> raw_local_irq_restore(__flags); \
> } while (0)
>
> The macros raw_local_irq_save and raw_local_irq_restore are defined in
> linux/irqflags.h which is not included implicitly on all configs.
> Therefore, include it explicitly.
>
> Fixes: ac906a377c67 ("lib: add allocation tagging support for memory allocation profiling")
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Klara Modin <[email protected]>
> ---
> include/linux/alloc_tag.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h
> index e867461585ff..afc9e259a2d3 100644
> --- a/include/linux/alloc_tag.h
> +++ b/include/linux/alloc_tag.h
> @@ -12,6 +12,7 @@
> #include <asm/percpu.h>
> #include <linux/cpumask.h>
> #include <linux/static_key.h>
> +#include <linux/irqflags.h>

Actually, shouldn't this end up in a percpu header? Or was there a
problem with that?

2024-04-07 17:13:57

by Klara Modin

[permalink] [raw]
Subject: Re: [PATCH] mm/memprofiling: explicitly include irqflags.h in alloc_tag.h



On 2024-04-07 19:01, Kent Overstreet wrote:
> On Sun, Apr 07, 2024 at 03:32:52PM +0200, Klara Modin wrote:
>> linux/alloc_tag.h uses the macro this_cpu_inc which eventually expands to:
>>
>> #define this_cpu_generic_to_op(pcp, val, op) \
>> do { \
>> unsigned long __flags; \
>> raw_local_irq_save(__flags); \
>> raw_cpu_generic_to_op(pcp, val, op); \
>> raw_local_irq_restore(__flags); \
>> } while (0)
>>
>> The macros raw_local_irq_save and raw_local_irq_restore are defined in
>> linux/irqflags.h which is not included implicitly on all configs.
>> Therefore, include it explicitly.
>>
>> Fixes: ac906a377c67 ("lib: add allocation tagging support for memory allocation profiling")
>> Link: https://lore.kernel.org/lkml/[email protected]/
>> Signed-off-by: Klara Modin <[email protected]>
>> ---
>> include/linux/alloc_tag.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h
>> index e867461585ff..afc9e259a2d3 100644
>> --- a/include/linux/alloc_tag.h
>> +++ b/include/linux/alloc_tag.h
>> @@ -12,6 +12,7 @@
>> #include <asm/percpu.h>
>> #include <linux/cpumask.h>
>> #include <linux/static_key.h>
>> +#include <linux/irqflags.h>
>
> Actually, shouldn't this end up in a percpu header? Or was there a
> problem with that?

If I understand it correctly, linux/alloc_tag.h does not include
linux/percpu.h (which has that include) to avoid a circular dependency
as linux/percpu.h includes linux/alloc_tag.h. It instead includes
arch-specific asm/percpu.h, and as a consequence it doesn't always get
linux/irqflags.h.

It's also entirely possible that I've mixed something up, I really don't
have much experience developing for the kernel.

2024-04-07 17:19:09

by Kent Overstreet

[permalink] [raw]
Subject: Re: [PATCH] mm/memprofiling: explicitly include irqflags.h in alloc_tag.h

On Sun, Apr 07, 2024 at 07:12:04PM +0200, Klara Modin wrote:
>
>
> On 2024-04-07 19:01, Kent Overstreet wrote:
> > On Sun, Apr 07, 2024 at 03:32:52PM +0200, Klara Modin wrote:
> > > linux/alloc_tag.h uses the macro this_cpu_inc which eventually expands to:
> > >
> > > #define this_cpu_generic_to_op(pcp, val, op) \
> > > do { \
> > > unsigned long __flags; \
> > > raw_local_irq_save(__flags); \
> > > raw_cpu_generic_to_op(pcp, val, op); \
> > > raw_local_irq_restore(__flags); \
> > > } while (0)
> > >
> > > The macros raw_local_irq_save and raw_local_irq_restore are defined in
> > > linux/irqflags.h which is not included implicitly on all configs.
> > > Therefore, include it explicitly.
> > >
> > > Fixes: ac906a377c67 ("lib: add allocation tagging support for memory allocation profiling")
> > > Link: https://lore.kernel.org/lkml/[email protected]/
> > > Signed-off-by: Klara Modin <[email protected]>
> > > ---
> > > include/linux/alloc_tag.h | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h
> > > index e867461585ff..afc9e259a2d3 100644
> > > --- a/include/linux/alloc_tag.h
> > > +++ b/include/linux/alloc_tag.h
> > > @@ -12,6 +12,7 @@
> > > #include <asm/percpu.h>
> > > #include <linux/cpumask.h>
> > > #include <linux/static_key.h>
> > > +#include <linux/irqflags.h>
> >
> > Actually, shouldn't this end up in a percpu header? Or was there a
> > problem with that?
>
> If I understand it correctly, linux/alloc_tag.h does not include
> linux/percpu.h (which has that include) to avoid a circular dependency as
> linux/percpu.h includes linux/alloc_tag.h. It instead includes arch-specific
> asm/percpu.h, and as a consequence it doesn't always get linux/irqflags.h.
>
> It's also entirely possible that I've mixed something up, I really don't
> have much experience developing for the kernel.

Gotcha

Acked-by: Kent Overstreet <[email protected]>