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

[David Hildenbrand <david@redhat.com>]

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