linux-kernel - Re: [PATCH 21/21] binder: switch alloc->mutex to spinlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 1 Dec 2023 07:46:31 +0000
From:   Carlos Llamas <cmllamas@...gle.com>
To:     Alice Ryhl <aliceryhl@...gle.com>
Cc:     Arve Hjønnevåg <arve@...roid.com>,
        Christian Brauner <brauner@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        kernel-team@...roid.com, linux-kernel@...r.kernel.org,
        Martijn Coenen <maco@...roid.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Todd Kjos <tkjos@...roid.com>
Subject: Re: [PATCH 21/21] binder: switch alloc->mutex to spinlock_t

On Tue, Nov 07, 2023 at 09:08:49AM +0000, Alice Ryhl wrote:
> Carlos Llamas <cmllamas@...gle.com> writes:
> > The alloc->mutex is a highly contended lock that causes performance
> > issues on Android devices. When a low-priority task is given this lock
> > and it sleeps, it becomes difficult for the task to wakeup and complete
> > its work. This delays other tasks that are also waiting on the mutex.
> 
> Grammar nit: "to wake up"

OK.

> 
> > The problem gets worse when there is memory pressure in the system,
> > because this increases the contention on the alloc->mutex while the
> > shrinker reclaims binder pages.
> > 
> > Switching to a spinlock helps to keep the waiters running and avoids the
> > overhead of waking up tasks. This significantly improves the transaction
> > latency when the problematic scenario occurs.
> >
> > [snip]
> >
> > Note that it is only possible to convert this lock after a series of
> > changes made by previous patches. These mainly include refactoring the
> > sections that might_sleep() and changing the locking order with the
> > mmap_lock amongst others.
> > 
> > Signed-off-by: Carlos Llamas <cmllamas@...gle.com>
> 
> Nice!
> 
> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
> 
> >  	 * binder_free_buf_locked(). However, that could
> > -	 * increase contention for the alloc mutex if clear_on_free
> > -	 * is used frequently for large buffers. The mutex is not
> > +	 * increase contention for the alloc->lock if clear_on_free
> > +	 * is used frequently for large buffers. This lock is not
> 
> Grammar nit: Shouldn't this say "However, that could increase contention
> on alloc->lock if clear_on_free is used frequently for large buffers."?

Do you mean "contention for" vs "contention on"? They both seem correct
to me but this was written by Todd, so I'd trust his english much more
than mine.

--
Carlos Llamas