Re: [Patch v3 2/2] mm/khugepaged: remove definition of struct khugepaged_mm_slot

[David Hildenbrand <david@redhat.com>]

On 24.09.25 02:48, Wei Yang wrote:
> Current code is not correct to get struct khugepaged_mm_slot by
> mm_slot_entry() without checking mm_slot is !NULL.  

"Current code calls mm_slot_entry() even when we don't have a valid 
slot, which is not future proof."

There is no problem
> reported since slot is the first element of struct khugepaged_mm_slot.

"Currently, this is not a problem because "slot" is the first member in 
struct khugepaged_mm_slot."

> 
> While struct khugepaged_mm_slot is just a wrapper of struct mm_slot, there
> is no need to define it.
> 
> Remove the definition of struct khugepaged_mm_slot, so there is not chance
> to miss use mm_slot_entry().
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Cc: Lance Yang <lance.yang@linux.dev>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Dev Jain <dev.jain@arm.com>
> Cc: Kiryl Shutsemau <kirill@shutemov.name>
> Cc: xu xin <xu.xin16@zte.com.cn>
> Cc: SeongJae Park <sj@kernel.org>
> Cc: Nico Pache <npache@redhat.com>
> 
> ---
> v3:
>    * fix a PF reported by SeongJae, where slot is changed to next one
> ---
>   mm/khugepaged.c | 58 ++++++++++++++++++-------------------------------
>   1 file changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 204ce3059267..e3f7d1760567 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -103,14 +103,6 @@ struct collapse_control {
>   	nodemask_t alloc_nmask;
>   };
>   
> -/**
> - * struct khugepaged_mm_slot - khugepaged information per mm that is being scanned
> - * @slot: hash lookup from mm to mm_slot
> - */
> -struct khugepaged_mm_slot {
> -	struct mm_slot slot;
> -};
> -
>   /**
>    * struct khugepaged_scan - cursor for scanning
>    * @mm_head: the head of the mm list to scan
> @@ -121,7 +113,7 @@ struct khugepaged_mm_slot {
>    */
>   struct khugepaged_scan {
>   	struct list_head mm_head;
> -	struct khugepaged_mm_slot *mm_slot;
> +	struct mm_slot *mm_slot;
>   	unsigned long address;
>   };
>   
> @@ -384,7 +376,10 @@ int hugepage_madvise(struct vm_area_struct *vma,
>   
>   int __init khugepaged_init(void)
>   {
> -	mm_slot_cache = KMEM_CACHE(khugepaged_mm_slot, 0);
> +	mm_slot_cache = kmem_cache_create("khugepaged_mm_slot",
> +					  sizeof(struct mm_slot),
> +					  __alignof__(struct mm_slot),
> +					  0, NULL);


Just wondering: do we really have to maintain the old name? Could we 
instead just have

mm_slot_cache = KMEM_CACHE(mm_slot, 0);

?

-- 
Cheers

David / dhildenb