Re: [PATCH v4] mm: use per_vma lock for MADV_DONTNEED

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2025/11/4 17:01, Lorenzo Stoakes wrote:
> On Tue, Nov 04, 2025 at 04:34:35PM +0800, Kefeng Wang wrote:
>>> +static enum madvise_lock_mode get_lock_mode(struct madvise_behavior *madv_behavior)
>>>    {
>>> +	int behavior = madv_behavior->behavior;
>>> +
>>>    	if (is_memory_failure(behavior))
>>> -		return 0;
>>> +		return MADVISE_NO_LOCK;
>>> -	if (madvise_need_mmap_write(behavior)) {
>>> +	switch (behavior) {
>>> +	case MADV_REMOVE:
>>> +	case MADV_WILLNEED:
>>> +	case MADV_COLD:
>>> +	case MADV_PAGEOUT:
>>> +	case MADV_FREE:
>>> +	case MADV_POPULATE_READ:
>>> +	case MADV_POPULATE_WRITE:
>>> +	case MADV_COLLAPSE:
>>> +	case MADV_GUARD_INSTALL:
>>> +	case MADV_GUARD_REMOVE:
>>> +		return MADVISE_MMAP_READ_LOCK;
>>> +	case MADV_DONTNEED:
>>> +	case MADV_DONTNEED_LOCKED:
>>> +		return MADVISE_VMA_READ_LOCK;
>>
>> I have a question, we will try per-vma lock for dontneed,
>> but there is a mmap_assert_locked() during madvise_dontneed_free(),
> 
> Hmm, this is only in the THP PUD huge case, and MADV_FREE is only valid for
> anonymous memory, and I think only DAX can have some weird THP PUD case.
> 
> So I don't think we can hit this.

Yes, we don't support pud THP for anonymous pages.

> 
> In any event, I think this mmap_assert_locked() is mistaken, as we should
> only need a VMA lock here.
> 
> So we could replace with a:
> 
> 	if (!rwsem_is_locked(&tlb->mm->mmap_lock))
> 		vma_assert_locked(vma);
> 
> ?
> 

The pmd dax/anon split don't have assert, for PUD dax, we maybe remove 
this assert?




>>
>> madvise_dontneed_free
>>    madvise_dontneed_single_vma
>>      zap_page_range_single_batched
>>        unmap_single_vma
>>           unmap_page_range
>>             zap_pud_range
>>               mmap_assert_locked
>>
>> We could fix it by passing the lock_mode into zap_detial and then check
>> the right lock here, but I'm not sure whether it is safe to zap page
>> only with vma lock?
> 
> It's fine to zap with the VMA lock. You need only hold the VMA stable which
> a VMA lock achieves.
> 
> See https://docs.kernel.org/mm/process_addrs.html

Thanks, I will learn it.

> 
>>
>> And another about 4f8ba33bbdfc （"mm: madvise: use per_vma lock
>> for MADV_FREE"）, it called walk_page_range_vma() in
>> madvise_free_single_vma(),  but from link[1] and 5631da56c9a8
>> ("fs/proc/task_mmu: read proc/pid/maps under per-vma lock"), it saids
>>
>>    "Note that similar approach would not work for /proc/pid/smaps
>>    reading as it also walks the page table and that's not RCU-safe"
>>
>> We could use walk_page_range_vma() instead of walk_page_range() in
>> smap_gather_stats(), and same question, why 4f8ba33bbdfc(for MADV_FREEE)
>> is safe but not for show_numa_map()/show_smap()?
> 
> We only use walk_page_range() there in case 4 listed in show_smaps_rollup()
> where the mmap lock is dropped on contention.

Sorry, I mean the walk_page_range() in smap_gather_stats() called by
show_smap()  from /proc/pid/smaps, not the walk_page_range() in
show_smaps_rollup() from /proc/pid/smaps_rollup.


> 
>>
>> Thanks.
>>
>> [1] https://lkml.kernel.org/r/20250719182854.3166724-1-surenb@google.com
> 
> AFAICT That's referring to a previous approach that tried to walk
> /proc/$pid/swaps under RCU _alone_ without VMA locks. This is not safe as
> page tables can be yanked from under you not under RCU.

But for now it tries per-vma lock or fallback to mmap lock, not 
lockless, so do you mean we could try per-vma lock for 
/proc/pid/numa_maps or /proc/pid/smaps ?

> 
> Cheers, Lorenzo
>