[PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[Lance Yang]

Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
field that only works under CONFIG_MM_ID.

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Mingzhe Yang <mingzhe.yang@ly.com>
Signed-off-by: Lance Yang <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

Re: [PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[David Hildenbrand]

On 17.04.25 14:49, Lance Yang wrote:
> Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
> is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
> field that only works under CONFIG_MM_ID.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Mingzhe Yang <mingzhe.yang@ly.com>

^ should that be here?

> Signed-off-by: Lance Yang <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

That should work. I can throw this into a cross-compile setup later if I 
get to it.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[Lance Yang]

Hi David,

Thanks for taking the time to review!

On Thu, Apr 17, 2025 at 8:56 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 17.04.25 14:49, Lance Yang wrote:
> > Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
> > is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
> > field that only works under CONFIG_MM_ID.
> >
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Mingzhe Yang <mingzhe.yang@ly.com>
>
> ^ should that be here?

Yep, that's my email too ;p

>
> > Signed-off-by: Lance Yang <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
>
> That should work. I can throw this into a cross-compile setup later if I
> get to it.
>

Yeah, just built kernels with and without both CONFIG_MM_ID and
CONFIG_TRANSPARENT_HUGEPAGE -- no issues either way ;p

> Acked-by: David Hildenbrand <david@redhat.com>

Thanks again for your time,
Lance

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[David Hildenbrand]

On 17.04.25 15:24, Lance Yang wrote:
> Hi David,
> 
> Thanks for taking the time to review!
> 
> On Thu, Apr 17, 2025 at 8:56 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 17.04.25 14:49, Lance Yang wrote:
>>> Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
>>> is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
>>> field that only works under CONFIG_MM_ID.
>>>
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Mingzhe Yang <mingzhe.yang@ly.com>
>>
>> ^ should that be here?
> 
> Yep, that's my email too ;p

Best to stick to only one here -- same individual :)

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[Lance Yang]

On Thu, Apr 17, 2025 at 9:26 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 17.04.25 15:24, Lance Yang wrote:
> > Hi David,
> >
> > Thanks for taking the time to review!
> >
> > On Thu, Apr 17, 2025 at 8:56 PM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 17.04.25 14:49, Lance Yang wrote:
> >>> Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
> >>> is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
> >>> field that only works under CONFIG_MM_ID.
> >>>
> >>> Suggested-by: David Hildenbrand <david@redhat.com>
> >>> Signed-off-by: Mingzhe Yang <mingzhe.yang@ly.com>
> >>
> >> ^ should that be here?
> >
> > Yep, that's my email too ;p
>
> Best to stick to only one here -- same individual :)

Got it. I'll keep that in mind next time ;)

Thanks,
Lance

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[Andrew Morton]

On Thu, 17 Apr 2025 20:49:08 +0800 Lance Yang <ioworker0@gmail.com> wrote:

> Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
> is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
> field that only works under CONFIG_MM_ID.
> 
> ...
>
> --- 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

I don't get it.  Sounds like we're adding a compile-time check to check
for a compilation error which would have happened anyway.

If folio_test_large_maybe_mapped_shared() is only used with
CONFIG_MM_ID enabled, then do

#ifdef CONFIG_MM_ID
static inline bool folio_test_large_maybe_mapped_shared(...)
{
}
#endif

?

Or, as "_mm_ids field only works under CONFIG_MM_ID", make it
not-even-present when !CONFIG_MM_ID?

--- a/include/linux/mm_types.h~a
+++ a/include/linux/mm_types.h
@@ -438,7 +438,9 @@ struct folio {
 					mm_id_mapcount_t _mm_id_mapcount[2];
 					union {
 						mm_id_t _mm_id[2];
+#ifdef CONFIG_MM_ID
 						unsigned long _mm_ids;
+#endif
 					};
 	/* private: the union with struct page is transitional */
 				};
_

or

--- a/include/linux/mm_types.h~a
+++ a/include/linux/mm_types.h
@@ -436,10 +436,12 @@ struct folio {
 					atomic_t _pincount;
 #endif /* CONFIG_64BIT */
 					mm_id_mapcount_t _mm_id_mapcount[2];
+#ifdef CONFIG_MM_ID
 					union {
 						mm_id_t _mm_id[2];
 						unsigned long _mm_ids;
 					};
+#endif
 	/* private: the union with struct page is transitional */
 				};
 				unsigned long _usable_1[4];
_



I dunno, it sounds like something hasn't been fully thought through
here.  It's hard to say because the changelog is unclear.  Perhaps
start out by fully describing what problem the patch is addressing?

Re: [PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[Lance Yang]

Hi Andrew,

Thanks for taking the time to review!

On Fri, Apr 18, 2025 at 6:02 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 17 Apr 2025 20:49:08 +0800 Lance Yang <ioworker0@gmail.com> wrote:
>
> > Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
> > is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
> > field that only works under CONFIG_MM_ID.
> >
> > ...
> >
> > --- 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
>
> I don't get it.  Sounds like we're adding a compile-time check to check
> for a compilation error which would have happened anyway.
>
> If folio_test_large_maybe_mapped_shared() is only used with
> CONFIG_MM_ID enabled, then do
>
> #ifdef CONFIG_MM_ID
> static inline bool folio_test_large_maybe_mapped_shared(...)
> {
> }
> #endif
>
> ?

Hmm... we considered using '#ifdef CONFIG_MM_ID' for
folio_test_large_maybe_mapped_shared(),
but since this function should never be called without CONFIG_MM_ID
enabled, compile-time errors might be the way to go -- and a compile-time
check here does the trick ;)

