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 X-Spam-Level: X-Spam-Status: No, score=-5.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6C7FACA9EB9 for ; Wed, 23 Oct 2019 09:09:05 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 06FAC20659 for ; Wed, 23 Oct 2019 09:09:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="MOj/ylau" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 06FAC20659 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 7A3756B0003; Wed, 23 Oct 2019 05:09:04 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 754216B0006; Wed, 23 Oct 2019 05:09:04 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6426E6B0007; Wed, 23 Oct 2019 05:09:04 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0048.hostedemail.com [216.40.44.48]) by kanga.kvack.org (Postfix) with ESMTP id 414386B0003 for ; Wed, 23 Oct 2019 05:09:04 -0400 (EDT) Received: from smtpin10.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with SMTP id B567B98BB for ; Wed, 23 Oct 2019 09:09:03 +0000 (UTC) X-FDA: 76074474966.10.land89_6c1e167ca5221 X-HE-Tag: land89_6c1e167ca5221 X-Filterd-Recvd-Size: 7934 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by imf26.hostedemail.com (Postfix) with ESMTP for ; Wed, 23 Oct 2019 09:09:02 +0000 (UTC) Received: by mail-wr1-f68.google.com with SMTP id o28so21168248wro.7 for ; Wed, 23 Oct 2019 02:09:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=sender:date:from:to:cc:subject:message-id:mail-followup-to :references:mime-version:content-disposition:in-reply-to:user-agent; bh=I3CdxrHaNyHyRZEG1FuNTtm7TdLvAqPyeGgl25NH314=; b=MOj/ylauwCVvu9XlWustyHzaSLBCLB0r8SxcpbGt/WNTTA+W3Eilry/hQ3LwrBxwyx gPOiRymXCK2dsCqO5dOhpdZklU2lV9U4UJv9kLSbljZH7tQDvZUYhNl9OO0bjpWmAYUq m7WRQS+woMs4MAqmlMSC3ZrjaNArF8FIbkVWU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to:user-agent; bh=I3CdxrHaNyHyRZEG1FuNTtm7TdLvAqPyeGgl25NH314=; b=optat1fXbLLtZ0CRWq4Myut7xns6OjIMrUilsy7VZqz4RvS9q0HQXMrjZEJE3cD3Dz CS2BEiDnrDJddplkbGD3NgcdG6ZbWG5Xh+u3X6qRmUOvfgVdZqm3AZ3xrLtkuTo0Jv9h 0WwvW/ImcQz/68/KLeQMxX9iGlAstiBZPCfqxBeECWPHdNwyYsaeXavSnXPV2DZd/TCh Y8DdxtqHBxGVo//6ZOnQNXy3a6sj4LFP0XX6LnxijUrAE+Jez1C5Iml4npmvNIsN6Iez sRdpG3GsU/xP1GTvy8u3CfjivXM8wwOgRRFgzJ/ldcpWVJ5FFXK4B5RYqzYD3hyMZc9G Eb/Q== X-Gm-Message-State: APjAAAUnkm3p2PjafBXze/BaLp/LaTSsRjap5b3RmprNBClrL5sBNCoC aYtkwit9FNaguMlAHxiMqwo7jg== X-Google-Smtp-Source: APXvYqxBlWiMbV68z/4WCdyBIn/eYixlaxzfRL2TT2uj5JcOqKaRJpFzViyC4G1k4Zqgafht+vBoZw== X-Received: by 2002:adf:f192:: with SMTP id h18mr7782727wro.148.1571821741491; Wed, 23 Oct 2019 02:09:01 -0700 (PDT) Received: from phenom.ffwll.local (212-51-149-96.fiber7.init7.net. [212.51.149.96]) by smtp.gmail.com with ESMTPSA id a189sm7456296wma.2.2019.10.23.02.09.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Oct 2019 02:09:00 -0700 (PDT) Date: Wed, 23 Oct 2019 11:08:58 +0200 From: Daniel Vetter To: Jason Gunthorpe Cc: "Koenig, Christian" , Andrea Arcangeli , "Yang, Philip" , Ralph Campbell , "linux-rdma@vger.kernel.org" , John Hubbard , "Kuehling, Felix" , "amd-gfx@lists.freedesktop.org" , "linux-mm@kvack.org" , Jerome Glisse , "dri-devel@lists.freedesktop.org" , Ben Skeggs Subject: Re: [PATCH hmm 00/15] Consolidate the mmu notifier interval_tree and locking Message-ID: <20191023090858.GV11828@phenom.ffwll.local> Mail-Followup-To: Jason Gunthorpe , "Koenig, Christian" , Andrea Arcangeli , "Yang, Philip" , Ralph Campbell , "linux-rdma@vger.kernel.org" , John Hubbard , "Kuehling, Felix" , "amd-gfx@lists.freedesktop.org" , "linux-mm@kvack.org" , Jerome Glisse , "dri-devel@lists.freedesktop.org" , Ben Skeggs References: <2df298e2-ee91-ef40-5da9-2bc1af3a17be@gmail.com> <2046e0b4-ba05-0683-5804-e9bbf903658d@amd.com> <20191018203608.GA5670@mellanox.com> <20191021135744.GA25164@mellanox.com> <20191021151221.GC25164@mellanox.com> <20191022075735.GV11828@phenom.ffwll.local> <20191022150109.GF22766@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191022150109.GF22766@mellanox.com> X-Operating-System: Linux phenom 5.2.0-2-amd64 User-Agent: Mutt/1.10.1 (2018-07-13) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000003, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Oct 22, 2019 at 03:01:13PM +0000, Jason Gunthorpe wrote: > On Tue, Oct 22, 2019 at 09:57:35AM +0200, Daniel Vetter wrote: > > > > The unusual bit in all of this is using a lock's critical region to > > > 'protect' data for read, but updating that same data before the lock's > > > critical secion. ie relying on the unlock barrier to 'release' program > > > ordered stores done before the lock's own critical region, and the > > > lock side barrier to 'acquire' those stores. > > > > I think this unusual use of locks as barriers for other unlocked accesses > > deserves comments even more than just normal barriers. Can you pls add > > them? I think the design seeems sound ... > > > > Also the comment on the driver's lock hopefully prevents driver > > maintainers from moving the driver_lock around in a way that would very > > subtle break the scheme, so I think having the acquire barrier commented > > in each place would be really good. > > There is already a lot of documentation, I think it would be helpful > if you could suggest some specific places where you think an addition > would help? I think the perspective of someone less familiar with this > design would really improve the documentation Hm I just meant the usual recommendation that "barriers must have comments explaining what they order, and where the other side of the barrier is". Using unlock/lock as a barrier imo just makes that an even better idea. Usually what I do is something like "we need to order $this against $that below, and the other side of this barrier is in function()." With maybe a bit more if it's not obvious how things go wrong if the orderin is broken. Ofc seqlock.h itself skimps on that rule and doesn't bother explaining its barriers :-/ > I've been tempted to force the driver to store the seq number directly > under the driver lock - this makes the scheme much clearer, ie > something like this: > > diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c > index 712c99918551bc..738fa670dcfb19 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_svm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c > @@ -488,7 +488,8 @@ struct svm_notifier { > }; > > static bool nouveau_svm_range_invalidate(struct mmu_range_notifier *mrn, > - const struct mmu_notifier_range *range) > + const struct mmu_notifier_range *range, > + unsigned long seq) > { > struct svm_notifier *sn = > container_of(mrn, struct svm_notifier, notifier); > @@ -504,6 +505,7 @@ static bool nouveau_svm_range_invalidate(struct mmu_range_notifier *mrn, > mutex_lock(&sn->svmm->mutex); > else if (!mutex_trylock(&sn->svmm->mutex)) > return false; > + mmu_range_notifier_update_seq(mrn, seq); > mutex_unlock(&sn->svmm->mutex); > return true; > } > > > At the cost of making the driver a bit more complex, what do you > think? Hm, spinning this further ... could we initialize the mmu range notifier with a pointer to the driver lock, so that we could put a lockdep_assert_held into mmu_range_notifier_update_seq? I think that would make this scheme substantially more driver-hacker proof :-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch