| Message ID | 20240717181239.2510054-2-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] alloc_tag: export mem_alloc_profiling_key used by modules | expand |
On 7/17/24 8:12 PM, Suren Baghdasaryan wrote: > Outline and export free_reserved_page() because modules use it and it > in turn uses page_ext_{get|put} which should not be exported. The same > result could be obtained by outlining {get|put}_page_tag_ref() but that > would have higher performance impact as these functions are used in > more performance critical paths. > > 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> > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> Are these two patches now stable@ material as 6.10 build is broken on ppc64 with alloc taging enabled? > --- > Changes since v1 [1] > - Outlined and exported free_reserved_page() in place of {get|put}_page_tag_ref, > per Vlastimil Babka > > [1] https://lore.kernel.org/all/20240717011631.2150066-2-surenb@google.com/ > > include/linux/mm.h | 16 +--------------- > mm/page_alloc.c | 17 +++++++++++++++++ > 2 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index eb7c96d24ac0..b58bad248eef 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3177,21 +3177,7 @@ extern void reserve_bootmem_region(phys_addr_t start, > phys_addr_t end, int nid); > > /* Free the reserved page into the buddy system, so it gets managed. */ > -static inline void free_reserved_page(struct page *page) > -{ > - if (mem_alloc_profiling_enabled()) { > - union codetag_ref *ref = get_page_tag_ref(page); > - > - if (ref) { > - set_codetag_empty(ref); > - put_page_tag_ref(ref); > - } > - } > - ClearPageReserved(page); > - init_page_count(page); > - __free_page(page); > - adjust_managed_page_count(page, 1); > -} > +void free_reserved_page(struct page *page); > #define free_highmem_page(page) free_reserved_page(page) > > static inline void mark_page_reserved(struct page *page) > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 9ecf99190ea2..7d2fa9f5e750 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -5805,6 +5805,23 @@ unsigned long free_reserved_area(void *start, void *end, int poison, const char > return pages; > } > > +void free_reserved_page(struct page *page) > +{ > + if (mem_alloc_profiling_enabled()) { > + union codetag_ref *ref = get_page_tag_ref(page); > + > + if (ref) { > + set_codetag_empty(ref); > + put_page_tag_ref(ref); > + } > + } > + ClearPageReserved(page); > + init_page_count(page); > + __free_page(page); > + adjust_managed_page_count(page, 1); > +} > +EXPORT_SYMBOL(free_reserved_page); > + > static int page_alloc_cpu_dead(unsigned int cpu) > { > struct zone *zone;
On Wed, Jul 17, 2024 at 12:36 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 7/17/24 8:12 PM, Suren Baghdasaryan wrote: > > Outline and export free_reserved_page() because modules use it and it > > in turn uses page_ext_{get|put} which should not be exported. The same > > result could be obtained by outlining {get|put}_page_tag_ref() but that > > would have higher performance impact as these functions are used in > > more performance critical paths. > > > > 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> > > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Acked-by: Vlastimil Babka <vbabka@suse.cz> > > Are these two patches now stable@ material as 6.10 build is broken on ppc64 > with alloc taging enabled? I tested them with that specific configuration mentioned in the bug report and with several usual ones I use. Yeah, I guess from now on all such fixes should have Cc: stable@vger.kernel.org # v6.10 > > > --- > > Changes since v1 [1] > > - Outlined and exported free_reserved_page() in place of {get|put}_page_tag_ref, > > per Vlastimil Babka > > > > [1] https://lore.kernel.org/all/20240717011631.2150066-2-surenb@google.com/ > > > > include/linux/mm.h | 16 +--------------- > > mm/page_alloc.c | 17 +++++++++++++++++ > > 2 files changed, 18 insertions(+), 15 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index eb7c96d24ac0..b58bad248eef 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -3177,21 +3177,7 @@ extern void reserve_bootmem_region(phys_addr_t start, > > phys_addr_t end, int nid); > > > > /* Free the reserved page into the buddy system, so it gets managed. */ > > -static inline void free_reserved_page(struct page *page) > > -{ > > - if (mem_alloc_profiling_enabled()) { > > - union codetag_ref *ref = get_page_tag_ref(page); > > - > > - if (ref) { > > - set_codetag_empty(ref); > > - put_page_tag_ref(ref); > > - } > > - } > > - ClearPageReserved(page); > > - init_page_count(page); > > - __free_page(page); > > - adjust_managed_page_count(page, 1); > > -} > > +void free_reserved_page(struct page *page); > > #define free_highmem_page(page) free_reserved_page(page) > > > > static inline void mark_page_reserved(struct page *page) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 9ecf99190ea2..7d2fa9f5e750 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -5805,6 +5805,23 @@ unsigned long free_reserved_area(void *start, void *end, int poison, const char > > return pages; > > } > > > > +void free_reserved_page(struct page *page) > > +{ > > + if (mem_alloc_profiling_enabled()) { > > + union codetag_ref *ref = get_page_tag_ref(page); > > + > > + if (ref) { > > + set_codetag_empty(ref); > > + put_page_tag_ref(ref); > > + } > > + } > > + ClearPageReserved(page); > > + init_page_count(page); > > + __free_page(page); > > + adjust_managed_page_count(page, 1); > > +} > > +EXPORT_SYMBOL(free_reserved_page); > > + > > static int page_alloc_cpu_dead(unsigned int cpu) > > { > > struct zone *zone; >
On 7/17/24 10:04 PM, Suren Baghdasaryan wrote: > On Wed, Jul 17, 2024 at 12:36 PM Vlastimil Babka <vbabka@suse.cz> wrote: >> >> On 7/17/24 8:12 PM, Suren Baghdasaryan wrote: >> > Outline and export free_reserved_page() because modules use it and it >> > in turn uses page_ext_{get|put} which should not be exported. The same >> > result could be obtained by outlining {get|put}_page_tag_ref() but that >> > would have higher performance impact as these functions are used in >> > more performance critical paths. >> > >> > 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> >> > Suggested-by: Vlastimil Babka <vbabka@suse.cz> >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> >> >> Acked-by: Vlastimil Babka <vbabka@suse.cz> >> >> Are these two patches now stable@ material as 6.10 build is broken on ppc64 >> with alloc taging enabled? > > I tested them with that specific configuration mentioned in the bug > report and with several usual ones I use. > Yeah, I guess from now on all such fixes should have > > Cc: stable@vger.kernel.org # v6.10 Right. BTW I have just realized that the way you did Patch 2/2 and outlined the whole free_reserved_page() (which is fair, it's an init-time function), mem_alloc_profiling_enabled() didn't stay inlined so Patch 1/2 is in fact not necessary anymore?
On Wed, Jul 17, 2024 at 1:19 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 7/17/24 10:04 PM, Suren Baghdasaryan wrote: > > On Wed, Jul 17, 2024 at 12:36 PM Vlastimil Babka <vbabka@suse.cz> wrote: > >> > >> On 7/17/24 8:12 PM, Suren Baghdasaryan wrote: > >> > Outline and export free_reserved_page() because modules use it and it > >> > in turn uses page_ext_{get|put} which should not be exported. The same > >> > result could be obtained by outlining {get|put}_page_tag_ref() but that > >> > would have higher performance impact as these functions are used in > >> > more performance critical paths. > >> > > >> > 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> > >> > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >> > >> Acked-by: Vlastimil Babka <vbabka@suse.cz> > >> > >> Are these two patches now stable@ material as 6.10 build is broken on ppc64 > >> with alloc taging enabled? > > > > I tested them with that specific configuration mentioned in the bug > > report and with several usual ones I use. > > Yeah, I guess from now on all such fixes should have > > > > Cc: stable@vger.kernel.org # v6.10 > > Right. BTW I have just realized that the way you did Patch 2/2 and outlined > the whole free_reserved_page() (which is fair, it's an init-time function), > mem_alloc_profiling_enabled() didn't stay inlined so Patch 1/2 is in fact > not necessary anymore? Yeah, I think you are right, currently no module has reasons to use mem_alloc_profiling_enabled() directly. That might change in the future but we can add the export at the time it's needed. I checked and ppc64 build passes with just this patch. Let me post v3 with just this patch and Cc stable@. > > >
On Wed, Jul 17, 2024 at 2:20 PM Suren Baghdasaryan <surenb@google.com> wrote: > > On Wed, Jul 17, 2024 at 1:19 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > > > On 7/17/24 10:04 PM, Suren Baghdasaryan wrote: > > > On Wed, Jul 17, 2024 at 12:36 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > >> > > >> On 7/17/24 8:12 PM, Suren Baghdasaryan wrote: > > >> > Outline and export free_reserved_page() because modules use it and it > > >> > in turn uses page_ext_{get|put} which should not be exported. The same > > >> > result could be obtained by outlining {get|put}_page_tag_ref() but that > > >> > would have higher performance impact as these functions are used in > > >> > more performance critical paths. > > >> > > > >> > 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> > > >> > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > > >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > >> > > >> Acked-by: Vlastimil Babka <vbabka@suse.cz> > > >> > > >> Are these two patches now stable@ material as 6.10 build is broken on ppc64 > > >> with alloc taging enabled? > > > > > > I tested them with that specific configuration mentioned in the bug > > > report and with several usual ones I use. > > > Yeah, I guess from now on all such fixes should have > > > > > > Cc: stable@vger.kernel.org # v6.10 > > > > Right. BTW I have just realized that the way you did Patch 2/2 and outlined > > the whole free_reserved_page() (which is fair, it's an init-time function), > > mem_alloc_profiling_enabled() didn't stay inlined so Patch 1/2 is in fact > > not necessary anymore? > > Yeah, I think you are right, currently no module has reasons to use > mem_alloc_profiling_enabled() directly. That might change in the > future but we can add the export at the time it's needed. > I checked and ppc64 build passes with just this patch. Let me post v3 > with just this patch and Cc stable@. v3 posted at https://lore.kernel.org/all/20240717212844.2749975-1-surenb@google.com/ > > > > > > > >
diff --git a/include/linux/mm.h b/include/linux/mm.h index eb7c96d24ac0..b58bad248eef 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3177,21 +3177,7 @@ extern void reserve_bootmem_region(phys_addr_t start, phys_addr_t end, int nid); /* Free the reserved page into the buddy system, so it gets managed. */ -static inline void free_reserved_page(struct page *page) -{ - if (mem_alloc_profiling_enabled()) { - union codetag_ref *ref = get_page_tag_ref(page); - - if (ref) { - set_codetag_empty(ref); - put_page_tag_ref(ref); - } - } - ClearPageReserved(page); - init_page_count(page); - __free_page(page); - adjust_managed_page_count(page, 1); -} +void free_reserved_page(struct page *page); #define free_highmem_page(page) free_reserved_page(page) static inline void mark_page_reserved(struct page *page) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 9ecf99190ea2..7d2fa9f5e750 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -5805,6 +5805,23 @@ unsigned long free_reserved_area(void *start, void *end, int poison, const char return pages; } +void free_reserved_page(struct page *page) +{ + if (mem_alloc_profiling_enabled()) { + union codetag_ref *ref = get_page_tag_ref(page); + + if (ref) { + set_codetag_empty(ref); + put_page_tag_ref(ref); + } + } + ClearPageReserved(page); + init_page_count(page); + __free_page(page); + adjust_managed_page_count(page, 1); +} +EXPORT_SYMBOL(free_reserved_page); + static int page_alloc_cpu_dead(unsigned int cpu) { struct zone *zone;
Outline and export free_reserved_page() because modules use it and it in turn uses page_ext_{get|put} which should not be exported. The same result could be obtained by outlining {get|put}_page_tag_ref() but that would have higher performance impact as these functions are used in more performance critical paths. 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> Suggested-by: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- Changes since v1 [1] - Outlined and exported free_reserved_page() in place of {get|put}_page_tag_ref, per Vlastimil Babka [1] https://lore.kernel.org/all/20240717011631.2150066-2-surenb@google.com/ include/linux/mm.h | 16 +--------------- mm/page_alloc.c | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 15 deletions(-)