linux-kernel - Re: [PATCH] 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: <1462384355.3933.26.camel@j-VirtualBox>
Date:	Wed, 04 May 2016 10:52:35 -0700
From:	Jason Low <jason.low2@...com>
To:	Waiman Long <waiman.long@....com>
Cc:	Davidlohr Bueso <dave@...olabs.net>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	Scott J Norton <scott.norton@....com>,
	Douglas Hatch <doug.hatch@....com>, jason.low2@....com
Subject: Re: [PATCH] locking/rwsem: Add reader owned state to the owner field

On Wed, 2016-05-04 at 13:27 -0400, Waiman Long wrote:
> On 05/03/2016 08:21 PM, Davidlohr Bueso wrote:
> > On Wed, 27 Apr 2016, Waiman Long wrote:
> >> static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
> >> @@ -378,7 +367,8 @@ static bool rwsem_optimistic_spin(struct 
> >> rw_semaphore *sem)
> >>
> >>     while (true) {
> >>         owner = READ_ONCE(sem->owner);
> >> -        if (owner && !rwsem_spin_on_owner(sem, owner))
> >> +        if (rwsem_is_writer_owned(owner) &&
> >> +           !rwsem_spin_on_owner(sem, owner))
> >>             break;
> >>
> >>         /* wait_lock will be acquired if write_lock is obtained */
> >> @@ -391,9 +381,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.
> >> +         * the owner complete. We also quit if the lock is owned by
> >> +         * readers.
> >>          */
> >> -        if (!owner && (need_resched() || rt_task(current)))
> >> +        if ((owner == RWSEM_READER_OWNED) ||

It would be good to provide and use a rwsem_is_reader_owned() function
like we do with rwsem_is_writer_owned(), especially if we're going to
add in the additional cast.

> >> #ifdef CONFIG_RWSEM_SPIN_ON_OWNER
> >> static inline void rwsem_set_owner(struct rw_semaphore *sem)
> >> {
> >> @@ -9,6 +26,16 @@ static inline void rwsem_clear_owner(struct 
> >> rw_semaphore *sem)
> >>     sem->owner = NULL;
> >> }
> >>
> >> +static inline void rwsem_reader_owned(struct rw_semaphore *sem)
> >
> > Nits: rwsem_set_reader_owner()?
> 
> How about rwsem_set_reder_owned()? reader_owner kind of looks weird to me.

I agree that rwsem_set_reader_owned() is a better name than
rwsem_set_reader_owner() since that matches the RWSEM_READER_OWNED
naming convention.

> >
> >> +{
> >> +    if (sem->owner != RWSEM_READER_OWNED)
> >> +        sem->owner = RWSEM_READER_OWNED;
> >
> > ... and just blindly setting it ough to be fine.
> 
> I was trying to minimize the number of writes to the rwsem cacheline so 
> as to reduce the amount of cacheline contention. I am not sure if the 
> CPU will be smart enough to discard the write if the cacheline has 
> already contained the value to be written.

It would be good to keep that check to avoid unnecessary cacheline
contention if sem->owner is already set to RWSEM_READER_OWNED.