Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks

[Peter Xu]

On Thu, Apr 11, 2024 at 03:50:54PM +0100, Matthew Wilcox wrote:
> On Wed, Apr 10, 2024 at 08:20:04PM -0400, Peter Xu wrote:
> > On Thu, Apr 11, 2024 at 12:59:09AM +0100, Matthew Wilcox wrote:
> > > On Wed, Apr 10, 2024 at 05:23:18PM -0400, Peter Xu wrote:
> > > > On Wed, Apr 10, 2024 at 10:10:45PM +0100, Matthew Wilcox wrote:
> > > > > > I can do some tests later today or tomorrow. Any suggestion you have on
> > > > > > amplifying such effect that you have concern with?
> > > > > 
> > > > > 8 socket NUMA system, 800MB text segment, 10,000 threads.  No, I'm not
> > > > > joking, that's a real customer workload.
> > > > 
> > > > Well, I believe you, but even with this, that's a total of 800MB memory on
> > > > a giant moster system... probably just to fault in once.
> > > > 
> > > > And even before we talk about that into details.. we're talking about such
> > > > giant program running acorss hundreds of cores with hundreds of MB text,
> > > > then... hasn't the program developer already considered mlockall() at the
> > > > entry of the program?  Wouldn't that greatly beneficial already with
> > > > whatever granule of locks that a future fault would take?
> > > 
> > > I don't care what your theory is, or even what your benchmarking shows.
> > > I had basically the inverse of this patch, and my customer's workload
> > > showed significant improvement as a result.  Data talks, bullshit walks.
> > > Your patch is NAKed and will remain NAKed.
> > 
> > Either would you tell me your workload, I may try it.
> > 
> > Or, please explain why it helps?  If such huge library is in a single VMA,
> > I don't see why per-vma lock is better than mmap lock.  If the text is
> > combined with multiple vmas, it should only help when each core faults at
> > least on different vmas, not the same.
> 
> Oh, you really don't understand.  The mmap_lock is catastrophically
> overloaded.  Before the per-VMA lock, every page fault took it for read,
> and every call to mmap() took it for write.  Because our rwsems are
> fair, once one thread has called mmap() it waits for all existing page
> faults to complete _and_ blocks all page faults from starting until
> it has completed.  That's a huge source of unexpected latency for any
> multithreaded application.
> 
> Anything we can do to avoid taking the mmap_sem, even for read, helps any
> multithreaded workload.  Your suggestion that "this is rare, it doesn't
> matter" shows that you don't get it.  That you haven't found a workload
> where you can measure it shows that your testing is inadequate.
> 
> Yes, there's added complexity with the per-VMA locks.  But we need it for
> good performance.  Throwing away performance on a very small reduction
> in complexity is a terrible trade-off.

Yes, this is a technical discussion, and such comments are what I'm looking
for.  Thank you.

What I am not sure so far is that what you worried on a performance degrade
for "this small corner case" doesn't exist.

Let's first check when that vmf_anon_prepare() lines are introduced:

commit 17c05f18e54158a3eed0c22c85b7a756b63dcc01
Author: Suren Baghdasaryan <surenb@xxxxxxxxxx>
Date:   Mon Feb 27 09:36:25 2023 -0800

    mm: prevent do_swap_page from handling page faults under VMA lock

It didn't seem like a plan to do late anon_vma check for any performance
reasons.

To figure these out, let me ask some more questions.

1) When you said "you ran a customer workload, and that greatly improved
   performance", are you comparing between:

   - with/without file-typed per-vma lock, or,
   - with/without this patch?

   Note that I'm hopefully not touching that fact that file per-vma should
   work like before for the majority.  And I'm surprised to see your
   comment because I didn't expect this is even measured before.

   To ask in another way: do you mean that it's your intention to check
   anon_vma late for private file mappings when working on file support on
   per-vma lock?

   If the answer is yes, I'd say please provide some document patch to
   support such behavior, you can stop my patch from getting merged now,
   but it's never clear whether someone else will see this and post it
   again.  If it wasn't obviously to Suren who introduced per-vma lock [1],
   then I won't be surprised it's unknown to most developers on the list.

2) What happens if lock_vma_under_rcu() keeps spreading its usage?

   Now it's already spread over to the uffd world.  Uffd has that special
   check to make sure file private mappings are fine in lock_vma().

   When it keeps going like that, I won't be surprised to see future users
   of lock_vma_under_rcu() forget the private file mappings and it can
   cause hard to debug issues.  Even if lock_vma_under_rcu() is exactly
   what you're looking for, I think we may need lock_vma_under_rcu_fault(),
   then lock_vma_under_rcu() should cover private file mappings to make
   sure future users don't expose easily to hard to debug issues.

[1] https://lore.kernel.org/r/CAJuCfpGj5xk-NxSwW6Mt8NGZcV9N__8zVPMGXDPAYKMcN9=Oig@xxxxxxxxxxxxxx

Thanks,

-- 
Peter Xu