>
> Or, as "_mm_ids field only works under CONFIG_MM_ID", make it
> not-even-present when !CONFIG_MM_ID?
>
> --- a/include/linux/mm_types.h~a
> +++ a/include/linux/mm_types.h
> @@ -438,7 +438,9 @@ struct folio {
>                                         mm_id_mapcount_t _mm_id_mapcount[2];
>                                         union {
>                                                 mm_id_t _mm_id[2];
> +#ifdef CONFIG_MM_ID
>                                                 unsigned long _mm_ids;
> +#endif
>                                         };
>         /* private: the union with struct page is transitional */
>                                 };
> _
>
> or
>
> --- a/include/linux/mm_types.h~a
> +++ a/include/linux/mm_types.h
> @@ -436,10 +436,12 @@ struct folio {
>                                         atomic_t _pincount;
>  #endif /* CONFIG_64BIT */
>                                         mm_id_mapcount_t _mm_id_mapcount[2];
> +#ifdef CONFIG_MM_ID
>                                         union {
>                                                 mm_id_t _mm_id[2];
>                                                 unsigned long _mm_ids;
>                                         };
> +#endif
>         /* private: the union with struct page is transitional */
>                                 };
>                                 unsigned long _usable_1[4];
> _
>
>
>
> I dunno, it sounds like something hasn't been fully thought through
> here.  It's hard to say because the changelog is unclear.  Perhaps
> start out by fully describing what problem the patch is addressing?
>

In patch #14[1], we rely on IS_ENABLED(CONFIG_MM_ID), so we need:
1) Some dummy functions
2) The _mm_ids field to remain present even when !CONFIG_MM_ID

This patch is intended to ensure that incorrect calls to
folio_test_large_maybe_mapped_shared()
are caught at compile-time.

[1] https://lore.kernel.org/linux-mm/20250303163014.1128035-15-david@redhat.com/

Thanks,
Lance

Re: [PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[David Hildenbrand]

On 18.04.25 09:26, Lance Yang wrote:
> Hi Andrew,
> 
> Thanks for taking the time to review!
> 
> On Fri, Apr 18, 2025 at 6:02 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>> On Thu, 17 Apr 2025 20:49:08 +0800 Lance Yang <ioworker0@gmail.com> wrote:
>>
>>> Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
>>> is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
>>> field that only works under CONFIG_MM_ID.
>>>
>>> ...
>>>
>>> --- 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
>>
>> I don't get it.  Sounds like we're adding a compile-time check to check
>> for a compilation error which would have happened anyway.
>>
>> If folio_test_large_maybe_mapped_shared() is only used with
>> CONFIG_MM_ID enabled, then do
>>
>> #ifdef CONFIG_MM_ID
>> static inline bool folio_test_large_maybe_mapped_shared(...)
>> {
>> }
>> #endif
>>
>> ?
> 
> Hmm... we considered using '#ifdef CONFIG_MM_ID' for
> folio_test_large_maybe_mapped_shared(),
> but since this function should never be called without CONFIG_MM_ID
> enabled, compile-time errors might be the way to go -- and a compile-time
> check here does the trick ;)

Yeah, I deliberately used plenty of IS_ENABLED to avoid a #ifdef mess 
all over the place.

Maybe clarify in the patch description that we want to prevent the 
function from getting used without CONFIG_MM_ID, and we don't want to 
use #ifdef because then we'd have to add even more #ifdef in callers 
that use IS_ENABLED(CONFIG_MM_ID).

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared()

[Lance Yang]

On Fri, Apr 18, 2025 at 6:38 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 18.04.25 09:26, Lance Yang wrote:
> > Hi Andrew,
> >
> > Thanks for taking the time to review!
> >
> > On Fri, Apr 18, 2025 at 6:02 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> >>
> >> On Thu, 17 Apr 2025 20:49:08 +0800 Lance Yang <ioworker0@gmail.com> wrote:
> >>
> >>> Add a compile-time check to make sure folio_test_large_maybe_mapped_shared()
> >>> is only used with CONFIG_MM_ID enabled, as it directly accesses the _mm_ids
> >>> field that only works under CONFIG_MM_ID.
> >>>
> >>> ...
> >>>
> >>> --- 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
> >>
> >> I don't get it.  Sounds like we're adding a compile-time check to check
> >> for a compilation error which would have happened anyway.
> >>
> >> If folio_test_large_maybe_mapped_shared() is only used with
> >> CONFIG_MM_ID enabled, then do
> >>
> >> #ifdef CONFIG_MM_ID
> >> static inline bool folio_test_large_maybe_mapped_shared(...)
> >> {
> >> }
> >> #endif
> >>
> >> ?
> >
> > Hmm... we considered using '#ifdef CONFIG_MM_ID' for
> > folio_test_large_maybe_mapped_shared(),
> > but since this function should never be called without CONFIG_MM_ID
> > enabled, compile-time errors might be the way to go -- and a compile-time
> > check here does the trick ;)
>
> Yeah, I deliberately used plenty of IS_ENABLED to avoid a #ifdef mess
> all over the place.
>
> Maybe clarify in the patch description that we want to prevent the
> function from getting used without CONFIG_MM_ID, and we don't want to
> use #ifdef because then we'd have to add even more #ifdef in callers
> that use IS_ENABLED(CONFIG_MM_ID).

Agreed, the patch description could be clearer. I'll update it to better
explain what it does and why we are avoiding #ifdef ;)

Thanks for your suggestion,
Lance

>
> --
> Cheers,
>
> David / dhildenb
>