From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753089AbbKLOkE (ORCPT ); Thu, 12 Nov 2015 09:40:04 -0500 Received: from e36.co.us.ibm.com ([32.97.110.154]:47550 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751043AbbKLOkC (ORCPT ); Thu, 12 Nov 2015 09:40:02 -0500 X-IBM-Helo: d03dlp01.boulder.ibm.com X-IBM-MailFrom: paulmck@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Thu, 12 Nov 2015 06:40:04 -0800 From: "Paul E. McKenney" To: Oleg Nesterov Cc: Boqun Feng , Peter Zijlstra , mingo@kernel.org, linux-kernel@vger.kernel.org, corbet@lwn.net, mhocko@kernel.org, dhowells@redhat.com, torvalds@linux-foundation.org, will.deacon@arm.com, Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras Subject: Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() Message-ID: <20151112144004.GU3972@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <20151102132901.157178466@infradead.org> <20151102134941.005198372@infradead.org> <20151103175958.GA4800@redhat.com> <20151111093939.GA6314@fixme-laptop.cn.ibm.com> <20151111121232.GN17308@twins.programming.kicks-ass.net> <20151111193953.GA23515@redhat.com> <20151112070915.GC6314@fixme-laptop.cn.ibm.com> <20151112150058.GA30321@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151112150058.GA30321@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15111214-0021-0000-0000-00001472A480 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 12, 2015 at 04:00:58PM +0100, Oleg Nesterov wrote: > On 11/12, Boqun Feng wrote: > > > > On Wed, Nov 11, 2015 at 08:39:53PM +0100, Oleg Nesterov wrote: > > > > > > object_t *object; > > > spinlock_t lock; > > > > > > void update(void) > > > { > > > object_t *o; > > > > > > spin_lock(&lock); > > > o = READ_ONCE(object); > > > if (o) { > > > BUG_ON(o->dead); > > > do_something(o); > > > } > > > spin_unlock(&lock); > > > } > > > > > > void destroy(void) // can be called only once, can't race with itself > > > { > > > object_t *o; > > > > > > o = object; > > > object = NULL; > > > > > > /* > > > * pairs with lock/ACQUIRE. The next update() must see > > > * object == NULL after spin_lock(); > > > */ > > > smp_mb(); > > > > > > spin_unlock_wait(&lock); > > > > > > /* > > > * pairs with unlock/RELEASE. The previous update() has > > > * already passed BUG_ON(o->dead). > > > * > > > * (Yes, yes, in this particular case it is not needed, > > > * we can rely on the control dependency). > > > */ > > > smp_mb(); > > > > > > o->dead = true; > > > } > > > > > > I believe the code above is correct and it needs the barriers on both sides. > > > > > > > Hmm.. probably incorrect.. because the ACQUIRE semantics of spin_lock() > > only guarantees that the memory operations following spin_lock() can't > > be reorder before the *LOAD* part of spin_lock() not the *STORE* part, > > i.e. the case below can happen(assuming the spin_lock() is implemented > > as ll/sc loop) > > > > spin_lock(&lock): > > r1 = *lock; // LL, r1 == 0 > > o = READ_ONCE(object); // could be reordered here. > > *lock = 1; // SC > > > > This could happen because of the ACQUIRE semantics of spin_lock(), and > > the current implementation of spin_lock() on PPC allows this happen. > > > > (Cc PPC maintainers for their opinions on this one) > > In this case the code above is obviously wrong. And I do not understand > how we can rely on spin_unlock_wait() then. > > And afaics do_exit() is buggy too then, see below. > > > I think it's OK for it as an ACQUIRE(with a proper barrier) or even just > > a control dependency to pair with spin_unlock(), for example, the > > following snippet in do_exit() is OK, except the smp_mb() is redundant, > > unless I'm missing something subtle: > > > > /* > > * The setting of TASK_RUNNING by try_to_wake_up() may be delayed > > * when the following two conditions become true. > > * - There is race condition of mmap_sem (It is acquired by > > * exit_mm()), and > > * - SMI occurs before setting TASK_RUNINNG. > > * (or hypervisor of virtual machine switches to other guest) > > * As a result, we may become TASK_RUNNING after becoming TASK_DEAD > > * > > * To avoid it, we have to wait for releasing tsk->pi_lock which > > * is held by try_to_wake_up() > > */ > > smp_mb(); > > raw_spin_unlock_wait(&tsk->pi_lock); > > Perhaps it is me who missed something. But I don't think we can remove > this mb(). And at the same time it can't help on PPC if I understand > your explanation above correctly. I cannot resist suggesting that any lock that interacts with spin_unlock_wait() must have all relevant acquisitions followed by smp_mb__after_unlock_lock(). Thanx, Paul