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

[Lorenzo Stoakes]

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.

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);

?

>
> 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

>
> 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.

>
> Thanks.
>
> [1] https://lkml.kernel.org/r/20250719182854.3166724-1-surenb@xxxxxxxxxx

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.

Cheers, Lorenzo