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

[Davidlohr Bueso]

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@xxxxxxxxxxxx> 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.

> >         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.

Thanks,
Davidlohr