linux-kernel - Re: [PATCH v4 08/16] locking/rwsem: Make rwsem_spin_on

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]

Date:   Wed, 17 Apr 2019 14:29:02 -0400
From:   Waiman Long <longman@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...hat.com>, Will Deacon <will.deacon@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Davidlohr Bueso <dave@...olabs.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        huang ying <huang.ying.caritas@...il.com>
Subject: Re: [PATCH v4 08/16] locking/rwsem: Make rwsem_spin_on_owner() return
 owner state

On 04/17/2019 08:47 AM, Peter Zijlstra wrote:
> On Wed, Apr 17, 2019 at 02:41:01PM +0200, Peter Zijlstra wrote:
>> On Sat, Apr 13, 2019 at 01:22:51PM -0400, Waiman Long wrote:
>>> In the special case that there is no active lock and the handoff bit
>>> is set, optimistic spinning has to be stopped.
>>> @@ -500,9 +521,19 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
>>>  
>>>  	/*
>>>  	 * If there is a new owner or the owner is not set, we continue
>>> -	 * spinning.
>>> +	 * spinning except when here is no active locks and the handoff bit
>>> +	 * is set. In this case, we have to stop spinning.
>>>  	 */
>>> -	return is_rwsem_owner_spinnable(READ_ONCE(sem->owner));
>>> +	owner = READ_ONCE(sem->owner);
>>> +	if (!is_rwsem_owner_spinnable(owner))
>>> +		return OWNER_NONSPINNABLE;
>>> +	if (owner && !is_rwsem_owner_reader(owner))
>>> +		return OWNER_WRITER;
>>> +
>>> +	count = atomic_long_read(&sem->count);
>>> +	if (RWSEM_COUNT_HANDOFF(count) && !RWSEM_COUNT_LOCKED(count))
>>> +		return OWNER_NONSPINNABLE;
>>> +	return !owner ? OWNER_NULL : OWNER_READER;
>>>  }
>> So this fixes a straight up bug in the last patch (and thus should be
>> done before so the bug never exists), and creates unreadable code while
>> at it.
>>
>> Also, I think only checking HANDOFF after the loop is wrong; the moment
>> HANDOFF happens you have to terminate the loop, irrespective of what
>> @owner does.
>>
>> Does something like so work?
>>
>> ---
>>
>> enum owner_state {
>> 	OWNER_NULL		= 1 << 0,
>> 	OWNER_WRITER		= 1 << 1,
>> 	OWNER_READER		= 1 << 2,
>> 	OWNER_NONSPINNABLE	= 1 << 3,
>> };
>> #define OWNER_SPINNABLE		(OWNER_NULL | OWNER_WRITER)
> Hmm, we should not spin on OWNER_NULL. Or at least not mixed in with the
> patch that changes the shape of all this. That should go in the RT
> thingy patch, which comes after this.

We do spin on OWNER_NULL right now, not in rwsem_spin_on_owner() but in
the main rwsem_optimistic_spin() function.

RT task will quit if owner is NULL.

Cheers,
Longman