Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230601055846.2349566-1-jstultz@google.com> <20230601055846.2349566-3-jstultz@google.com>
 <509d5ee4-45ec-1279-97da-a308ec7f51aa@arm.com>
In-Reply-To: <509d5ee4-45ec-1279-97da-a308ec7f51aa@arm.com>
From:   John Stultz <jstultz@google.com>
Date:   Fri, 23 Jun 2023 17:41:08 -0700
Message-ID: <CANDhNCr0NiCB-+h3pQtY9SG+Tm1_01eRpJVhhF75u_djFiEcQQ@mail.gmail.com>
Subject: Re: [PATCH v4 02/13] locking/ww_mutex: Remove wakeups from under mutex::wait_lock
To:     Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Joel Fernandes <joelaf@google.com>,
        Qais Yousef <qyousef@google.com>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Valentin Schneider <vschneid@redhat.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>,
        Zimuzo Ezeozue <zezeozue@google.com>,
        Youssef Esmat <youssefesmat@google.com>,
        Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Will Deacon <will@kernel.org>,
        Waiman Long <longman@redhat.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        "Paul E . McKenney" <paulmck@kernel.org>, kernel-team@android.com,
        "Connor O'Brien" <connoro@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 23, 2023 at 5:21=E2=80=AFAM Dietmar Eggemann
<dietmar.eggemann@arm.com> wrote:
>
> Hi John,
>
> On 01/06/2023 07:58, John Stultz wrote:
> > From: Peter Zijlstra <peterz@infradead.org>
> >
> > In preparation to nest mutex::wait_lock under rq::lock we need to remov=
e
> > wakeups from under it.
>
> [...]
>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Signed-off-by: Connor O'Brien <connoro@google.com>
> > Signed-off-by: John Stultz <jstultz@google.com>
> > ---
> > v2:
> > * Move wake_q_init() as suggested by Waiman Long
> > ---
> >  include/linux/ww_mutex.h  |  3 +++
> >  kernel/locking/mutex.c    |  8 ++++++++
> >  kernel/locking/ww_mutex.h | 10 ++++++++--
> >  3 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> > index bb763085479a..9335b2202017 100644
> > --- a/include/linux/ww_mutex.h
> > +++ b/include/linux/ww_mutex.h
> > @@ -19,6 +19,7 @@
> >
> >  #include <linux/mutex.h>
> >  #include <linux/rtmutex.h>
> > +#include <linux/sched/wake_q.h>
> >
> >  #if defined(CONFIG_DEBUG_MUTEXES) || \
> >     (defined(CONFIG_PREEMPT_RT) && defined(CONFIG_DEBUG_RT_MUTEXES))
> > @@ -58,6 +59,7 @@ struct ww_acquire_ctx {
> >       unsigned int acquired;
> >       unsigned short wounded;
> >       unsigned short is_wait_die;
> > +     struct wake_q_head wake_q;
>
> you told me that there is already an issue in this patch even w/o PE
> when running `insmod /lib/modules/test-ww_mutex.ko`.
>
> The issue is related to Connor's version (1):
>
> https://lkml.kernel.org/r/20221003214501.2050087-2-connoro@google.com
>
>   struct ww_acquire_ctx {
>
>           struct wake_q_head wake_q;
>
>
>   __mutex_lock_common()
>
>     if (ww_ctx)
>         ww_ctx_wake(ww_ctx)
>
>           wake_up_q(&ww_ctx->wake_q);
>           wake_q_init(&ww_ctx->wake_q);
>
>
> Juri's version (2):
>
> https://lkml.kernel.org/r/20181009092434.26221-3-juri.lelli@redhat.com
>
>   __mutex_lock_common()
>
>       DEFINE_WAKE_Q(wake_q) <-- !!!
>
>       __ww_mutex_check_waiters(..., wake_q)
>
>         __ww_mutex_die(..., wake_q)
>
>           wake_q_add(wake_q, waiter->task)
>
>       wake_up_q(&wake_q)
>
>
> `insmod /lib/modules/test-ww_mutex.ko` runs fine with (2) but not with
> (1) (both w/o the remaining PE patches).
>
> So to test the PE issues we talked about already which come with `[PATCH
> v4 09/13] sched: Add proxy execution` and CONFIG_PROXY_EXEC=3Dy we need t=
o
> fix (1) or go back to (2).
>
> I haven't found any clues why (2) was changed to (1) so far.

Right. I don't have context for why, but moving the wake_q to the
ww_ctx does seem to break the wake_q assumptions, and results in lost
wakeups.

In my current tree, I've switched back to Juri's older version of the
patch, but adding one fix from Connor's, and with that the patch
doesn't run into this trouble.

That said, I still am catching and debugging problems with later
patches in the series, which has required breaking up the core proxy
change into much finer grained changes so I can debug what's going
wrong.  My v5 patch set will reflect this.

thanks
-john