Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Fri, 1 Dec 2023 07:46:31 +0000
From:   Carlos Llamas <cmllamas@google.com>
To:     Alice Ryhl <aliceryhl@google.com>
Cc:     Arve =?iso-8859-1?B?SGr4bm5lduVn?= <arve@android.com>,
        Christian Brauner <brauner@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Joel Fernandes <joel@joelfernandes.org>,
        kernel-team@android.com, linux-kernel@vger.kernel.org,
        Martijn Coenen <maco@android.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Todd Kjos <tkjos@android.com>
Subject: Re: [PATCH 21/21] binder: switch alloc->mutex to spinlock_t
Message-ID: <ZWmPV-N7Ua795HQ9@google.com>
References: <20231102185934.773885-22-cmllamas@google.com>
 <20231107090849.262070-1-aliceryhl@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231107090849.262070-1-aliceryhl@google.com>
Precedence: bulk

On Tue, Nov 07, 2023 at 09:08:49AM +0000, Alice Ryhl wrote:
> Carlos Llamas <cmllamas@google.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@google.com>
> 
> Nice!
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.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