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.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,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 ED44FCA9EB7 for ; Wed, 23 Oct 2019 09:32:21 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A16E821D71 for ; Wed, 23 Oct 2019 09:32:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="rVZ4tg4I" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A16E821D71 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 570596B0266; Wed, 23 Oct 2019 05:32:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 546F66B026C; Wed, 23 Oct 2019 05:32:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4363F6B026D; Wed, 23 Oct 2019 05:32:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0043.hostedemail.com [216.40.44.43]) by kanga.kvack.org (Postfix) with ESMTP id 23E9F6B0266 for ; Wed, 23 Oct 2019 05:32:21 -0400 (EDT) Received: from smtpin05.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with SMTP id AB652180AD838 for ; Wed, 23 Oct 2019 09:32:20 +0000 (UTC) X-FDA: 76074533640.05.soap72_146116f628b46 X-HE-Tag: soap72_146116f628b46 X-Filterd-Recvd-Size: 8098 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by imf40.hostedemail.com (Postfix) with ESMTP for ; Wed, 23 Oct 2019 09:32:19 +0000 (UTC) Received: by mail-wm1-f65.google.com with SMTP id v6so1931823wmj.0 for ; Wed, 23 Oct 2019 02:32:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=reply-to:subject:to:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding:content-language; bh=EwzJlitg6ojpTgAu/CD7TkQAOGgugVP58FFyv2RoWtw=; b=rVZ4tg4I22VQyUArQiihwBc10IhXwEssWDf1EaFeaIXqsGC6JfdVQ3m8a/+eFez/uv cBI5tmVKmRe5xwC2hsqoyWkcMnEwVvJMzguUqKhiylze81YYpUjxlJGmahgktbdYGMF3 7stesmV/VO9FsVwh0xa64s1wOLimS1Yr1zQaK49meIOcg2FOzdWsFrqVWuNkJZMA9ler J6m58AAFZyFWE0TN2mMV8z2LWcUCEKPX/BZHda4GtmP9hHo+s6q4FeIosOmGcNkqUI4R 06UI7jOr184MX3RX6QI8HFxtm2ezvjUTDRQxyx419bdvk6QI/kKmCCrZJGIzpz5QWD0Q 1T9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:reply-to:subject:to:references:from:message-id :date:user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=EwzJlitg6ojpTgAu/CD7TkQAOGgugVP58FFyv2RoWtw=; b=qAH79oizBU4u2ApeYIFLAnubTnTRdGXDdB/8a6ces8TVLfzPQ5AVOLLMQbQKSJ4dfq G92GsVokZ9UBkzGyBPOrBSPweLot252qRrsSYq5N7FSRnnKBHKz37okJDH1eJjeKN3HF B9+DkDZWip6scz91ZfeHKiAqjShEXrC4SRhOJMRPHeUgLEngjilSLiOgusDEVyJONEjY JugVrkfpGUXh6o1B6HPbtlsaKWAhaOb2jpDtZ2lOQebrhbHukFEI4K0lUiZKDZaFEhvd ynTFpvb27xRlIW7c4Tmr43F7lNsWXdTaaQWyjqrDCbyWTrt0+aVOA0ggipJaKWKoVPnk xIHg== X-Gm-Message-State: APjAAAU27ulgk26iKQxGa6RKyU1eJJwXpa0eJ/TnNTVzbMcSQk7v4cNW USDGFDAXvwasT0c+5BTMuOs= X-Google-Smtp-Source: APXvYqy8pbfED4xFpipcpITgTxdfhElPA191PjQR1rjhPeOrBaay791F3suidtz4fnnlrOK6oWHG4g== X-Received: by 2002:a05:600c:2295:: with SMTP id 21mr6789042wmf.63.1571823138669; Wed, 23 Oct 2019 02:32:18 -0700 (PDT) Received: from ?IPv6:2a02:908:1252:fb60:be8a:bd56:1f94:86e7? ([2a02:908:1252:fb60:be8a:bd56:1f94:86e7]) by smtp.gmail.com with ESMTPSA id h17sm22678474wme.6.2019.10.23.02.32.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 23 Oct 2019 02:32:17 -0700 (PDT) Reply-To: christian.koenig@amd.com Subject: Re: [PATCH hmm 00/15] Consolidate the mmu notifier interval_tree and locking 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> <20191023090858.GV11828@phenom.ffwll.local> From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: <13edf841-421e-3522-fcec-ef919c2013ef@gmail.com> Date: Wed, 23 Oct 2019 11:32:16 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20191023090858.GV11828@phenom.ffwll.local> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Am 23.10.19 um 11:08 schrieb Daniel Vetter: > 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 :-) Going another step further.... what hinders us to put the lock into the mmu range notifier itself and have _lock()/_unlock() helpers? I mean having the lock in the driver only makes sense when the driver would be using the same lock for multiple things, e.g. multiple MMU range notifiers under the same lock. But I really don't see that use case here. Regards, Christian. > > Cheers, Daniel