Re: [QUESTION] mmap lock in khugepaged

[Dev Jain]

+the list and Zi Yan.

On 28/11/24 5:04 pm, David Hildenbrand wrote:
> > > Maybe, we'd have to do the isolation+copy under the PMD lock. And
> > > currently, we have to drop the PMD lock in order to have the
> > > pte_offset_map_lock() work IIRC.
> >
> > Is there a problem in holding two page table locks simultaneously?
>
> Depending on CONFIG_SPLIT_PTE_PTLOCKS, it might not even be two locks 
> (I assume we could have such configs with khugepaged).
>
> Not sure if there could be an issue with lock inversion.
>
> So I suspect this no not be 100% trivial :)
>
> > > Most importantly, the copy that currently runs under no spinlocks
> > > would now run under spinlock. Up to 512 MiB on arm64 64K, not sure if
> > > that can be a problem ... we currently seem to take care of that
> >
> > But we already are taking mmap_write_lock(), so that should not matter?
>
> We are dealing with a spinlock vs. a rwsem.
>
> We usually want to avoid holding spinlocks for an excessive amount of 
> time, because all other CPUs waiting for that lock will ... spin with 
> preemption disabled instead of rescheduling and doing something useful.

Ah okay.


> Further, without CONFIG_SPLIT_PMD_PTLOCKS, in fact everybody who wnats 
> to take a PMD lock in that process would be spinning on the same PMD 
> lock :)
>
>
> > I mean, if we can get rid of the mmap exclusive lock, then the copying
> > would still be a bottleneck, and all fault handlers will back off, but
>
> I'm trying to digest it once again, but I'm afraid I don't see how 
> fault handlers will back off.
>
> Won't they either see pmd_none(), to then either call pte_alloc_one() 
> where they would spin on the PMD lock, or try allocating a PMD THP to 
> insert it, and then spin on the PMD lock, to figure out later that it 
> was all in vain?

Ya that's what I meant when I said "back off", sorry I wasn't clear, I 
didn't mean VM_FAULT_FALLBACK/RETRY or something...yes, the work will be 
in vain and the MMU will refault...
> Thinking about it, I am also not sure if most other code can deal with 
> temporary pmd_none(). These don't necessarily take the PMD lock, 
> because "why would they" right now.
>
> See walk_pmd_range() as one example, if it spots pmd_none() it assumes 
> "there really is nothing" without checking the PMD lock.
>
> As a more concrete example, assume someone calls MADV_DONTNEED and we 
> end up in zap_pmd_range(). If we assume "pmd_none() == really nothing" 
> we'll skip that entry without getting the PMD lock involved. That 
> would mean that you would be breaking MADV_DONTNEED if you managed to 
> collapse or not -- memory would not get discarded.
>
> This is a real problem with anonymous memory.
Yes, I thought of different locking fashions but the problem seems to be 
that any pagetable walker will choose an action path according to the value
it sees.

> Unless I am missing something it's all very tricky and there might be 
> a lot of such code that assumes "if I hold a mmap lock / VMA lock in 
> read mode, pmd_none() means there is nothing even without holding the 
> PMD lock when checking".

Yes, I was betting on the fact that, if the code assumes that pmd_none() 
means there is nothing, eventually when it will take the PMD lock to 
write to it, it will check whether
the PMD changed, and back off. I wasn't aware of the MADV_DONTNEED 
thingy, thanks.
> > at least processes will be able to mmap() and do stuff with their VMAs,
> > and I would guess that this is worth optimizing...
>
> It would certainly be interesting to get rid of the mmap lock in write 
> mode here, but it's all rater tricky (and the code has rather nasty 
> hidden implications).
>
> > > pte = pte_offset_map_lock(mm, &_pmd, address, &pte_ptl);
> > > if (pte) {
> > >      ...
> > >      spun_unlock(pte);
> > > } ...
> > >
> > > result = __collapse_huge_page_copy(...);
> > > pte_unmap(pte);
> > >
> > >
> > > Deep in __collapse_huge_page_copy() we seem to re-rake the PTL lock.
> > > No-split-spinlock confiogs might be problematic ...
> >
> > Could you elaborate a little? I haven't read about the older config...
>
> See above regarding CONFIG_SPLIT_PTE_PTLOCKS and friends.
>
> > > I recall that for shmem that's "easier", because we don't have to
> > > reinstall a PMD immediately, we cna be sure that the page table is
> > > kept empty/unmodified, ...

All in all, the general solution seems to be that, if I can take all 
pagetable walkers into an invalid state and make them backoff, then I am 
safe.
For example, we do not zero out the PMD, we take the pte PTL, we do 
stuff, then instead of setting the PTEs to zero, we set it to a universal
invalid state upon which no pagetable walker can take an action; an 
instance of that can be to set the PTE to a swap entry such that the fault
handler ends up in do_swap_page() ->print_bad_pte(). So now we can take 
the PMD lock (in fact we don't need it since any pagetable walking
is rendered useless) and populate the PMD to resume the new pagetable 
walking. Another *ridiculous* idea may be to remember the PGD we
came from and nuke it (but I guess there is an alternate path for that 
in walk_pgd_range() and so on?)


>