From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753465AbcEMR6M (ORCPT ); Fri, 13 May 2016 13:58:12 -0400 Received: from mail-pa0-f43.google.com ([209.85.220.43]:35590 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753363AbcEMR6I (ORCPT ); Fri, 13 May 2016 13:58:08 -0400 Subject: Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field To: Peter Zijlstra References: <1462580424-40333-1-git-send-email-Waiman.Long@hpe.com> <5733AC64.6020306@hurleysoftware.com> <20160513150749.GT3192@twins.programming.kicks-ass.net> Cc: Waiman Long , Ingo Molnar , linux-kernel@vger.kernel.org, Davidlohr Bueso , Jason Low , Dave Chinner , Scott J Norton , Douglas Hatch From: Peter Hurley Message-ID: <573615AD.60300@hurleysoftware.com> Date: Fri, 13 May 2016 10:58:05 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160513150749.GT3192@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/13/2016 08:07 AM, Peter Zijlstra wrote: > On Wed, May 11, 2016 at 03:04:20PM -0700, Peter Hurley wrote: >>> + return !rwsem_is_reader_owned(READ_ONCE(sem->owner)); >> >> It doesn't make sense to force reload sem->owner here; if sem->owner >> is not being reloaded then the loop above will execute forever. >> >> Arguably, this check should be bumped out to the optimistic spin and >> reload/check the owner there? >> > > Note that barrier() and READ_ONCE() have overlapping but not identical > results and the combined use actually makes sense here. > > Yes, a barrier() anywhere in the loop will force a reload of the > variable, _however_ it doesn't force that reload to not suffer from > load tearing. > > Using volatile also forces a reload, but also ensures the load cannot > be torn IFF it is of machine word side and naturally aligned. > > So while the READ_ONCE() here is pointless for forcing the reload; > that's already ensured, we still need to make sure the load isn't torn. If load tearing a naturally aligned pointer is a real code generation possibility then the rcu list code is broken too (which loads ->next directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()). For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc that had to do with control dependencies and not load tearing. OTOH, this patch might actually produce store-tearing: +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 != RWSEM_READER_OWNED) + sem->owner = RWSEM_READER_OWNED; +} Regards, Peter Hurley