From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932485AbcI3Tcj (ORCPT ); Fri, 30 Sep 2016 15:32:39 -0400 Received: from foss.arm.com ([217.140.101.70]:49062 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751076AbcI3Tci (ORCPT ); Fri, 30 Sep 2016 15:32:38 -0400 Date: Fri, 30 Sep 2016 20:32:32 +0100 From: Mark Rutland To: Brent DeGraaf Cc: Peter Zijlstra , Ingo Molnar , Catalin Marinas , Will Deacon , Timur Tabi , Nathan Lynch , linux-kernel@vger.kernel.org, Christopher Covington , linux-arm-kernel@lists.infradead.org Subject: Re: [RFC] arm64: Enforce observed order for spinlock and data Message-ID: <20160930193231.GA2521@remoulade> References: <1475257257-23072-1-git-send-email-bdegraaf@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1475257257-23072-1-git-send-email-bdegraaf@codeaurora.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 30, 2016 at 01:40:57PM -0400, Brent DeGraaf wrote: > Prior spinlock code solely used load-acquire and store-release > semantics to ensure ordering of the spinlock lock and the area it > protects. However, store-release semantics and ordinary stores do > not protect against accesses to the protected area being observed > prior to the access that locks the lock itself. > > While the load-acquire and store-release ordering is sufficient > when the spinlock routines themselves are strictly used, other > kernel code that references the lock values directly (e.g. lockrefs) > could observe changes to the area protected by the spinlock prior > to observance of the lock itself being in a locked state, despite > the fact that the spinlock logic itself is correct. If the spinlock logic is correct, why are we changing that, and not the lockref code that you say has a problem? What exactly goes wrong in the lockref code? Can you give a concrete example? Why does the lockref code accesses lock-protected fields without taking the lock first? Wouldn't concurrent modification be a problem regardless? > + /* > + * Yes: The store done on this cpu was the one that locked the lock. > + * Store-release one-way barrier on LL/SC means that accesses coming > + * after this could be reordered into the critical section of the I assume you meant s/store-release/load-acquire/ here. This does not make sense to me otherwise. > + * load-acquire/store-release, where we did not own the lock. On LSE, > + * even the one-way barrier of the store-release semantics is missing, Likewise (for the LSE case description). > + * so LSE needs an explicit barrier here as well. Without this, the > + * changed contents of the area protected by the spinlock could be > + * observed prior to the lock. > + */ By whom? We generally expect that if data is protected by a lock, you take the lock before accessing it. If you expect concurrent lockless readers, then there's a requirement on the writer side to explicitly provide the ordering it requires -- spinlocks are not expected to provide that. So, why aren't those observers taking the lock? What pattern of accesses are made by readers and writers such that there is a problem? What does this result in? > +" dmb ish\n" > +" b 3f\n" > +"4:\n" > /* > * No: spin on the owner. Send a local event to avoid missing an > * unlock before the exclusive load. > @@ -116,7 +129,15 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) > " ldaxrh %w2, %4\n" > " eor %w1, %w2, %w0, lsr #16\n" > " cbnz %w1, 2b\n" > - /* We got the lock. Critical section starts here. */ > + /* > + * We got the lock and have observed the prior owner's store-release. > + * In this case, the one-way barrier of the prior owner that we > + * observed combined with the one-way barrier of our load-acquire is > + * enough to ensure accesses to the protected area coming after this > + * are not accessed until we own the lock. In this case, other > + * observers will not see our changes prior to observing the lock > + * itself. Critical locked section starts here. > + */ Each of these comments ends up covers, and their repeated presence makes the code harder to read. If there's a common problem, note it once at the top of the file. Thanks, Mark.