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

[Lance Yang <ioworker0@gmail.com>]

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