Re: [PATCH] locking/rwsem: Teach contention tracing about optimistic spinning

[Namhyung Kim <namhyung@kernel.org>]

Hi Davidlohr,

On Fri, Apr 29, 2022 at 11:56 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> Sorry for the late reply.
>
> On Wed, 27 Apr 2022, Namhyung Kim wrote:
>
> >Hi Davidlohr,
> >
> >On Wed, Apr 27, 2022 at 9:04 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
> >>
> >> Similar to the mutex counterpart, we can further distinguish the
> >> types of contention. Similarly this patch also introduces potentially
> >> various _begin() tracepoints with a single respective _end().
> >
> >Thanks for doing this.  I was thinking about it as well.
>
> I really like your work on this. I've always wanted a low overhead
> equivalent-ish of lock_stat, which could be used in production and
> look forward to see these tracepoints put to good use.
>
> >> @@ -115,7 +116,8 @@ TRACE_EVENT(contention_begin,
> >>                                 { LCB_F_WRITE,          "WRITE" },
> >>                                 { LCB_F_RT,             "RT" },
> >>                                 { LCB_F_PERCPU,         "PERCPU" },
> >> -                               { LCB_F_MUTEX,          "MUTEX" }
> >> +                               { LCB_F_MUTEX,          "MUTEX" },
> >> +                               { LCB_F_RWSEM,          "RWSEM" }
> >>                           ))
> >>  );
> >
> >Well I'm ok with this but it'd be better if we can do this
> >without adding a new flag.  Originally a mutex can be
> >identified with 0, and a rwsem with either of READ or WRITE.
> >
> >The MUTEX flag was added to note optimistic spins
> >on mutex and now we need something similar for
> >rwsem.  Then can we change the MUTEX to OPTIMISTIC
> >if it's not too late?
>
> Ok. Perhaps name it OSQ? I had thought of that but at the
> time was also thinking about potentially the rtmutex and
> rt spinlock spinning too - which don't use osq so the name
> would fit in that sense.
>
> While not in Linus' tree, I wouldn't think it's too late.

Any updates?  It's now in Linus' tree so we should change
this before the official release.  Or we can keep the current
flags and then add one like in your original code.


>
> >>         for (;;) {
> >>                 if (rwsem_try_write_lock(sem, &waiter)) {
> >> @@ -1161,18 +1167,25 @@ rwsem_down_write_slowpath(struct rw_semaphore *sem, int state)
> >>                 if (waiter.handoff_set) {
> >>                         enum owner_state owner_state;
> >>
> >> +                       trace_contention_begin(sem, LCB_F_RWSEM |
> >> +                                              LCB_F_WRITE | LCB_F_SPIN);
> >>                         preempt_disable();
> >>                         owner_state = rwsem_spin_on_owner(sem);
> >>                         preempt_enable();
> >>
> >> -                       if (owner_state == OWNER_NULL)
> >> -                               goto trylock_again;
> >> +                       if (owner_state == OWNER_NULL) {
> >> +                               raw_spin_lock_irq(&sem->wait_lock);
> >> +                               if (rwsem_try_write_lock(sem, &waiter))
> >> +                                       break;
> >> +                               raw_spin_unlock_irq(&sem->wait_lock);
> >> +                       }
> >> +
> >> +                       trace_contention_begin(sem, LCB_F_RWSEM | LCB_F_WRITE);
> >
> >I'm afraid that it'd generate many contention_begin
> >trace events for a single lock acquisition.
>
> You are right, lets just trace the "normal" optimistic spinning
> at the start of the write slowpath.

I have to admit that I overlooked the mutex code already
has the same logic.  I still prefer having less number of
events but you might want to have the same with the
mutex for the precise tracking of the spins.

Thanks,
Namhyung