* [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() @ 2025-04-18 15:22 Lance Yang 2025-04-20 23:29 ` Andrew Morton 0 siblings, 1 reply; 10+ messages in thread From: Lance Yang @ 2025-04-18 15:22 UTC (permalink / raw) To: akpm; +Cc: mingzhe.yang, linux-mm, linux-kernel, David Hildenbrand, Lance Yang From: Lance Yang <lance.yang@linux.dev> To prevent folio_test_large_maybe_mapped_shared() from being used without CONFIG_MM_ID, we add a compile-time check rather than wrapping it in '#ifdef', avoiding even more #ifdef in callers that already use IS_ENABLED(CONFIG_MM_ID). Also, we used plenty of IS_ENABLED() on purpose to keep the code free of '#ifdef' mess. Suggested-by: David Hildenbrand <david@redhat.com> Signed-off-by: Lance Yang <lance.yang@linux.dev> Acked-by: David Hildenbrand <david@redhat.com> --- v1 -> v2: * Update the changelog, suggested by Andrew and David * https://lore.kernel.org/linux-mm/20250417124908.58543-1-ioworker0@gmail.com include/linux/page-flags.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index d3909cb1e576..6bd9b9043976 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -1232,6 +1232,8 @@ static inline int folio_has_private(const struct folio *folio) static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) { + /* This function should never be called without CONFIG_MM_ID enabled. */ + BUILD_BUG_ON(!IS_ENABLED(CONFIG_MM_ID)); return test_bit(FOLIO_MM_IDS_SHARED_BITNUM, &folio->_mm_ids); } #undef PF_ANY -- 2.49.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-18 15:22 [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() Lance Yang @ 2025-04-20 23:29 ` Andrew Morton 2025-04-21 5:13 ` Lance Yang 0 siblings, 1 reply; 10+ messages in thread From: Andrew Morton @ 2025-04-20 23:29 UTC (permalink / raw) To: Lance Yang Cc: mingzhe.yang, linux-mm, linux-kernel, David Hildenbrand, Lance Yang On Fri, 18 Apr 2025 23:22:28 +0800 Lance Yang <ioworker0@gmail.com> wrote: > From: Lance Yang <lance.yang@linux.dev> > > To prevent folio_test_large_maybe_mapped_shared() from being used without > CONFIG_MM_ID, we add a compile-time check rather than wrapping it in > '#ifdef', avoiding even more #ifdef in callers that already use > IS_ENABLED(CONFIG_MM_ID). > > Also, we used plenty of IS_ENABLED() on purpose to keep the code free of > '#ifdef' mess. I dunno, this just seems really whacky. > --- a/include/linux/page-flags.h > +++ b/include/linux/page-flags.h > @@ -1232,6 +1232,8 @@ static inline int folio_has_private(const struct folio *folio) > > static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) > { > + /* This function should never be called without CONFIG_MM_ID enabled. */ A correcter comment would be "This function should never be compiled without CONFIG_MM_ID enabled". Which lets the cat out of the bag. Why the heck is it being compiled with CONFIG_MM_ID=n?? We have tools to prevent that. Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It should have no callers, right? If the linker ends up complaining then something went wrong. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-20 23:29 ` Andrew Morton @ 2025-04-21 5:13 ` Lance Yang 2025-04-21 5:16 ` Lance Yang 2025-04-21 19:22 ` Andrew Morton 0 siblings, 2 replies; 10+ messages in thread From: Lance Yang @ 2025-04-21 5:13 UTC (permalink / raw) To: Andrew Morton, David Hildenbrand Cc: mingzhe.yang, linux-mm, linux-kernel, Lance Yang April 21, 2025 at 7:29 AM, "Andrew Morton" <akpm@linux-foundation.org> wrote: > > On Fri, 18 Apr 2025 23:22:28 +0800 Lance Yang <ioworker0@gmail.com> wrote: > > > > > From: Lance Yang <lance.yang@linux.dev> > > > > > > > > To prevent folio_test_large_maybe_mapped_shared() from being used without > > > > CONFIG_MM_ID, we add a compile-time check rather than wrapping it in > > > > '#ifdef', avoiding even more #ifdef in callers that already use > > > > IS_ENABLED(CONFIG_MM_ID). > > > > > > > > Also, we used plenty of IS_ENABLED() on purpose to keep the code free of > > > > '#ifdef' mess. > > > > I dunno, this just seems really whacky. I'd hope David could leave some comments on that. > > > > > --- a/include/linux/page-flags.h > > > > +++ b/include/linux/page-flags.h > > > > @@ -1232,6 +1232,8 @@ static inline int folio_has_private(const struct folio *folio) > > > > > > > > static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) > > > > { > > > > + /* This function should never be called without CONFIG_MM_ID enabled. */ > > > > A correcter comment would be "This function should never be compiled > > without CONFIG_MM_ID enabled". Yes, that is more exact ;) > > Which lets the cat out of the bag. Why the heck is it being compiled > > with CONFIG_MM_ID=n?? We have tools to prevent that. > > Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It > > should have no callers, right? If the linker ends up complaining then > > something went wrong. The reason we can't simply add #ifdef CONFIG_MM_ID around folio_test_large_maybe_mapped_shared() is because its caller folio_maybe_mapped_shared() relies on IS_ENABLED(CONFIG_MM_ID). If we do, with CONFIG_TRANSPARENT_HUGEPAGE=N, we'll hit compilation errors like: ./include/linux/mm.h: In function ‘folio_maybe_mapped_shared’: ./include/linux/mm.h:2337:16: error: implicit declaration of function ‘folio_test_large_maybe_mapped_shared’; did you mean ‘folio_maybe_mapped_shared’? [-Werror=implicit-function-declaration] 2337 | return folio_test_large_maybe_mapped_shared(folio); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | folio_maybe_mapped_shared cc1: some warnings being treated as errors Thanks, Lance > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-21 5:13 ` Lance Yang @ 2025-04-21 5:16 ` Lance Yang 2025-04-21 7:17 ` David Hildenbrand 2025-04-21 19:22 ` Andrew Morton 1 sibling, 1 reply; 10+ messages in thread From: Lance Yang @ 2025-04-21 5:16 UTC (permalink / raw) To: Andrew Morton, David Hildenbrand Cc: mingzhe.yang, linux-mm, linux-kernel, Lance Yang April 21, 2025 at 1:13 PM, "Lance Yang" <lance.yang@linux.dev> wrote: > > April 21, 2025 at 7:29 AM, "Andrew Morton" <akpm@linux-foundation.org> wrote: > > > > > On Fri, 18 Apr 2025 23:22:28 +0800 Lance Yang <ioworker0@gmail.com> wrote: > > > > > > > > > > > > From: Lance Yang <lance.yang@linux.dev> > > > > > > > > > > > > > > > > To prevent folio_test_large_maybe_mapped_shared() from being used without > > > > > > > > CONFIG_MM_ID, we add a compile-time check rather than wrapping it in > > > > > > > > '#ifdef', avoiding even more #ifdef in callers that already use > > > > > > > > IS_ENABLED(CONFIG_MM_ID). > > > > > > > > > > > > > > > > Also, we used plenty of IS_ENABLED() on purpose to keep the code free of > > > > > > > > '#ifdef' mess. > > > > > > > > > > > > I dunno, this just seems really whacky. > > > > I'd hope David could leave some comments on that. > > > > > --- a/include/linux/page-flags.h > > > > > > > > +++ b/include/linux/page-flags.h > > > > > > > > @@ -1232,6 +1232,8 @@ static inline int folio_has_private(const struct folio *folio) > > > > > > > > > > > > > > > > static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) > > > > > > > > { > > > > > > > > + /* This function should never be called without CONFIG_MM_ID enabled. */ > > > > > > > > > > > > A correcter comment would be "This function should never be compiled > > > > > > > > without CONFIG_MM_ID enabled". > > > > Yes, that is more exact ;) > > > > > Which lets the cat out of the bag. Why the heck is it being compiled > > > > > > > > with CONFIG_MM_ID=n?? We have tools to prevent that. > > > > > > > > Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It > > > > > > > > should have no callers, right? If the linker ends up complaining then > > > > > > > > something went wrong. > > > > The reason we can't simply add #ifdef CONFIG_MM_ID around folio_test_large_maybe_mapped_shared() > > is because its caller folio_maybe_mapped_shared() relies on IS_ENABLED(CONFIG_MM_ID). static inline bool folio_maybe_mapped_shared(struct folio *folio) { [...] if (!IS_ENABLED(CONFIG_MM_ID)) return true; [...] return folio_test_large_maybe_mapped_shared(folio); } folio_maybe_mapped_shared() is always available - doesn't depend on CONFIG_MM_ID or CONFIG_TRANSPARENT_HUGEPAGE. Thanks, Lance > > If we do, with CONFIG_TRANSPARENT_HUGEPAGE=N, we'll hit compilation errors like: > > ./include/linux/mm.h: In function ‘folio_maybe_mapped_shared’: > > ./include/linux/mm.h:2337:16: error: implicit declaration of function ‘folio_test_large_maybe_mapped_shared’; did you mean ‘folio_maybe_mapped_shared’? [-Werror=implicit-function-declaration] > > 2337 | return folio_test_large_maybe_mapped_shared(folio); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | folio_maybe_mapped_shared > > cc1: some warnings being treated as errors > > Thanks, > > Lance > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-21 5:16 ` Lance Yang @ 2025-04-21 7:17 ` David Hildenbrand 2025-04-21 7:50 ` Lance Yang 0 siblings, 1 reply; 10+ messages in thread From: David Hildenbrand @ 2025-04-21 7:17 UTC (permalink / raw) To: Lance Yang, Andrew Morton Cc: mingzhe.yang, linux-mm, linux-kernel, Lance Yang On 21.04.25 07:16, Lance Yang wrote: > April 21, 2025 at 1:13 PM, "Lance Yang" <lance.yang@linux.dev> wrote: > > > >> >> April 21, 2025 at 7:29 AM, "Andrew Morton" <akpm@linux-foundation.org> wrote: >> >>> >>> On Fri, 18 Apr 2025 23:22:28 +0800 Lance Yang <ioworker0@gmail.com> wrote: >>> >>> >>> >>> >>> >>> From: Lance Yang <lance.yang@linux.dev> >>> >>> >>> >>> >>> >>> >>> >>> To prevent folio_test_large_maybe_mapped_shared() from being used without >>> >>> >>> >>> CONFIG_MM_ID, we add a compile-time check rather than wrapping it in >>> >>> >>> >>> '#ifdef', avoiding even more #ifdef in callers that already use >>> >>> >>> >>> IS_ENABLED(CONFIG_MM_ID). >>> >>> >>> >>> >>> >>> >>> >>> Also, we used plenty of IS_ENABLED() on purpose to keep the code free of >>> >>> >>> >>> '#ifdef' mess. >>> >>> >>> >>> >>> >>> I dunno, this just seems really whacky. >>> >> >> I'd hope David could leave some comments on that. >> >>> >>> --- a/include/linux/page-flags.h >>> >>> >>> >>> +++ b/include/linux/page-flags.h >>> >>> >>> >>> @@ -1232,6 +1232,8 @@ static inline int folio_has_private(const struct folio *folio) >>> >>> >>> >>> >>> >>> >>> >>> static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) >>> >>> >>> >>> { >>> >>> >>> >>> + /* This function should never be called without CONFIG_MM_ID enabled. */ >>> >>> >>> >>> >>> >>> A correcter comment would be "This function should never be compiled >>> >>> >>> >>> without CONFIG_MM_ID enabled". >>> >> >> Yes, that is more exact ;) >> >>> >>> Which lets the cat out of the bag. Why the heck is it being compiled >>> >>> >>> >>> with CONFIG_MM_ID=n?? We have tools to prevent that. >>> >>> >>> >>> Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It >>> >>> >>> >>> should have no callers, right? If the linker ends up complaining then >>> >>> >>> >>> something went wrong. >>> >> >> The reason we can't simply add #ifdef CONFIG_MM_ID around folio_test_large_maybe_mapped_shared() >> >> is because its caller folio_maybe_mapped_shared() relies on IS_ENABLED(CONFIG_MM_ID). > > static inline bool folio_maybe_mapped_shared(struct folio *folio) > { > [...] > if (!IS_ENABLED(CONFIG_MM_ID)) > return true; > [...] > return folio_test_large_maybe_mapped_shared(folio); > } > > folio_maybe_mapped_shared() is always available - doesn't depend on > CONFIG_MM_ID or CONFIG_TRANSPARENT_HUGEPAGE. We could #ifdef in folio_maybe_mapped_shared(), which I find rather suboptimal ... or simply inline it into the 4 callers. That might be the best approach, given that only selected user should be using the low-level primitive and everybody else should be using folio_maybe_mapped_shared(). -- Cheers, David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-21 7:17 ` David Hildenbrand @ 2025-04-21 7:50 ` Lance Yang 0 siblings, 0 replies; 10+ messages in thread From: Lance Yang @ 2025-04-21 7:50 UTC (permalink / raw) To: David Hildenbrand, Andrew Morton Cc: mingzhe.yang, linux-mm, linux-kernel, Lance Yang April 21, 2025 at 3:17 PM, "David Hildenbrand" <david@redhat.com> wrote: > > On 21.04.25 07:16, Lance Yang wrote: > > > > > April 21, 2025 at 1:13 PM, "Lance Yang" <lance.yang@linux.dev> wrote: > > > > > >> > > > > > > > > April 21, 2025 at 7:29 AM, "Andrew Morton" <akpm@linux-foundation.org> wrote: > > > > > > > On Fri, 18 Apr 2025 23:22:28 +0800 Lance Yang <ioworker0@gmail.com> wrote: > > > > >>> > > > > >>> > > > > From: Lance Yang <lance.yang@linux.dev> > > > > >>> > > > > >>> > > > > >>> > > > > To prevent folio_test_large_maybe_mapped_shared() from being used without > > > > >>> > > > > CONFIG_MM_ID, we add a compile-time check rather than wrapping it in > > > > >>> > > > > '#ifdef', avoiding even more #ifdef in callers that already use > > > > >>> > > > > IS_ENABLED(CONFIG_MM_ID). > > > > >>> > > > > >>> > > > > >>> > > > > Also, we used plenty of IS_ENABLED() on purpose to keep the code free of > > > > >>> > > > > '#ifdef' mess. > > > > >>> > > > > >>> > > > > I dunno, this just seems really whacky. > > > > > > > > I'd hope David could leave some comments on that. > > > > > > > --- a/include/linux/page-flags.h > > > > >>> > > > > +++ b/include/linux/page-flags.h > > > > >>> > > > > @@ -1232,6 +1232,8 @@ static inline int folio_has_private(const struct folio *folio) > > > > >>> > > > > >>> > > > > >>> > > > > static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) > > > > >>> > > > > { > > > > >>> > > > > + /* This function should never be called without CONFIG_MM_ID enabled. */ > > > > >>> > > > > >>> > > > > A correcter comment would be "This function should never be compiled > > > > >>> > > > > without CONFIG_MM_ID enabled". > > > > > > > > Yes, that is more exact ;) > > > > > > > Which lets the cat out of the bag. Why the heck is it being compiled > > > > >>> > > > > with CONFIG_MM_ID=n?? We have tools to prevent that. > > > > >>> > > > > Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It > > > > >>> > > > > should have no callers, right? If the linker ends up complaining then > > > > >>> > > > > something went wrong. > > > > > > > > The reason we can't simply add #ifdef CONFIG_MM_ID around folio_test_large_maybe_mapped_shared() > > > > > > is because its caller folio_maybe_mapped_shared() relies on IS_ENABLED(CONFIG_MM_ID). > > > > > > > static inline bool folio_maybe_mapped_shared(struct folio *folio) > > > > { > > > > [...] > > > > if (!IS_ENABLED(CONFIG_MM_ID)) > > > > return true; > > > > [...] > > > > return folio_test_large_maybe_mapped_shared(folio); > > > > } > > > > folio_maybe_mapped_shared() is always available - doesn't depend on > > > > CONFIG_MM_ID or CONFIG_TRANSPARENT_HUGEPAGE. > > > > We could #ifdef in folio_maybe_mapped_shared(), which I find rather suboptimal ... > > or simply inline it into the 4 callers. > > That might be the best approach, given that only selected user should be using the low-level primitive and everybody else should be using folio_maybe_mapped_shared(). Yep, I tend to prefer the second approach as well, given there are only four callers. Andrew, wdyt? Thanks, Lance > > -- Cheers, > > David / dhildenb > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-21 5:13 ` Lance Yang 2025-04-21 5:16 ` Lance Yang @ 2025-04-21 19:22 ` Andrew Morton 2025-04-22 4:35 ` Lance Yang 1 sibling, 1 reply; 10+ messages in thread From: Andrew Morton @ 2025-04-21 19:22 UTC (permalink / raw) To: Lance Yang Cc: David Hildenbrand, mingzhe.yang, linux-mm, linux-kernel, Lance Yang On Mon, 21 Apr 2025 05:13:03 +0000 "Lance Yang" <lance.yang@linux.dev> wrote: > > Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It > > > > should have no callers, right? If the linker ends up complaining then > > > > something went wrong. > > The reason we can't simply add #ifdef CONFIG_MM_ID around folio_test_large_maybe_mapped_shared() > is because its caller folio_maybe_mapped_shared() relies on IS_ENABLED(CONFIG_MM_ID). > > If we do, with CONFIG_TRANSPARENT_HUGEPAGE=N, we'll hit compilation errors like: > > ./include/linux/mm.h: In function ‘folio_maybe_mapped_shared’: > ./include/linux/mm.h:2337:16: error: implicit declaration of function ‘folio_test_large_maybe_mapped_shared’; did you mean ‘folio_maybe_mapped_shared’? [-Werror=implicit-function-declaration] > 2337 | return folio_test_large_maybe_mapped_shared(folio); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | folio_maybe_mapped_shared > cc1: some warnings being treated as errors That's OK - provide a declaration of folio_maybe_mapped_shared() but no definition. So the compiled-out code can be compiled and the linker will confirm that it's never actually called. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-21 19:22 ` Andrew Morton @ 2025-04-22 4:35 ` Lance Yang 2025-04-22 7:00 ` David Hildenbrand 0 siblings, 1 reply; 10+ messages in thread From: Lance Yang @ 2025-04-22 4:35 UTC (permalink / raw) To: Andrew Morton, David Hildenbrand Cc: mingzhe.yang, linux-mm, linux-kernel, Lance Yang April 22, 2025 at 3:22 AM, "Andrew Morton" <akpm@linux-foundation.org> wrote: > > On Mon, 21 Apr 2025 05:13:03 +0000 "Lance Yang" <lance.yang@linux.dev> wrote: > > > > > Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It > > > > > > > > should have no callers, right? If the linker ends up complaining then > > > > > > > > something went wrong. > > > > > > > > The reason we can't simply add #ifdef CONFIG_MM_ID around folio_test_large_maybe_mapped_shared() > > > > is because its caller folio_maybe_mapped_shared() relies on IS_ENABLED(CONFIG_MM_ID). > > > > > > > > If we do, with CONFIG_TRANSPARENT_HUGEPAGE=N, we'll hit compilation errors like: > > > > > > > > ./include/linux/mm.h: In function ‘folio_maybe_mapped_shared’: > > > > ./include/linux/mm.h:2337:16: error: implicit declaration of function ‘folio_test_large_maybe_mapped_shared’; did you mean ‘folio_maybe_mapped_shared’? [-Werror=implicit-function-declaration] > > > > 2337 | return folio_test_large_maybe_mapped_shared(folio); > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > | folio_maybe_mapped_shared > > > > cc1: some warnings being treated as errors > > > > That's OK - provide a declaration of folio_maybe_mapped_shared() but no > > definition. So the compiled-out code can be compiled and the linker > > will confirm that it's never actually called. > Got it, that works as well ;) So if David is cool with it, I'll send out the new version like this: diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h index d3909cb1e576..a762e4b4eab4 100644 --- a/include/linux/page-flags.h +++ b/include/linux/page-flags.h @@ -1230,10 +1230,15 @@ static inline int folio_has_private(const struct folio *folio) return !!(folio->flags & PAGE_FLAGS_PRIVATE); } +#ifdef CONFIG_MM_ID static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) { return test_bit(FOLIO_MM_IDS_SHARED_BITNUM, &folio->_mm_ids); } +#else +bool folio_test_large_maybe_mapped_shared(const struct folio *folio); +#endif + Thanks, Lance ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-22 4:35 ` Lance Yang @ 2025-04-22 7:00 ` David Hildenbrand 2025-04-22 23:20 ` Andrew Morton 0 siblings, 1 reply; 10+ messages in thread From: David Hildenbrand @ 2025-04-22 7:00 UTC (permalink / raw) To: Lance Yang, Andrew Morton Cc: mingzhe.yang, linux-mm, linux-kernel, Lance Yang On 22.04.25 06:35, Lance Yang wrote: > April 22, 2025 at 3:22 AM, "Andrew Morton" <akpm@linux-foundation.org> wrote: > > > >> >> On Mon, 21 Apr 2025 05:13:03 +0000 "Lance Yang" <lance.yang@linux.dev> wrote: >> >>> >>> Can we just slap "#ifdef CONFIG_MM_ID" around the whole function? It >>> >>> >>> >>> should have no callers, right? If the linker ends up complaining then >>> >>> >>> >>> something went wrong. >>> >>> >>> >>> The reason we can't simply add #ifdef CONFIG_MM_ID around folio_test_large_maybe_mapped_shared() >>> >>> is because its caller folio_maybe_mapped_shared() relies on IS_ENABLED(CONFIG_MM_ID). >>> >>> >>> >>> If we do, with CONFIG_TRANSPARENT_HUGEPAGE=N, we'll hit compilation errors like: >>> >>> >>> >>> ./include/linux/mm.h: In function ‘folio_maybe_mapped_shared’: >>> >>> ./include/linux/mm.h:2337:16: error: implicit declaration of function ‘folio_test_large_maybe_mapped_shared’; did you mean ‘folio_maybe_mapped_shared’? [-Werror=implicit-function-declaration] >>> >>> 2337 | return folio_test_large_maybe_mapped_shared(folio); >>> >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> | folio_maybe_mapped_shared >>> >>> cc1: some warnings being treated as errors >>> >> >> That's OK - provide a declaration of folio_maybe_mapped_shared() but no >> >> definition. So the compiled-out code can be compiled and the linker >> >> will confirm that it's never actually called. >> > > Got it, that works as well ;) > > So if David is cool with it, I'll send out the new version like this: > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > index d3909cb1e576..a762e4b4eab4 100644 > --- a/include/linux/page-flags.h > +++ b/include/linux/page-flags.h > @@ -1230,10 +1230,15 @@ static inline int folio_has_private(const struct folio *folio) > return !!(folio->flags & PAGE_FLAGS_PRIVATE); > } > > +#ifdef CONFIG_MM_ID > static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) > { > return test_bit(FOLIO_MM_IDS_SHARED_BITNUM, &folio->_mm_ids); > } > +#else > +bool folio_test_large_maybe_mapped_shared(const struct folio *folio); > +#endif Fine with me. At this point, I do prefer inlining the function, though. -- Cheers, David / dhildenb ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() 2025-04-22 7:00 ` David Hildenbrand @ 2025-04-22 23:20 ` Andrew Morton 0 siblings, 0 replies; 10+ messages in thread From: Andrew Morton @ 2025-04-22 23:20 UTC (permalink / raw) To: David Hildenbrand Cc: Lance Yang, mingzhe.yang, linux-mm, linux-kernel, Lance Yang On Tue, 22 Apr 2025 09:00:54 +0200 David Hildenbrand <david@redhat.com> wrote: > > --- a/include/linux/page-flags.h > > +++ b/include/linux/page-flags.h > > @@ -1230,10 +1230,15 @@ static inline int folio_has_private(const struct folio *folio) > > return !!(folio->flags & PAGE_FLAGS_PRIVATE); > > } > > > > +#ifdef CONFIG_MM_ID > > static inline bool folio_test_large_maybe_mapped_shared(const struct folio *folio) > > { > > return test_bit(FOLIO_MM_IDS_SHARED_BITNUM, &folio->_mm_ids); > > } > > +#else > > +bool folio_test_large_maybe_mapped_shared(const struct folio *folio); > > +#endif > > Fine with me. At this point, I do prefer inlining the function, though. The above has the advantage that if the compiler unintendedly emits a call to folio_test_large_maybe_mapped_shared(), the link will fail. I do think a little comment which explains this trick is needed. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-04-22 23:20 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-04-18 15:22 [RESEND PATCH v2 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() Lance Yang 2025-04-20 23:29 ` Andrew Morton 2025-04-21 5:13 ` Lance Yang 2025-04-21 5:16 ` Lance Yang 2025-04-21 7:17 ` David Hildenbrand 2025-04-21 7:50 ` Lance Yang 2025-04-21 19:22 ` Andrew Morton 2025-04-22 4:35 ` Lance Yang 2025-04-22 7:00 ` David Hildenbrand 2025-04-22 23:20 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox