Re: [PATCH] mm/mm_slot: make sure slot is the first element of its wrapper structure

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Sep 15, 2025 at 09:48:55AM +0200, David Hildenbrand wrote:
>On 15.09.25 09:42, Wei Yang wrote:
>> On Mon, Sep 15, 2025 at 09:32:27AM +0200, David Hildenbrand wrote:
>> > On 15.09.25 03:47, Wei Yang wrote:
>> > > On Sun, Sep 14, 2025 at 01:05:48PM +0800, Lance Yang wrote:
>> > > > Hi Wei,
>> > > > 
>> > > > On 2025/9/14 08:00, Wei Yang wrote:
>> > > > > When using mm_slot in ksm/khugepaged, there is code snip like:
>> > > > > 
>> > > > > 	slot = mm_slot_lookup(mm_slots_hash, mm);
>> > > > > 	mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
>> > > > > 	if (mm_slot && ..) {
>> > > > > 	}
>> > > > > 
>> > > > > This is only valid when mm_slot is the first element of its wrapper
>> > > > > structure, otherwise a NULL slot would converted to a mm_slot with
>> > > > > negative value. And current code thinks it is valid and continue.
>> > > > > 
>> > > > > Current code works since mm_slot is the first element, but make sure it
>> > > > > won't be disturbed.
>> > > > 
>> > > > Good catch! That's indeed quite brittle ;)
>> > > > 
>> > > > Just one nit below.
>> > > > 
>> > > > > 
>> > > > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> > > > > ---
>> > > > >     mm/khugepaged.c | 5 ++++-
>> > > > >     mm/ksm.c        | 5 ++++-
>> > > > >     2 files changed, 8 insertions(+), 2 deletions(-)
>> > > > > 
>> > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> > > > > index af5f5c80fe4e..668e74ad33b7 100644
>> > > > > --- a/mm/khugepaged.c
>> > > > > +++ b/mm/khugepaged.c
>> > > > > @@ -108,7 +108,7 @@ struct collapse_control {
>> > > > >      * @slot: hash lookup from mm to mm_slot
>> > > > >      */
>> > > > >     struct khugepaged_mm_slot {
>> > > > > -	struct mm_slot slot;
>> > > > > +	struct mm_slot slot;	/* keep it the first element */
>> > > > >     };
>> > > > >     /**
>> > > > > @@ -2382,6 +2382,9 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
>> > > > >     	struct vm_area_struct *vma;
>> > > > >     	int progress = 0;
>> > > > > +	BUILD_BUG_ON_MSG(mm_slot_entry(NULL, struct khugepaged_mm_slot, slot),
>> > > > > +			"slot should be the first element");
>> > > > > +
>> > > > 
>> > > > I wonder if this BUILD_BUG_ON_MSG() in khugepaged_scan_mm_slot() would
>> > > > be better placed in khugepaged_init(), like you did in ksm_init()?
>> > > 
>> > > khugepaged_mm_slot is defined in khugepaged.c, maybe we don't want to export
>> > > it.
>> > > 
>> > 
>> > We should probably just use a static_assert right nxt to the struct and make
>> > sure that offsetof(struct khugepaged_mm_slot, slot) == 0 ?
>> > 
>> 
>> I am ok with this, but Dev suggest to fix in the code.
>> 
>> Hmm... both works to me.
>
>Right, if it can just be made working without requiring it to be the first
>entry, we should do that instead.
>

Got it, thanks.

>-- 
>Cheers
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me