From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id BD368C04A6A for ; Thu, 27 Jul 2023 15:08:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234425AbjG0PIQ (ORCPT ); Thu, 27 Jul 2023 11:08:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54228 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234546AbjG0PIE (ORCPT ); Thu, 27 Jul 2023 11:08:04 -0400 Received: from casper.infradead.org (casper.infradead.org [IPv6:2001:8b0:10b:1236::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1C08DE47 for ; Thu, 27 Jul 2023 08:07:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=oNHK2MbbJ6j1a4TfO0kZ5fTh+NDwsDl1r4u8RwZS2vw=; b=vZOntyDsid8d/Ee8U4oTNIWUM1 qsAeBSbwte9KQNffQQN/iam3+iftr7Cp3gBnWSnZi4KsFsbEQ2+1UoVQV0WL/cEhtLcpLPz00ZYLw G3yoWRFdrF8SNl2e4fq6a6ooSY3bbpJqTukS9pBo1DJ8wtH+7OaBn6cpG6zokRUtXwZoK0WP50w98 qP1b7tK+3Y9D98NbI/davY8tTF11caaPgW4bjd7gjHof7tiHwqJSb+mKCfCRubAqzS/u8skVPC7cH Cw0PZcY+VJ78Tb9nk6/qI8PUrwsPzWZVOuXZpNZv686Ne8+SZxCJospPNuuhQfBw9eJT/U9sfulOM b71xg4Kw==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1qP2aS-007a4H-Uu; Thu, 27 Jul 2023 15:07:33 +0000 Date: Thu, 27 Jul 2023 16:07:32 +0100 From: Matthew Wilcox To: Jann Horn Cc: paulmck@kernel.org, Andrew Morton , Linus Torvalds , Peter Zijlstra , Suren Baghdasaryan , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Alan Stern , Andrea Parri , Will Deacon , Boqun Feng , Nicholas Piggin , David Howells , Jade Alglave , Luc Maranget , Akira Yokosawa , Daniel Lustig , Joel Fernandes Subject: Re: [PATCH 0/2] fix vma->anon_vma check for per-VMA locking; fix anon_vma memory ordering Message-ID: References: <20230726214103.3261108-1-jannh@google.com> <31df93bd-4862-432c-8135-5595ffd2bd43@paulmck-laptop> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 27, 2023 at 04:39:34PM +0200, Jann Horn wrote: > Assume that we are holding some kind of lock that ensures that the > only possible concurrent update to "vma->anon_vma" is that it changes > from a NULL pointer to a non-NULL pointer (using smp_store_release()). > > > if (READ_ONCE(vma->anon_vma) != NULL) { > // we now know that vma->anon_vma cannot change anymore > > // access the same memory location again with a plain load > struct anon_vma *a = vma->anon_vma; > > // this needs to be address-dependency-ordered against one of > // the loads from vma->anon_vma > struct anon_vma *root = a->root; > } > > > Is this fine? If it is not fine just because the compiler might > reorder the plain load of vma->anon_vma before the READ_ONCE() load, > would it be fine after adding a barrier() directly after the > READ_ONCE()? > > I initially suggested using READ_ONCE() for this, and then Linus and > me tried to reason it out and Linus suggested (if I understood him > correctly) that you could make the ugly argument that this works > because loads from the same location will not be reordered by the > hardware. So on anything other than alpha, we'd still have the > required address-dependency ordering because that happens for all > loads, even plain loads, while on alpha, the READ_ONCE() includes a > memory barrier. But that argument is weirdly reliant on > architecture-specific implementation details. > > The other option is to replace the READ_ONCE() with a > smp_load_acquire(), at which point it becomes a lot simpler to show > that the code is correct. Aren't we straining at gnats here? The context of this is handling a page fault, and we used to take an entire rwsem for read. I'm having a hard time caring about "the extra expense" of an unnecessarily broad barrier. Cost of an L3 cacheline miss is in the thousands of cycles. Cost of a barrier is ... tens?