Re: [PATCH 1/5] mm: use inline helper functions instead of ugly macros

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2/25/26 9:56 PM, David Hildenbrand (Arm) wrote:
> On 2/24/26 02:56, Baolin Wang wrote:
>> People have already complained that these *_clear_young_notify() related
>> macros are very ugly, so let's use inline helpers to make them more readable.
> 
> Could it have been me :)

Lorenzo and Matthew have also said they dislike these ugly macros:)

>> In addition, I cannot implement these inline helper functions in the
> 
> "We cannot implement ..."

Sure.

>> mmu_notifier.h file, because some arch-specific files will include the
>> mmu_notifier.h, which introduces header compilation dependencies and causes
>> build errors (e.g., arch/arm64/include/asm/tlbflush.h). Moreover, since
>> these functions are only used in the mm, implementing these inline helpers
>> in the mm/internal.h header seems reasonable.
> 
> Agreed. if it compiles, all good.
> 
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   include/linux/mmu_notifier.h | 54 ------------------------------------
>>   mm/internal.h                | 53 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 53 insertions(+), 54 deletions(-)
>>
>> diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
>> index 07a2bbaf86e9..93894b90c8c1 100644
>> --- a/include/linux/mmu_notifier.h
>> +++ b/include/linux/mmu_notifier.h
>> @@ -515,55 +515,6 @@ static inline void mmu_notifier_range_init_owner(
>>   	range->owner = owner;
>>   }
>>   
>> -#define clear_flush_young_ptes_notify(__vma, __address, __ptep, __nr)	\
>> -({									\
>> -	int __young;							\
>> -	struct vm_area_struct *___vma = __vma;				\
>> -	unsigned long ___address = __address;				\
>> -	unsigned int ___nr = __nr;					\
>> -	__young = clear_flush_young_ptes(___vma, ___address, __ptep, ___nr);	\
>> -	__young |= mmu_notifier_clear_flush_young(___vma->vm_mm,	\
>> -						  ___address,		\
>> -						  ___address +		\
>> -						  ___nr * PAGE_SIZE);	\
>> -	__young;							\
>> -})
>> -
>> -#define pmdp_clear_flush_young_notify(__vma, __address, __pmdp)		\
>> -({									\
>> -	int __young;							\
>> -	struct vm_area_struct *___vma = __vma;				\
>> -	unsigned long ___address = __address;				\
>> -	__young = pmdp_clear_flush_young(___vma, ___address, __pmdp);	\
>> -	__young |= mmu_notifier_clear_flush_young(___vma->vm_mm,	\
>> -						  ___address,		\
>> -						  ___address +		\
>> -							PMD_SIZE);	\
>> -	__young;							\
>> -})
>> -
>> -#define ptep_clear_young_notify(__vma, __address, __ptep)		\
>> -({									\
>> -	int __young;							\
>> -	struct vm_area_struct *___vma = __vma;				\
>> -	unsigned long ___address = __address;				\
>> -	__young = ptep_test_and_clear_young(___vma, ___address, __ptep);\
>> -	__young |= mmu_notifier_clear_young(___vma->vm_mm, ___address,	\
>> -					    ___address + PAGE_SIZE);	\
>> -	__young;							\
>> -})
>> -
>> -#define pmdp_clear_young_notify(__vma, __address, __pmdp)		\
>> -({									\
>> -	int __young;							\
>> -	struct vm_area_struct *___vma = __vma;				\
>> -	unsigned long ___address = __address;				\
>> -	__young = pmdp_test_and_clear_young(___vma, ___address, __pmdp);\
>> -	__young |= mmu_notifier_clear_young(___vma->vm_mm, ___address,	\
>> -					    ___address + PMD_SIZE);	\
>> -	__young;							\
>> -})
>> -
>>   #else /* CONFIG_MMU_NOTIFIER */
>>   
>>   struct mmu_notifier_range {
>> @@ -651,11 +602,6 @@ static inline void mmu_notifier_subscriptions_destroy(struct mm_struct *mm)
>>   
>>   #define mmu_notifier_range_update_to_read_only(r) false
>>   
>> -#define clear_flush_young_ptes_notify clear_flush_young_ptes
>> -#define pmdp_clear_flush_young_notify pmdp_clear_flush_young
>> -#define ptep_clear_young_notify ptep_test_and_clear_young
>> -#define pmdp_clear_young_notify pmdp_test_and_clear_young
>> -
>>   static inline void mmu_notifier_synchronize(void)
>>   {
>>   }
>> diff --git a/mm/internal.h b/mm/internal.h
>> index e0ef192b0be3..1ba175b8d4f1 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -11,6 +11,7 @@
>>   #include <linux/khugepaged.h>
>>   #include <linux/mm.h>
>>   #include <linux/mm_inline.h>
>> +#include <linux/mmu_notifier.h>
>>   #include <linux/pagemap.h>
>>   #include <linux/pagewalk.h>
>>   #include <linux/rmap.h>
>> @@ -1789,4 +1790,56 @@ static inline int io_remap_pfn_range_complete(struct vm_area_struct *vma,
>>   	return remap_pfn_range_complete(vma, addr, pfn, size, prot);
>>   }
>>   
>> +#ifdef CONFIG_MMU_NOTIFIER
>> +static inline int clear_flush_young_ptes_notify(struct vm_area_struct *vma,
>> +						unsigned long addr, pte_t *ptep,
>> +						unsigned int nr)
> 
> Two tabs indent on the second line makes this fit into two lines.

OK.

> Same for the others.
> 
> With that
> 
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>

Thanks.

>> +{
>> +	int young;
>> +
>> +	young = clear_flush_young_ptes(vma, addr, ptep, nr);
>> +	young |= mmu_notifier_clear_flush_young(vma->vm_mm, addr,
>> +						addr + nr * PAGE_SIZE);
>> +	return young;
>> +}
>> +
>> +static inline int pmdp_clear_flush_young_notify(struct vm_area_struct *vma,
>> +						unsigned long addr, pmd_t *pmdp)
>> +{
>> +	int young;
>> +
>> +	young = pmdp_clear_flush_young(vma, addr, pmdp);
>> +	young |= mmu_notifier_clear_flush_young(vma->vm_mm, addr, addr + PMD_SIZE);
>> +	return young;
>> +}
>> +
>> +static inline int ptep_clear_young_notify(struct vm_area_struct *vma,
>> +					  unsigned long addr, pte_t *ptep)
>> +{
>> +	int young;
>> +
>> +	young = ptep_test_and_clear_young(vma, addr, ptep);
>> +	young |= mmu_notifier_clear_young(vma->vm_mm, addr, addr + PAGE_SIZE);
>> +	return young;
>> +}
>> +
>> +static inline int pmdp_clear_young_notify(struct vm_area_struct *vma,
>> +					  unsigned long addr, pmd_t *pmdp)
>> +{
>> +	int young;
>> +
>> +	young = pmdp_test_and_clear_young(vma, addr, pmdp);
>> +	young |= mmu_notifier_clear_young(vma->vm_mm, addr, addr + PMD_SIZE);
>> +	return young;
>> +}
> 
> Wonder why someone thought it would be a good idea to not include the
> "test_and_", to make the function names inconsistent.

Sounds reasonable. I can send a patch to rename them to:
ptep_test_and_clear_young_notify
pmdp_test_and_clear_young_notify