Re: [PATCH v3 2/8] binder: concurrent page installation

[Carlos Llamas <cmllamas@google.com>]

On Fri, Nov 08, 2024 at 03:41:41PM -0500, Liam R. Howlett wrote:
> * Carlos Llamas <cmllamas@google.com> [241108 14:11]:
> > Allow multiple callers to install pages simultaneously by downgrading
> > the mmap_sem to non-exclusive mode.
> 
> Since we actually allow downgrading of a lock, it would be more clear to
> say that you are changing from a read lock to a write lock.  I was
> searching for the lock downgrade in this patch :)

ah, I can see how the wording here can be misleading. I'll rephrase that
to avoid confusion.

> > -	ret = vm_insert_page(alloc->vma, addr, page);
> > -	if (ret) {
> > +	mmap_read_lock(alloc->mm);
> 
> Ah, I debate bring this up, but you can do this without the mmap read
> lock.  You can use rcu and the per-vma locking like in the page fault
> path.  If you look at how that is done using the lock_vma_under_rcu()
> (mm/memory.c) then you can see that pages are installed today without
> the mmap locking (on some architectures - which includes x86_64 and
> arm64).

Right, per-vma locking is implemented in patch 7 of this series:
https://lore.kernel.org/all/20241108191057.3288442-8-cmllamas@google.com/

> userfaultfd had to do something like this as well, and created some
> abstraction to facilitate either mmap or rcu, based on the arch support.
> Going to rcu really helped performance there [1].  There was also a
> chance of the vma being missed, so it is checked again under the mmap
> read lock, but realistically that never happens and exists to ensure
> correctness.

hmm, if there are more users of this pattern in the future then perhaps
it might be worth adding a common helper?

> You also mention the shrinker and using the alloc->mutex, well zapping
> pages can also be done under the vma specific lock - if that helps?
> 
> Freeing page tables is different though, that needs more locking, but I
> don't think this is an issue for you.
> 
> [1]. https://lore.kernel.org/all/20240215182756.3448972-1-lokeshgidra@google.com/

ha, I was not aware of this. I'll have a look thanks.

> 
> > +	vma = vma_lookup(alloc->mm, addr);
> 
> Thank you for not saving a pointer anymore.

lol, thanks to you for initiating this effort!

Cheers,
Carlos Llamas