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: <20220316224548.500123-1-namhyung@kernel.org> <20220316224548.500123-3-namhyung@kernel.org>
 <365529974.156362.1647524728813.JavaMail.zimbra@efficios.com>
In-Reply-To: <365529974.156362.1647524728813.JavaMail.zimbra@efficios.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Fri, 18 Mar 2022 14:34:53 -0700
Message-ID: <CAM9d7chFVp6SPGoZPJF6+CMkbZyp1Fmxxu2Xn3Ks=DYcgbUG5w@mail.gmail.com>
Subject: Re: [PATCH 2/2] locking: Apply contention tracepoints in the slow path
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>, Will Deacon <will@kernel.org>,
        Waiman Long <longman@redhat.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        rostedt <rostedt@goodmis.org>,
        Byungchul Park <byungchul.park@lge.com>,
        paulmck <paulmck@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Radoslaw Burny <rburny@google.com>,
        linux-arch <linux-arch@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Mar 17, 2022 at 6:45 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Mar 16, 2022, at 6:45 PM, Namhyung Kim namhyung@kernel.org wrote:
>
> > Adding the lock contention tracepoints in various lock function slow
> > paths.  Note that each arch can define spinlock differently, I only
> > added it only to the generic qspinlock for now.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > kernel/locking/mutex.c        |  3 +++
> > kernel/locking/percpu-rwsem.c |  3 +++
> > kernel/locking/qrwlock.c      |  9 +++++++++
> > kernel/locking/qspinlock.c    |  5 +++++
> > kernel/locking/rtmutex.c      | 11 +++++++++++
> > kernel/locking/rwbase_rt.c    |  3 +++
> > kernel/locking/rwsem.c        |  9 +++++++++
> > kernel/locking/semaphore.c    | 14 +++++++++++++-
> > 8 files changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> > index ee2fd7614a93..c88deda77cf2 100644
> > --- a/kernel/locking/mutex.c
> > +++ b/kernel/locking/mutex.c
> > @@ -644,6 +644,7 @@ __mutex_lock_common(struct mutex *lock, unsigned int state,
> > unsigned int subclas
> >       }
> >
> >       set_current_state(state);
> > +     trace_contention_begin(lock, 0);
>
> This should be LCB_F_SPIN rather than the hardcoded 0.

I don't think so.  LCB_F_SPIN is for spin locks indicating that
it's spinning on a cpu.  And the value is not 0.

>
> >       for (;;) {
> >               bool first;
> >
> > @@ -710,6 +711,7 @@ __mutex_lock_common(struct mutex *lock, unsigned int state,
> > unsigned int subclas
> > skip_wait:
> >       /* got the lock - cleanup and rejoice! */
> >       lock_acquired(&lock->dep_map, ip);
> > +     trace_contention_end(lock, 0);
> >
> >       if (ww_ctx)
> >               ww_mutex_lock_acquired(ww, ww_ctx);
> > @@ -721,6 +723,7 @@ __mutex_lock_common(struct mutex *lock, unsigned int state,
> > unsigned int subclas
> > err:
> >       __set_current_state(TASK_RUNNING);
> >       __mutex_remove_waiter(lock, &waiter);
> > +     trace_contention_end(lock, ret);
> > err_early_kill:
> >       raw_spin_unlock(&lock->wait_lock);
> >       debug_mutex_free_waiter(&waiter);
> > diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
> > index c9fdae94e098..833043613af6 100644
> > --- a/kernel/locking/percpu-rwsem.c
> > +++ b/kernel/locking/percpu-rwsem.c
> > @@ -9,6 +9,7 @@
> > #include <linux/sched/task.h>
> > #include <linux/sched/debug.h>
> > #include <linux/errno.h>
> > +#include <trace/events/lock.h>
> >
> > int __percpu_init_rwsem(struct percpu_rw_semaphore *sem,
> >                       const char *name, struct lock_class_key *key)
> > @@ -154,6 +155,7 @@ static void percpu_rwsem_wait(struct percpu_rw_semaphore
> > *sem, bool reader)
> >       }
> >       spin_unlock_irq(&sem->waiters.lock);
> >
> > +     trace_contention_begin(sem, LCB_F_PERCPU | (reader ? LCB_F_READ :
> > LCB_F_WRITE));
> >       while (wait) {
> >               set_current_state(TASK_UNINTERRUPTIBLE);
> >               if (!smp_load_acquire(&wq_entry.private))
> > @@ -161,6 +163,7 @@ static void percpu_rwsem_wait(struct percpu_rw_semaphore
> > *sem, bool reader)
> >               schedule();
> >       }
> >       __set_current_state(TASK_RUNNING);
> > +     trace_contention_end(sem, 0);
>
> So for the reader-write locks, and percpu rwlocks, the "trace contention end" will always
> have ret=0. Likewise for qspinlock, qrwlock, and rtlock. It seems to be a waste of trace
> buffer space to always have space for a return value that is always 0.

Right, I think it'd be better to have a new tracepoint for the error cases
and get rid of the return value in the contention_end.

Like contention_error or contention_return ?

>
> Sorry if I missed prior dicussions of that topic, but why introduce this single
> "trace contention begin/end" muxer tracepoint with flags rather than per-locking-type
> tracepoint ? The per-locking-type tracepoint could be tuned to only have the fields
> that are needed for each locking type.

No prior discussions on that topic and thanks for bringing it out.

Having per-locking-type tracepoints will help if you want to filter
out specific types of locks efficiently.  Otherwise it'd be simpler
for users to have a single set of tracepoints to handle all locking
types like the existing lockdep tracepoints do.

As it's in a contended path, I think it's allowed to be a little bit
less efficient and the flags can tell which type of locks it's tracing
so you can filter it out anyway.

Thanks,
Namhyung