* [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