From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alistair Popple Date: Mon, 22 Mar 2021 09:27:33 +0000 Subject: Re: [PATCH v6 8/8] nouveau/svm: Implement atomic SVM access Message-Id: <6407817.nLXe9rGL3b@nvdebian> List-Id: References: <20210312083851.15981-1-apopple@nvidia.com> <20210312083851.15981-9-apopple@nvidia.com> <20210315075113.GD4136862@infradead.org> In-Reply-To: <20210315075113.GD4136862@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Christoph Hellwig Cc: linux-mm@kvack.org, nouveau@lists.freedesktop.org, bskeggs@redhat.com, akpm@linux-foundation.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org, dri-devel@lists.freedesktop.org, jhubbard@nvidia.com, rcampbell@nvidia.com, jglisse@redhat.com, jgg@nvidia.com, daniel@ffwll.ch, willy@infradead.org On Monday, 15 March 2021 6:51:13 PM AEDT Christoph Hellwig wrote: > > - /*XXX: atomic? */ > > - return (fa->access = 0 || fa->access = 3) - > > - (fb->access = 0 || fb->access = 3); > > + /* Atomic access (2) has highest priority */ > > + return (-1*(fa->access = 2) + (fa->access = 0 || fa->access = 3)) - > > + (-1*(fb->access = 2) + (fb->access = 0 || fb->access = 3)); > > This looks really unreabable. If the magic values 0, 2 and 3 had names > it might become a little more understadable, then factor the duplicated > calculation of the priority value into a helper and we'll have code that > mere humans can understand.. Fair enough, will add some definitions for the magic values. > > + mutex_lock(&svmm->mutex); > > + if (mmu_interval_read_retry(¬ifier->notifier, > > + notifier_seq)) { > > + mutex_unlock(&svmm->mutex); > > + continue; > > + } > > + break; > > + } > > This looks good, why not: > > mutex_lock(&svmm->mutex); > if (!mmu_interval_read_retry(¬ifier->notifier, > notifier_seq)) > break; > mutex_unlock(&svmm->mutex); > } I had copied that from nouveau_range_fault() but this suggestion is better. Will update, thanks for looking.