diff mbox series

[2/2] alloc_tag: outline and export {get|put}_page_tag_ref() used by modules

Message ID 20240717011631.2150066-2-surenb@google.com (mailing list archive)
State New
Headers show
Series [1/2] alloc_tag: export mem_alloc_profiling_key used by modules | expand

Commit Message

Suren Baghdasaryan July 17, 2024, 1:16 a.m. UTC
Outline and export get_page_tag_ref() and put_page_tag_ref() so that
modules can use them without exporting page_ext_get() and page_ext_put().

Fixes: dcfe378c81f7 ("lib: introduce support for page allocation tagging")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202407080044.DWMC9N9I-lkp@intel.com/
Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/pgalloc_tag.h | 23 +++--------------------
 lib/alloc_tag.c             | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+), 20 deletions(-)

Comments

Christoph Hellwig July 17, 2024, 4:47 a.m. UTC | #1
Thanks, this looks much better:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Vlastimil Babka July 17, 2024, 9:30 a.m. UTC | #2
On 7/17/24 3:16 AM, Suren Baghdasaryan wrote:
> Outline and export get_page_tag_ref() and put_page_tag_ref() so that
> modules can use them without exporting page_ext_get() and page_ext_put().
> 
> Fixes: dcfe378c81f7 ("lib: introduce support for page allocation tagging")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202407080044.DWMC9N9I-lkp@intel.com/
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Better than exporting page_ext but still seems like suboptimal level to me.
You have various inline functions that use get/put_page_tag_ref and now will
call into them both. IMHO outlining those (and leaving only the static key
check inline) is the better way.

As for the report that triggered this, AFAICS it was due to
free_reserved_page() so it could be enough to only outline that memalloc
profiling part there and leave the rest as that seems to be used only from
core code and no modules.

