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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220429184513.vftv4pj72fxzrpm2@offworld>
Date:   Fri, 29 Apr 2022 11:45:13 -0700
From:   Davidlohr Bueso <dave@...olabs.net>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Waiman Long <longman@...hat.com>,
        Will Deacon <will@...nel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Ingo Molnar <mingo@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] locking/rwsem: Teach contention tracing about optimistic
 spinning

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