On Thu, Mar 17, 2022 at 6:45 AM Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>
> ----- On Mar 16, 2022, at 6:45 PM, Namhyung Kim namhyung@xxxxxxxxxx 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@xxxxxxxxxx>
> > ---
> > 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
