diff mbox series

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

Message ID 20250417124908.58543-1-ioworker0@gmail.com (mailing list archive)
State New
Headers show
Series [1/1] mm/rmap: add CONFIG_MM_ID guard for folio_test_large_maybe_mapped_shared() | expand

Commit Message

Lance Yang April 17, 2025, 12:49 p.m. UTC 
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(+)

Comments

David Hildenbrand April 17, 2025, 12:56 p.m. UTC | #1 
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>
Lance Yang April 17, 2025, 1:24 p.m. UTC | #2 
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
>
David Hildenbrand April 17, 2025, 1:26 p.m. UTC | #3 
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 :)
Lance Yang April 17, 2025, 2:29 p.m. UTC | #4 
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
>
Andrew Morton April 17, 2025, 10:02 p.m. UTC | #5 
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 */
 				};
Lance Yang April 18, 2025, 7:26 a.m. UTC | #6 
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
David Hildenbrand April 18, 2025, 10:38 a.m. UTC | #7 
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).
Lance Yang April 18, 2025, 11:40 a.m. UTC | #8 
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
>
diff mbox series

Patch

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