Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 29 Apr 2022 11:45:13 -0700
From:   Davidlohr Bueso <dave@stgolabs.net>
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Waiman Long <longman@redhat.com>,
        Will Deacon <will@kernel.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Ingo Molnar <mingo@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] locking/rwsem: Teach contention tracing about optimistic
 spinning
Message-ID: <20220429184513.vftv4pj72fxzrpm2@offworld>
References: <20220427155241.26410-1-dave@stgolabs.net>
 <CAM9d7ciQnUcefR-hLBHSDhu6zZovxXvGYW-CdSBiub_GAOfAiA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <CAM9d7ciQnUcefR-hLBHSDhu6zZovxXvGYW-CdSBiub_GAOfAiA@mail.gmail.com>
User-Agent: NeoMutt/20220408
Precedence: bulk

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.

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