linux-kernel - Re: [PATCH-tip 1/3] locking/rwsem: Check wait

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: <438e0ad2-c263-679d-d369-be9750b2c646@redhat.com>
Date:   Mon, 27 Feb 2017 10:02:36 -0500
From:   Waiman Long <longman@...hat.com>
To:     Davidlohr Bueso <dave@...olabs.net>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH-tip 1/3] locking/rwsem: Check wait_list without lock if
 spinner present

On 02/26/2017 01:49 PM, Davidlohr Bueso wrote:
> On Wed, 22 Feb 2017, Waiman Long wrote:
>
>> We can safely check the wait_list to see if waiters are present without
>> lock when there are spinners to fall back on in case we miss a waiter.
>> The advantage is that we can save a pair of spin_lock/unlock calls
>> when the wait_list is empty. This translates to a reduction in latency
>> and hence slightly better performance.
>
> This benefit is only seen in (rare) situations where there are only
> writers with short hold times, no? I don't really have any objection
> as I doubt the additional load will have any impact on the common case,
> but it would still be nice to have more data for other benchmarks where
> the lock is at least shared at times -- ie: a good thing to measure is
> also fault, mmap related benchmarks.

If a up_write() or up_read() coincides with a writer attempting to lock
(down_write). The unlocker may go into the wake_rwsem path even if on
one is on the wait queue. In this case, this patch can save an unneeded
spin_lock/unlock. This was what happened in the microbenchmark that I used.

The additional load shouldn't have any noticeable performance impact as
the wait_list need to be read sooner or later anyway. BTW, can you
suggest a good benchmark for testing fault, mmap related code paths?

>> +        /*
>> +         * Normally checking wait_list without wait_lock isn't safe
>> +         * as we may miss an incoming waiter. With spinners present,
>> +         * however, we have someone to fall back on in case that
>> +         * happens. This can save a pair of spin_lock/unlock calls
>> +         * when there is no waiter.
>> +         */
>
> I would drop the last part regarding saving the spin_lock, it should be
> evident from the code. 

I will do that.

Cheers,
Longman