From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754846AbcEDRyy (ORCPT ); Wed, 4 May 2016 13:54:54 -0400 Received: from g1t6214.austin.hp.com ([15.73.96.122]:35446 "EHLO g1t6214.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753251AbcEDRyw (ORCPT ); Wed, 4 May 2016 13:54:52 -0400 Message-ID: <1462384355.3933.26.camel@j-VirtualBox> Subject: Re: [PATCH] locking/rwsem: Add reader owned state to the owner field From: Jason Low To: Waiman Long Cc: Davidlohr Bueso , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, Scott J Norton , Douglas Hatch , jason.low2@hpe.com Date: Wed, 04 May 2016 10:52:35 -0700 In-Reply-To: <572A30EB.7050804@hpe.com> References: <1461781737-11209-1-git-send-email-Waiman.Long@hpe.com> <20160504002136.GA10179@linux-uzut.site> <572A30EB.7050804@hpe.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.