From patchwork Tue Mar 27 11:02:58 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Will Deacon X-Patchwork-Id: 10309731 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 47AEE600F6 for ; Tue, 27 Mar 2018 11:02:50 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2842828173 for ; Tue, 27 Mar 2018 11:02:50 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 1CFCA286B0; Tue, 27 Mar 2018 11:02:50 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8304F28173 for ; Tue, 27 Mar 2018 11:02:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751120AbeC0LCs (ORCPT ); Tue, 27 Mar 2018 07:02:48 -0400 Received: from foss.arm.com ([217.140.101.70]:52838 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751030AbeC0LCs (ORCPT ); Tue, 27 Mar 2018 07:02:48 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DBEA480D; Tue, 27 Mar 2018 04:02:47 -0700 (PDT) Received: from edgewater-inn.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id ACA7D3F590; Tue, 27 Mar 2018 04:02:47 -0700 (PDT) Received: by edgewater-inn.cambridge.arm.com (Postfix, from userid 1000) id 709A21AE12D3; Tue, 27 Mar 2018 12:02:58 +0100 (BST) Date: Tue, 27 Mar 2018 12:02:58 +0100 From: Will Deacon To: Arnd Bergmann Cc: Benjamin Herrenschmidt , Jason Gunthorpe , Sinan Kaya , David Laight , Oliver , "open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)" , "linux-rdma@vger.kernel.org" , "Paul E. McKenney" , Peter Zijlstra , Ingo Molnar , Jonathan Corbet Subject: Re: RFC on writel and writel_relaxed Message-ID: <20180327110258.GF2464@arm.com> References: <1522101717.7364.14.camel@kernel.crashing.org> <20180326222756.GJ15554@ziepe.ca> <1522141019.7364.43.camel@kernel.crashing.org> <20180327095745.GB29373@arm.com> <20180327100944.GD29373@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Mar 27, 2018 at 12:53:49PM +0200, Arnd Bergmann wrote: > On Tue, Mar 27, 2018 at 12:09 PM, Will Deacon wrote: > > On Tue, Mar 27, 2018 at 12:05:06PM +0200, Arnd Bergmann wrote: > >> > - > >> > - See Documentation/DMA-API.txt for more information on consistent memory. > >> > + can see it now has ownership. Note that, when using writel(), a prior > >> > + wmb() is not needed to guarantee that the cache coherent memory writes > >> > + have completed before writing to the cache incoherent MMIO region. > >> > + If this ordering between incoherent MMIO and coherent memory regions > > One more thing: I think the term "incoherent MMIO" is a bit confusing, I'd > prefer just "MMIO" here. At least I don't have the faintest clue what the > difference between "coherent MMIO" and "incoherent MMIO" would be ;-) Yes, you're right. I was just following the terminology that's already used here, but actually that seems not be used anywhere else in the document! I'll kill it. > >> > + is not required, writel_relaxed() can be used instead and is significantly > >> > + cheaper on some weakly-ordered architectures. > >> > >> I think that's a great improvement, but I'm a bit worried about recommending > >> writel_relaxed() too much: I've seen a lot of drivers that just always use > >> writel_relaxed() over write(), and some of them get that wrong when they > >> don't understand the difference but end up using DMA without explicit > >> barriers anyway. > >> > >> Also, having an architecture-independent driver use wmb()+writel_relaxed() > >> ends up being more expensive than just using write(). Not sure how to > >> best phrase it though. > > > > Perhaps I add reword that with a simple example to say: > > > > If this ordering between incoherent MMIO and coherent memory regions > > is not required (e.g. in a sequence of accesses all to the MMIO region) > > [...] > > > > since that seems to be the usual case where the _relaxed accessors help. > > That still doesn't quite capture what I'd like driver writes to do: in essence > I would recommend them to use writel() all the time, except in performance > critical code that has been shown to be correct and has a comment to explain > why _relaxed() is ok in that particular function. > > Maybe it can just be rephrased to warn against the use of writel_relaxed() > here, and explain the difference that way: > > can see it now has ownership. Note that, when using writel(), a prior > wmb() is not needed to guarantee that the cache coherent memory writes > have completed before writing to the cache incoherent MMIO region. > The cheaper writel_relaxed() does not guarantee the DMA to be visible > to the device and must not be used here. Fair enough. I'd rather people used _relaxed by default, but I have to admit that it will probably just result in them getting things wrong. Just a tiny bit of wordsmithing brings this to: If you're happy with that, I'll send it as a proper patch. Cheers, Will --- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt index a863009849a3..3247547d1c36 100644 --- a/Documentation/memory-barriers.txt +++ b/Documentation/memory-barriers.txt @@ -1909,9 +1909,6 @@ There are some more advanced barrier functions: /* assign ownership */ desc->status = DEVICE_OWN; - /* force memory to sync before notifying device via MMIO */ - wmb(); - /* notify device of new descriptors */ writel(DESC_NOTIFY, doorbell); } @@ -1919,11 +1916,15 @@ There are some more advanced barrier functions: The dma_rmb() allows us guarantee the device has released ownership before we read the data from the descriptor, and the dma_wmb() allows us to guarantee the data is written to the descriptor before the device - can see it now has ownership. The wmb() is needed to guarantee that the - cache coherent memory writes have completed before attempting a write to - the cache incoherent MMIO region. - - See Documentation/DMA-API.txt for more information on consistent memory. + can see it now has ownership. Note that, when using writel(), a prior + wmb() is not needed to guarantee that the cache coherent memory writes + have completed before writing to the MMIO region. The cheaper + writel_relaxed() does not provide this guarantee and must not be used + here. + + See the subsection "Kernel I/O barrier effects" for more information on + relaxed I/O accessors and the Documentation/DMA-API.txt file for more + information on consistent memory. MMIO WRITE BARRIER