> ---
>  include/linux/pgalloc_tag.h | 23 +++--------------------
>  lib/alloc_tag.c             | 23 +++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h
> index 9cacadbd61f8..3c6ab717bd57 100644
> --- a/include/linux/pgalloc_tag.h
> +++ b/include/linux/pgalloc_tag.h
> @@ -13,6 +13,9 @@
>  
>  extern struct page_ext_operations page_alloc_tagging_ops;
>  
> +union codetag_ref *get_page_tag_ref(struct page *page);
> +void put_page_tag_ref(union codetag_ref *ref);
> +
>  static inline union codetag_ref *codetag_ref_from_page_ext(struct page_ext *page_ext)
>  {
>  	return (void *)page_ext + page_alloc_tagging_ops.offset;
> @@ -23,26 +26,6 @@ static inline struct page_ext *page_ext_from_codetag_ref(union codetag_ref *ref)
>  	return (void *)ref - page_alloc_tagging_ops.offset;
>  }
>  
> -/* Should be called only if mem_alloc_profiling_enabled() */
> -static inline union codetag_ref *get_page_tag_ref(struct page *page)
> -{
> -	if (page) {
> -		struct page_ext *page_ext = page_ext_get(page);
> -
> -		if (page_ext)
> -			return codetag_ref_from_page_ext(page_ext);
> -	}
> -	return NULL;
> -}
> -
> -static inline void put_page_tag_ref(union codetag_ref *ref)
> -{
> -	if (WARN_ON(!ref))
> -		return;
> -
> -	page_ext_put(page_ext_from_codetag_ref(ref));
> -}
> -
>  static inline void pgalloc_tag_add(struct page *page, struct task_struct *task,
>  				   unsigned int nr)
>  {
> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> index 832f79a32b3e..5271cc070901 100644
> --- a/lib/alloc_tag.c
> +++ b/lib/alloc_tag.c
> @@ -4,6 +4,7 @@
>  #include <linux/gfp.h>
>  #include <linux/module.h>
>  #include <linux/page_ext.h>
> +#include <linux/pgalloc_tag.h>
>  #include <linux/proc_fs.h>
>  #include <linux/seq_buf.h>
>  #include <linux/seq_file.h>
> @@ -22,6 +23,28 @@ struct allocinfo_private {
>  	bool print_header;
>  };
>  
> +/* Should be called only if mem_alloc_profiling_enabled() */
> +union codetag_ref *get_page_tag_ref(struct page *page)
> +{
> +	if (page) {
> +		struct page_ext *page_ext = page_ext_get(page);
> +
> +		if (page_ext)
> +			return codetag_ref_from_page_ext(page_ext);
> +	}
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(get_page_tag_ref);
> +
> +void put_page_tag_ref(union codetag_ref *ref)
> +{
> +	if (WARN_ON(!ref))
> +		return;
> +
> +	page_ext_put(page_ext_from_codetag_ref(ref));
> +}
> +EXPORT_SYMBOL_GPL(put_page_tag_ref);
> +
>  static void *allocinfo_start(struct seq_file *m, loff_t *pos)
>  {
>  	struct allocinfo_private *priv;
Suren Baghdasaryan July 17, 2024, 4:56 p.m. UTC | #3
On Wed, Jul 17, 2024 at 2:30 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 7/17/24 3:16 AM, Suren Baghdasaryan wrote:
> > Outline and export get_page_tag_ref() and put_page_tag_ref() so that
> > modules can use them without exporting page_ext_get() and page_ext_put().
> >
> > Fixes: dcfe378c81f7 ("lib: introduce support for page allocation tagging")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202407080044.DWMC9N9I-lkp@intel.com/
> > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>
> Better than exporting page_ext but still seems like suboptimal level to me.
> You have various inline functions that use get/put_page_tag_ref and now will
> call into them both. IMHO outlining those (and leaving only the static key
> check inline) is the better way.
>
> As for the report that triggered this, AFAICS it was due to
> free_reserved_page() so it could be enough to only outline that memalloc
> profiling part there and leave the rest as that seems to be used only from
> core code and no modules.

Doh! I didn't realize that's the only place these functions are used
directly but looks like you are right. Outlining free_reserved_page()
is definitely a much better solution with less performance impact.
I'll post a replacement patch shortly.
Thanks Vlastimil!

>
> > ---
> >  include/linux/pgalloc_tag.h | 23 +++--------------------
> >  lib/alloc_tag.c             | 23 +++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h
> > index 9cacadbd61f8..3c6ab717bd57 100644
> > --- a/include/linux/pgalloc_tag.h
> > +++ b/include/linux/pgalloc_tag.h
> > @@ -13,6 +13,9 @@
> >
> >  extern struct page_ext_operations page_alloc_tagging_ops;
> >
> > +union codetag_ref *get_page_tag_ref(struct page *page);
> > +void put_page_tag_ref(union codetag_ref *ref);
> > +
> >  static inline union codetag_ref *codetag_ref_from_page_ext(struct page_ext *page_ext)
> >  {
> >       return (void *)page_ext + page_alloc_tagging_ops.offset;
> > @@ -23,26 +26,6 @@ static inline struct page_ext *page_ext_from_codetag_ref(union codetag_ref *ref)
> >       return (void *)ref - page_alloc_tagging_ops.offset;
> >  }
> >
> > -/* Should be called only if mem_alloc_profiling_enabled() */
> > -static inline union codetag_ref *get_page_tag_ref(struct page *page)
> > -{
> > -     if (page) {
> > -             struct page_ext *page_ext = page_ext_get(page);
> > -
> > -             if (page_ext)
> > -                     return codetag_ref_from_page_ext(page_ext);
> > -     }
> > -     return NULL;
> > -}
> > -
> > -static inline void put_page_tag_ref(union codetag_ref *ref)
> > -{
> > -     if (WARN_ON(!ref))
> > -             return;
> > -
> > -     page_ext_put(page_ext_from_codetag_ref(ref));
> > -}
> > -
> >  static inline void pgalloc_tag_add(struct page *page, struct task_struct *task,
> >                                  unsigned int nr)
> >  {
> > diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> > index 832f79a32b3e..5271cc070901 100644
> > --- a/lib/alloc_tag.c
> > +++ b/lib/alloc_tag.c
> > @@ -4,6 +4,7 @@
> >  #include <linux/gfp.h>
> >  #include <linux/module.h>
> >  #include <linux/page_ext.h>
> > +#include <linux/pgalloc_tag.h>
> >  #include <linux/proc_fs.h>
> >  #include <linux/seq_buf.h>
> >  #include <linux/seq_file.h>
> > @@ -22,6 +23,28 @@ struct allocinfo_private {
> >       bool print_header;
> >  };
> >
> > +/* Should be called only if mem_alloc_profiling_enabled() */
> > +union codetag_ref *get_page_tag_ref(struct page *page)
> > +{
> > +     if (page) {
> > +             struct page_ext *page_ext = page_ext_get(page);
> > +
> > +             if (page_ext)
> > +                     return codetag_ref_from_page_ext(page_ext);
> > +     }
> > +     return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(get_page_tag_ref);
> > +
> > +void put_page_tag_ref(union codetag_ref *ref)
> > +{
> > +     if (WARN_ON(!ref))
> > +             return;
> > +
> > +     page_ext_put(page_ext_from_codetag_ref(ref));
> > +}
> > +EXPORT_SYMBOL_GPL(put_page_tag_ref);
> > +
> >  static void *allocinfo_start(struct seq_file *m, loff_t *pos)
> >  {
> >       struct allocinfo_private *priv;
>
Suren Baghdasaryan July 17, 2024, 6:24 p.m. UTC | #4
On Wed, Jul 17, 2024 at 9:56 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Jul 17, 2024 at 2:30 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> >
> > On 7/17/24 3:16 AM, Suren Baghdasaryan wrote:
> > > Outline and export get_page_tag_ref() and put_page_tag_ref() so that
> > > modules can use them without exporting page_ext_get() and page_ext_put().
> > >
> > > Fixes: dcfe378c81f7 ("lib: introduce support for page allocation tagging")
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202407080044.DWMC9N9I-lkp@intel.com/
> > > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >
> > Better than exporting page_ext but still seems like suboptimal level to me.
> > You have various inline functions that use get/put_page_tag_ref and now will
> > call into them both. IMHO outlining those (and leaving only the static key
> > check inline) is the better way.
> >
> > As for the report that triggered this, AFAICS it was due to
> > free_reserved_page() so it could be enough to only outline that memalloc
> > profiling part there and leave the rest as that seems to be used only from
> > core code and no modules.
>
> Doh! I didn't realize that's the only place these functions are used
> directly but looks like you are right. Outlining free_reserved_page()
> is definitely a much better solution with less performance impact.
> I'll post a replacement patch shortly.
> Thanks Vlastimil!

v2 posted at [1]

As a reminder for Andrew, the patchset [1] should replace the older
patch [2] currently present in mm-unstable and mm-hotfixes-unstable.

[1] https://lore.kernel.org/all/20240717181239.2510054-2-surenb@google.com/
[2] ac5ca7954e4e ("alloc_tag: export memory allocation profiling
symbols used by modules")

Thanks,
Suren.

>
> >
> > > ---
> > >  include/linux/pgalloc_tag.h | 23 +++--------------------
> > >  lib/alloc_tag.c             | 23 +++++++++++++++++++++++
> > >  2 files changed, 26 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h
> > > index 9cacadbd61f8..3c6ab717bd57 100644
> > > --- a/include/linux/pgalloc_tag.h
> > > +++ b/include/linux/pgalloc_tag.h
> > > @@ -13,6 +13,9 @@
> > >
> > >  extern struct page_ext_operations page_alloc_tagging_ops;
> > >
> > > +union codetag_ref *get_page_tag_ref(struct page *page);
> > > +void put_page_tag_ref(union codetag_ref *ref);
> > > +
> > >  static inline union codetag_ref *codetag_ref_from_page_ext(struct page_ext *page_ext)
> > >  {
> > >       return (void *)page_ext + page_alloc_tagging_ops.offset;
> > > @@ -23,26 +26,6 @@ static inline struct page_ext *page_ext_from_codetag_ref(union codetag_ref *ref)
> > >       return (void *)ref - page_alloc_tagging_ops.offset;
> > >  }
> > >
> > > -/* Should be called only if mem_alloc_profiling_enabled() */
> > > -static inline union codetag_ref *get_page_tag_ref(struct page *page)
> > > -{
> > > -     if (page) {
> > > -             struct page_ext *page_ext = page_ext_get(page);
> > > -
> > > -             if (page_ext)
> > > -                     return codetag_ref_from_page_ext(page_ext);
> > > -     }
> > > -     return NULL;
> > > -}
> > > -
> > > -static inline void put_page_tag_ref(union codetag_ref *ref)
> > > -{
> > > -     if (WARN_ON(!ref))
> > > -             return;
> > > -
> > > -     page_ext_put(page_ext_from_codetag_ref(ref));
> > > -}
> > > -
> > >  static inline void pgalloc_tag_add(struct page *page, struct task_struct *task,
> > >                                  unsigned int nr)
> > >  {
> > > diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> > > index 832f79a32b3e..5271cc070901 100644
> > > --- a/lib/alloc_tag.c
> > > +++ b/lib/alloc_tag.c
> > > @@ -4,6 +4,7 @@
> > >  #include <linux/gfp.h>
> > >  #include <linux/module.h>
> > >  #include <linux/page_ext.h>
> > > +#include <linux/pgalloc_tag.h>
> > >  #include <linux/proc_fs.h>
> > >  #include <linux/seq_buf.h>
> > >  #include <linux/seq_file.h>
> > > @@ -22,6 +23,28 @@ struct allocinfo_private {
> > >       bool print_header;
> > >  };
> > >
> > > +/* Should be called only if mem_alloc_profiling_enabled() */
> > > +union codetag_ref *get_page_tag_ref(struct page *page)
> > > +{
> > > +     if (page) {
> > > +             struct page_ext *page_ext = page_ext_get(page);
> > > +
> > > +             if (page_ext)
> > > +                     return codetag_ref_from_page_ext(page_ext);
> > > +     }
> > > +     return NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(get_page_tag_ref);
> > > +
> > > +void put_page_tag_ref(union codetag_ref *ref)
> > > +{
> > > +     if (WARN_ON(!ref))
> > > +             return;
> > > +
> > > +     page_ext_put(page_ext_from_codetag_ref(ref));
> > > +}
> > > +EXPORT_SYMBOL_GPL(put_page_tag_ref);
> > > +
> > >  static void *allocinfo_start(struct seq_file *m, loff_t *pos)
> > >  {
> > >       struct allocinfo_private *priv;
> >
diff mbox series

Patch

diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h
index 9cacadbd61f8..3c6ab717bd57 100644
--- a/include/linux/pgalloc_tag.h
+++ b/include/linux/pgalloc_tag.h
@@ -13,6 +13,9 @@ 
 
 extern struct page_ext_operations page_alloc_tagging_ops;
 
+union codetag_ref *get_page_tag_ref(struct page *page);
+void put_page_tag_ref(union codetag_ref *ref);
+
 static inline union codetag_ref *codetag_ref_from_page_ext(struct page_ext *page_ext)
 {
 	return (void *)page_ext + page_alloc_tagging_ops.offset;
@@ -23,26 +26,6 @@  static inline struct page_ext *page_ext_from_codetag_ref(union codetag_ref *ref)
 	return (void *)ref - page_alloc_tagging_ops.offset;
 }
 
-/* Should be called only if mem_alloc_profiling_enabled() */
-static inline union codetag_ref *get_page_tag_ref(struct page *page)
-{
-	if (page) {
-		struct page_ext *page_ext = page_ext_get(page);
-
-		if (page_ext)
-			return codetag_ref_from_page_ext(page_ext);
-	}
-	return NULL;
-}
-
-static inline void put_page_tag_ref(union codetag_ref *ref)
-{
-	if (WARN_ON(!ref))
-		return;
-
-	page_ext_put(page_ext_from_codetag_ref(ref));
-}
-
 static inline void pgalloc_tag_add(struct page *page, struct task_struct *task,
 				   unsigned int nr)
 {
diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
index 832f79a32b3e..5271cc070901 100644
--- a/lib/alloc_tag.c
+++ b/lib/alloc_tag.c
@@ -4,6 +4,7 @@ 
 #include <linux/gfp.h>
 #include <linux/module.h>
 #include <linux/page_ext.h>
+#include <linux/pgalloc_tag.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_buf.h>
 #include <linux/seq_file.h>
@@ -22,6 +23,28 @@  struct allocinfo_private {
 	bool print_header;
 };
 
+/* Should be called only if mem_alloc_profiling_enabled() */
+union codetag_ref *get_page_tag_ref(struct page *page)
+{
+	if (page) {
+		struct page_ext *page_ext = page_ext_get(page);
+
+		if (page_ext)
+			return codetag_ref_from_page_ext(page_ext);
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(get_page_tag_ref);
+
+void put_page_tag_ref(union codetag_ref *ref)
+{
+	if (WARN_ON(!ref))
+		return;
+
+	page_ext_put(page_ext_from_codetag_ref(ref));
+}
+EXPORT_SYMBOL_GPL(put_page_tag_ref);
+
 static void *allocinfo_start(struct seq_file *m, loff_t *pos)
 {
 	struct allocinfo_private *priv;