Synchronization around mmap_changing in userfaultfd

Synchronization around mmap_changing in userfaultfd

[Lokesh Gidra]

Hi,

We have been seeing mmap_lock contention issues while using
userfaultfd for GC in Android. But now that per-vma locks are being
used in the kernel, we were hoping to use it in userfaultfd code to
pin the VMA in COPY/MOVE/ZEROPAGE etc. operations. But while going
through the code, I noticed that mmap_changing is implicitly protected
by mmap_lock:

1) All increments to it (except for userfault_remove) are done with
mmap_lock in write-mode
2) All reads (in copy/move/zeropage etc) are done with mmap_lock in read-mode

I wanted to understand if that's just out of convenience, and
therefore would it be ok to introduce a read-write semaphore in
userfaultfd_ctx to achieve the same synchronization:

1) All increments are done with this semaphore in write-mode
2) All operations (copy/move/zeropage etc) are done within the
critical section of this semaphore in read-mode and checking that
mmap_changing is 0.

If this is wrong, then kindly explain why mmap_changing needs to be
protected with mmap_lock.


Thanks,
Lokesh

Re: Synchronization around mmap_changing in userfaultfd

[Mike Rapoport]

Hi,

On Thu, Jan 11, 2024 at 03:32:20PM -0800, Lokesh Gidra wrote:
> Hi,
> 
> We have been seeing mmap_lock contention issues while using
> userfaultfd for GC in Android. But now that per-vma locks are being
> used in the kernel, we were hoping to use it in userfaultfd code to
> pin the VMA in COPY/MOVE/ZEROPAGE etc. operations. But while going
> through the code, I noticed that mmap_changing is implicitly protected
> by mmap_lock:
> 
> 1) All increments to it (except for userfault_remove) are done with
> mmap_lock in write-mode
> 2) All reads (in copy/move/zeropage etc) are done with mmap_lock in read-mode
> 
> I wanted to understand if that's just out of convenience, and
> therefore would it be ok to introduce a read-write semaphore in
> userfaultfd_ctx to achieve the same synchronization:
> 
> 1) All increments are done with this semaphore in write-mode
> 2) All operations (copy/move/zeropage etc) are done within the
> critical section of this semaphore in read-mode and checking that
> mmap_changing is 0.

mmap_changing was added to the existing critical sections that were already
protected with mmap_lock, so I didn't see a reason for additional lock to
protect mmap_changing.
 
With per-vma locks, your proposal makes perfect sense to me.

> If this is wrong, then kindly explain why mmap_changing needs to be
> protected with mmap_lock.
> 
> 
> Thanks,
> Lokesh
> 

-- 
Sincerely yours,
Mike.

Re: Synchronization around mmap_changing in userfaultfd

[Lokesh Gidra]

On Thu, Jan 25, 2024 at 1:19 AM Mike Rapoport <rppt@kernel.org> wrote:
>
> Hi,
>
> On Thu, Jan 11, 2024 at 03:32:20PM -0800, Lokesh Gidra wrote:
> > Hi,
> >
> > We have been seeing mmap_lock contention issues while using
> > userfaultfd for GC in Android. But now that per-vma locks are being
> > used in the kernel, we were hoping to use it in userfaultfd code to
> > pin the VMA in COPY/MOVE/ZEROPAGE etc. operations. But while going
> > through the code, I noticed that mmap_changing is implicitly protected
> > by mmap_lock:
> >
> > 1) All increments to it (except for userfault_remove) are done with
> > mmap_lock in write-mode
> > 2) All reads (in copy/move/zeropage etc) are done with mmap_lock in read-mode
> >
> > I wanted to understand if that's just out of convenience, and
> > therefore would it be ok to introduce a read-write semaphore in
> > userfaultfd_ctx to achieve the same synchronization:
> >
> > 1) All increments are done with this semaphore in write-mode
> > 2) All operations (copy/move/zeropage etc) are done within the
> > critical section of this semaphore in read-mode and checking that
> > mmap_changing is 0.
>
> mmap_changing was added to the existing critical sections that were already
> protected with mmap_lock, so I didn't see a reason for additional lock to
> protect mmap_changing.
>
> With per-vma locks, your proposal makes perfect sense to me.

Thanks so much for confirming. I'll send the patches for review very soon.
>
> > If this is wrong, then kindly explain why mmap_changing needs to be
> > protected with mmap_lock.
> >
> >
> > Thanks,
> > Lokesh
> >
>
> --
> Sincerely yours,
> Mike.