Received: by 2002:a25:ad19:0:0:0:0:0 with SMTP id y25csp2040842ybi; Thu, 18 Jul 2019 02:27:27 -0700 (PDT) X-Google-Smtp-Source: APXvYqwZkFKFLOPPHR8EwDzcixm3InFkDGIU0pxSqT696q7YfUwBngScSUdPYfc0qLIyZRlhw6iS X-Received: by 2002:a63:e44b:: with SMTP id i11mr209254pgk.297.1563442047626; Thu, 18 Jul 2019 02:27:27 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1563442047; cv=none; d=google.com; s=arc-20160816; b=dQaqYi3/WvKUM8a/bIennKniJQ65EmGvEyTQRJCPlKdf6hDyPQwH1eDRBL6t2+Pmfz YKcd3chPNERldSe2U+ZvnHtfyZ1rLHrIP8rQjcBdWyTqTngCcrP1ha3upf+DLVeeAc+e h1jy+Gyof/Hr6B97nVW4kQp/rDdqsX4gYaxhuakvLzXT8wOLpalVt4JwrKOmAYweiGdR oTs4afWCkYgBUmrHfJYFsD05qMPMb639HK8CGREL0hLjDaHZxyrg7ugc8JCvNh5iWXJX wIogmEVm4DgYMbUfmyl5yxLGk71zMnHPjmsqeb53v2OelXYf3U0Z6ql8sS83Cfmf0BUb s0Lw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=vML9IfOKzKHjJUuuMGLBkMBrTB4ZhyRxL/fItFVRXaw=; b=JJwfBLUMDeIBM/4yPXZv2tLnEWzCw24hCB5Pc4liUc1U9cWd3mZL8084lAQC4qd70t QrwfC8KzTgGteF6jnU1Tl2Ljy5hZL93o6dSdPSVsuu9ZWEnKAG1L4CdFO2hKkOO+6IY2 Nn4JnwsBoceLWPNVqH6TbZ6bFWdIkQ+NljoYKpHlWVkTel1VxP1NqsO3cBY47sV+9PwK SaBML27I0HVV8a62loeBTyNZnU4gMx+LSxP7l5fMB0Dq8CxdrpQH6naOEjh3w/twje3w PDqsp89mfLaIPHgNk0dWF/0xa8xqnIs00EkCt6qEK2ZelZYzRrF8qTvAMoSNzsUEXmvp 8Q9w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b="K6/iOWGG"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id x1si255284pgt.258.2019.07.18.02.27.11; Thu, 18 Jul 2019 02:27:27 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b="K6/iOWGG"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727617AbfGRJ0r (ORCPT + 99 others); Thu, 18 Jul 2019 05:26:47 -0400 Received: from mail.kernel.org ([198.145.29.99]:34792 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726397AbfGRJ0q (ORCPT ); Thu, 18 Jul 2019 05:26:46 -0400 Received: from willie-the-truck (236.31.169.217.in-addr.arpa [217.169.31.236]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 6765C20578; Thu, 18 Jul 2019 09:26:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1563442005; bh=J2dEcF1SlRL4s9DV6Ki0Uk/88KpAyOrkhzkzF15duRQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=K6/iOWGGIMlLIcWzqGNTeh7dpM3OA1bijDLUV5UV8gchEp9KpPa8zQGr9KKuhBC/W vql9RPR5Sq2jycovaCfrVjBORFjzQH6/HnClRRgrFOHXlrHXpVBpKAhxw6rwaHW8ua 1SGmjWdPNZgDmq8okKOT7arEcaT+AqjZfDZ0r0D8= Date: Thu, 18 Jul 2019 10:26:41 +0100 From: Will Deacon To: Jan Stancek Cc: Waiman Long , linux-kernel@vger.kernel.org, dbueso@suse.de, peterz@infradead.org, mingo@redhat.com, jade.alglave@arm.com, paulmck@linux.vnet.ibm.com Subject: Re: [PATCH v2] locking/rwsem: add acquire barrier to read_slowpath exit when queue is empty Message-ID: <20190718092640.52oliw3sid7gxyh6@willie-the-truck> References: <20190716185807.GJ3402@hirez.programming.kicks-ass.net> <20190717131335.b2ry43t2ov7ba4t4@willie-the-truck> <21ff5905-198b-6ea5-6c2a-9fb10cb48ea7@redhat.com> <20190717192200.GA17687@dustball.usersys.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190717192200.GA17687@dustball.usersys.redhat.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jan, Waiman, [+Jade and Paul for the litmus test at the end] On Wed, Jul 17, 2019 at 09:22:00PM +0200, Jan Stancek wrote: > On Wed, Jul 17, 2019 at 10:19:04AM -0400, Waiman Long wrote: > > > If you add a comment to the code outlining the issue (preferably as a litmus > > > test involving sem->count and some shared data which happens to be > > > vmacache_seqnum in your test)), then: > > > > > > Reviewed-by: Will Deacon > > > > > > Thanks, > > > > > > Will > > > > Agreed. A comment just above smp_acquire__after_ctrl_dep() on why this > > is needed will be great. > > > > Other than that, > > > > Acked-by: Waiman Long > > > > litmus test looks a bit long, would following be acceptable? > > diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c > index 37524a47f002..d9c96651bfc7 100644 > --- a/kernel/locking/rwsem.c > +++ b/kernel/locking/rwsem.c > @@ -1032,6 +1032,13 @@ static inline bool rwsem_reader_phase_trylock(struct rw_semaphore *sem, > */ > if (adjustment && !(atomic_long_read(&sem->count) & > (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))) { > + /* > + * down_read() issued ACQUIRE on enter, but we can race > + * with writer who did RELEASE only after us. > + * ACQUIRE here makes sure reader operations happen only > + * after all writer ones. > + */ How about an abridged form of the litmus test here, just to show the cod flow? e.g.: /* * We need to ensure ACQUIRE semantics when reading sem->count so that * we pair with the RELEASE store performed by an unlocking/downgrading * writer. * * P0 (writer) P1 (reader) * * down_write(sem); * * downgrade_write(sem); * -> fetch_add_release(&sem->count) * * down_read_slowpath(sem); * -> atomic_read(&sem->count) * * smp_acquire__after_ctrl_dep() * */ In writing this, I also noticed that we don't have any explicit ordering at the end of the reader slowpath when we wait on the queue but get woken immediately: if (!waiter.task) break; Am I missing something? > + smp_acquire__after_ctrl_dep(); > raw_spin_unlock_irq(&sem->wait_lock); > rwsem_set_reader_owned(sem); > lockevent_inc(rwsem_rlock_fast); > > > with litmus test in commit log: > ----------------------------------- 8< ------------------------------------ > C rwsem > > { > atomic_t rwsem_count = ATOMIC_INIT(1); > int vmacache_seqnum = 10; > } > > P0(int *vmacache_seqnum, atomic_t *rwsem_count) > { > r0 = READ_ONCE(*vmacache_seqnum); > WRITE_ONCE(*vmacache_seqnum, r0 + 1); > /* downgrade_write */ > r1 = atomic_fetch_add_release(-1+256, rwsem_count); > } > > P1(int *vmacache_seqnum, atomic_t *rwsem_count, spinlock_t *sem_wait_lock) > { > /* rwsem_read_trylock */ > r0 = atomic_add_return_acquire(256, rwsem_count); > /* rwsem_down_read_slowpath */ > spin_lock(sem_wait_lock); > r0 = atomic_read(rwsem_count); > if ((r0 & 1) == 0) { > // BUG: needs barrier > spin_unlock(sem_wait_lock); > r1 = READ_ONCE(*vmacache_seqnum); > } > } > exists (1:r1=10) > ----------------------------------- 8< ------------------------------------ Thanks for writing this! It's definitely worth sticking it in the commit log, but Paul and Jade might also like to include it as part of their litmus test repository too. Will