linux-kernel - Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field

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: <57314650.5050206@hpe.com>
Date:	Mon, 9 May 2016 22:24:16 -0400
From:	Waiman Long <waiman.long@....com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	Ingo Molnar <mingo@...hat.com>, <linux-kernel@...r.kernel.org>,
	Davidlohr Bueso <dave@...olabs.net>,
	Jason Low <jason.low2@...com>,
	Dave Chinner <david@...morbit.com>,
	Scott J Norton <scott.norton@....com>,
	Douglas Hatch <doug.hatch@....com>
Subject: Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner
 field

On 05/09/2016 04:27 AM, Peter Zijlstra wrote:
> On Fri, May 06, 2016 at 08:20:24PM -0400, Waiman Long wrote:
>> @@ -391,9 +386,11 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
>>   		 * When there's no owner, we might have preempted between the
>>   		 * owner acquiring the lock and setting the owner field. If
>>   		 * we're an RT task that will live-lock because we won't let
>> +		 * the owner complete. We also quit if the lock is owned by
>> +		 * readers.
> Maybe also note why we quit on readers.

Sure. Will do so.

>>   		*/
>> +		if (rwsem_is_reader_owned(owner) ||
>> +		   (!owner&&  (need_resched() || rt_task(current))))
>>   			break;
>>
>>   		/*
>
>> diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h
>> index 870ed9a..d7fea18 100644
>> --- a/kernel/locking/rwsem.h
>> +++ b/kernel/locking/rwsem.h
>> @@ -1,3 +1,20 @@
>> +/*
>> + * The owner field of the rw_semaphore structure will be set to
>> + * RWSEM_READ_OWNED when a reader grabs the lock. A writer will clear
>> + * the owner field when it unlocks. A reader, on the other hand, will
>> + * not touch the owner field when it unlocks.
>> + *
>> + * In essence, the owner field now has the following 3 states:
>> + *  1) 0
>> + *     - lock is free or the owner hasn't set the field yet
>> + *  2) RWSEM_READER_OWNED
>> + *     - lock is currently or previously owned by readers (lock is free
>> + *       or not set by owner yet)
>> + *  3) Other non-zero value
>> + *     - a writer owns the lock
>> + */
>> +#define RWSEM_READER_OWNED	1UL
> #define RWSEM_READER_OWNED	((struct task_struct *)1UL)

Will make the change.

>> +
>>   #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
>>   static inline void rwsem_set_owner(struct rw_semaphore *sem)
>>   {
>> @@ -9,6 +26,26 @@ static inline void rwsem_clear_owner(struct rw_semaphore *sem)
>>   	sem->owner = NULL;
>>   }
>>
>> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem)
>> +{
>> +	/*
>> +	 * We check the owner value first to make sure that we will only
>> +	 * do a write to the rwsem cacheline when it is really necessary
>> +	 * to minimize cacheline contention.
>> +	 */
>> +	if (sem->owner != (struct task_struct *)RWSEM_READER_OWNED)
>> +		sem->owner = (struct task_struct *)RWSEM_READER_OWNED;
> How much if anything did this optimization matter?

I hadn't run any performance test to verify the effective of this 
change. For a reader-heavy rwsem, this change should be able to save 
quite a lot of needless write to the rwsem cacheline.

>> +}
>> +
>> +static inline bool rwsem_is_writer_owned(struct task_struct *owner)
>> +{
>> +	return (unsigned long)owner>  RWSEM_READER_OWNED;
>> +}
> Tad too clever that; what does GCC generate if you write the obvious:
>
> 	return owner&&  owner != RWSEM_READER_OWNER;

You are right. GCC is intelligent enough to make the necessary 
optimization. I will revert it to this form which is more obvious.

>> +
>> +static inline bool rwsem_is_reader_owned(struct task_struct *owner)
>> +{
>> +	return owner == (struct task_struct *)RWSEM_READER_OWNED;
>> +}
> So I don't particularly like these names; they read like they take a
> rwsem as argument, but they don't.
>
> Would something like: rwsem_owner_is_{reader,writer}() make more sense?

Yes, these names look good to me.

Cheers,
Longman