From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Christian_K=c3=b6nig?= Subject: Re: [PATCH] mm, oom: distinguish blockable mode for mmu notifiers Date: Fri, 24 Aug 2018 13:43:16 +0200 Message-ID: References: <20180716115058.5559-1-mhocko@kernel.org> <8cbfb09f-0c5a-8d43-1f5e-f3ff7612e289@I-love.SAKURA.ne.jp> <20180824113248.GH29735@dhcp22.suse.cz> Reply-To: christian.koenig-5C7GfCeVMHo@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20180824113248.GH29735-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org> Content-Language: en-US List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: Michal Hocko , Tetsuo Handa , =?UTF-8?Q?Christian_K=c3=b6nig?= Cc: kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , David Airlie , Joonas Lahtinen , Sudeep Dutt , dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Andrea Arcangeli , "David (ChunMing) Zhou" , Dimitri Sivanich , linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Jason Gunthorpe , Doug Ledford , David Rientjes , xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b@public.gmane.org, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Jani Nikula , Leon Romanovsky , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Rodrigo Vivi , Boris Ostrovsky , Juergen Gross Mike Marciniszyn QW0gMjQuMDguMjAxOCB1bSAxMzozMiBzY2hyaWViIE1pY2hhbCBIb2NrbzoKPiBPbiBGcmkgMjQt MDgtMTggMTk6NTQ6MTksIFRldHN1byBIYW5kYSB3cm90ZToKPj4gVHdvIG1vcmUgd29ycmllcyBm b3IgdGhpcyBwYXRjaC4KPj4KPj4KPj4KPj4+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9hbWQvYW1k Z3B1L2FtZGdwdV9tbi5jCj4+PiArKysgYi9kcml2ZXJzL2dwdS9kcm0vYW1kL2FtZGdwdS9hbWRn cHVfbW4uYwo+Pj4gQEAgLTE3OCwxMiArMTc4LDE4IEBAIHZvaWQgYW1kZ3B1X21uX3VubG9jayhz dHJ1Y3QgYW1kZ3B1X21uICptbikKPj4+ICAgICoKPj4+ICAgICogQGFtbjogb3VyIG5vdGlmaWVy Cj4+PiAgICAqLwo+Pj4gLXN0YXRpYyB2b2lkIGFtZGdwdV9tbl9yZWFkX2xvY2soc3RydWN0IGFt ZGdwdV9tbiAqYW1uKQo+Pj4gK3N0YXRpYyBpbnQgYW1kZ3B1X21uX3JlYWRfbG9jayhzdHJ1Y3Qg YW1kZ3B1X21uICphbW4sIGJvb2wgYmxvY2thYmxlKQo+Pj4gICB7Cj4+PiAtICAgICAgIG11dGV4 X2xvY2soJmFtbi0+cmVhZF9sb2NrKTsKPj4+ICsgICAgICAgaWYgKGJsb2NrYWJsZSkKPj4+ICsg ICAgICAgICAgICAgICBtdXRleF9sb2NrKCZhbW4tPnJlYWRfbG9jayk7Cj4+PiArICAgICAgIGVs c2UgaWYgKCFtdXRleF90cnlsb2NrKCZhbW4tPnJlYWRfbG9jaykpCj4+PiArICAgICAgICAgICAg ICAgcmV0dXJuIC1FQUdBSU47Cj4+PiArCj4+PiAgICAgICAgICBpZiAoYXRvbWljX2luY19yZXR1 cm4oJmFtbi0+cmVjdXJzaW9uKSA9PSAxKQo+Pj4gICAgICAgICAgICAgICAgICBkb3duX3JlYWRf bm9uX293bmVyKCZhbW4tPmxvY2spOwo+PiBXaHkgZG9uJ3Qgd2UgbmVlZCB0byB1c2UgdHJ5bG9j ayBoZXJlIGlmIGJsb2NrYWJsZSA9PSBmYWxzZSA/Cj4+IFdhbnQgY29tbWVudCB3aHkgaXQgaXMg c2FmZSB0byB1c2UgYmxvY2tpbmcgbG9jayBoZXJlLgo+IEhtbSwgSSBhbSBwcmV0dHkgc3VyZSBJ IGhhdmUgY2hlY2tlZCB0aGUgY29kZSBidXQgaXQgd2FzIHF1aXRlIGNvbmZ1c2luZwo+IHNvIEkg bWlnaHQgaGF2ZSBtaXNzZWQgc29tZXRoaW5nLiBEb3VibGUgY2hlY2tpbmcgbm93LCBpdCBzZWVt cyB0aGF0Cj4gdGhpcyByZWFkX2xvY2sgaXMgbm90IHVzZWQgYW55d2hlcmUgZWxzZSBhbmQgaXQg aXMgbm90IF90aGVfIGxvY2sgd2UgYXJlCj4gaW50ZXJlc3RlZCBhYm91dC4gSXQgaXMgdGhlIGFt bi0+bG9jayAoYW1kZ3B1X21uX2xvY2spIHdoaWNoIG1hdHRlcnMgYXMKPiBpdCBpcyB0YWtlbiBp biBleGNsdXNpdmUgbW9kZSBmb3IgZXhwZW5zaXZlIG9wZXJhdGlvbnMuCgpUaGUgd3JpdGUgc2lk ZSBvZiB0aGUgbG9jayBpcyBvbmx5IHRha2VuIGluIHRoZSBjb21tYW5kIHN1Ym1pc3Npb24gSU9D VEwuCgpTbyB5b3UgYWN0dWFsbHkgZG9uJ3QgbmVlZCB0byBjaGFuZ2UgYW55dGhpbmcgaGVyZSAo ZXZlbiB0aGUgcHJvcG9zZWQgCmNoYW5nZXMgYXJlIG92ZXJraWxsKSBzaW5jZSB3ZSBjYW4ndCB0 ZWFyIGRvd24gdGhlIHN0cnVjdF9tbSB3aGlsZSBhbiAKSU9DVEwgaXMgc3RpbGwgdXNpbmcuCgo+ IElzIHRoYXQgY29ycmVjdCBDaHJpc3RpYW4/IElmIHRoaXMgaXMgY29ycmVjdCB0aGVuIHdlIG5l ZWQgdG8gdXBkYXRlIHRoZQo+IGxvY2tpbmcgaGVyZS4gSSBhbSBzdHJ1Z2dsaW5nIHRvIGdyYXNw IHRoZSByZWYgY291bnRpbmcgcGFydC4gV2h5IGNhbm5vdAo+IGFsbCByZWFkZXJzIHNpbXBseSB0 YWtlIHRoZSBsb2NrIHJhdGhlciB0aGFuIHJlbHkgb24gc29tZWJvZHkgZWxzZSB0bwo+IHRha2Ug aXQ/IDFlZDNkMjU2N2M4MDAgZGlkbid0IHJlYWxseSBoZWxwIG1lIHRvIHVuZGVyc3RhbmQgdGhl IGxvY2tpbmcKPiBzY2hlbWUgaGVyZSBzbyBhbnkgaGVscCB3b3VsZCBiZSBhcHByZWNpYXRlZC4K ClRoYXQgd29uJ3Qgd29yayBsaWtlIHRoaXMgdGhlcmUgbWlnaHQgYmUgbXVsdGlwbGUgCmludmFs aWRhdGVfcmFuZ2Vfc3RhcnQoKS9pbnZhbGlkYXRlX3JhbmdlX2VuZCgpIHBhaXJzIG9wZW4gYXQg dGhlIHNhbWUgCnRpbWUuIEUuZy4gdGhlIGxvY2sgbWlnaHQgYmUgdGFrZW4gcmVjdXJzaXZlbHkg YW5kIHRoYXQgaXMgaWxsZWdhbCBmb3IgYSAKcndfc2VtYXBob3JlLgoKUmVnYXJkcywKQ2hyaXN0 aWFuLgoKPgo+IEkgYW0gd29uZGVyaW5nIHdoeSB3ZSBjYW5ub3QgZG8KPiBkaWZmIC0tZ2l0IGEv ZHJpdmVycy9ncHUvZHJtL2FtZC9hbWRncHUvYW1kZ3B1X21uLmMgYi9kcml2ZXJzL2dwdS9kcm0v YW1kL2FtZGdwdS9hbWRncHVfbW4uYwo+IGluZGV4IGU1NTUwOGIzOTQ5Ni4uOTMwMzQxNzg2NzNk IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9hbWQvYW1kZ3B1L2FtZGdwdV9tbi5jCj4g KysrIGIvZHJpdmVycy9ncHUvZHJtL2FtZC9hbWRncHUvYW1kZ3B1X21uLmMKPiBAQCAtMTgwLDE0 ICsxODAsMTEgQEAgdm9pZCBhbWRncHVfbW5fdW5sb2NrKHN0cnVjdCBhbWRncHVfbW4gKm1uKQo+ ICAgICovCj4gICBzdGF0aWMgaW50IGFtZGdwdV9tbl9yZWFkX2xvY2soc3RydWN0IGFtZGdwdV9t biAqYW1uLCBib29sIGJsb2NrYWJsZSkKPiAgIHsKPiAtCWlmIChibG9ja2FibGUpCj4gLQkJbXV0 ZXhfbG9jaygmYW1uLT5yZWFkX2xvY2spOwo+IC0JZWxzZSBpZiAoIW11dGV4X3RyeWxvY2soJmFt bi0+cmVhZF9sb2NrKSkKPiAtCQlyZXR1cm4gLUVBR0FJTjsKPiAtCj4gLQlpZiAoYXRvbWljX2lu Y19yZXR1cm4oJmFtbi0+cmVjdXJzaW9uKSA9PSAxKQo+IC0JCWRvd25fcmVhZF9ub25fb3duZXIo JmFtbi0+bG9jayk7Cj4gLQltdXRleF91bmxvY2soJmFtbi0+cmVhZF9sb2NrKTsKPiArCWlmICgh ZG93bl9yZWFkX3RyeWxvY2soJmFtbi0+bG9jaykpIHsKPiArCQlpZiAoIWJsb2NrYWJsZSkKPiAr CQkJcmV0dXJuIC1FQUdBSU47Cj4gKwkJZG93bl9yZWFkKGFtbi0+bG9jayk7Cj4gKwl9Cj4gICAK PiAgIAlyZXR1cm4gMDsKPiAgIH0KPiBAQCAtMTk5LDggKzE5Niw3IEBAIHN0YXRpYyBpbnQgYW1k Z3B1X21uX3JlYWRfbG9jayhzdHJ1Y3QgYW1kZ3B1X21uICphbW4sIGJvb2wgYmxvY2thYmxlKQo+ ICAgICovCj4gICBzdGF0aWMgdm9pZCBhbWRncHVfbW5fcmVhZF91bmxvY2soc3RydWN0IGFtZGdw dV9tbiAqYW1uKQo+ICAgewo+IC0JaWYgKGF0b21pY19kZWNfcmV0dXJuKCZhbW4tPnJlY3Vyc2lv bikgPT0gMCkKPiAtCQl1cF9yZWFkX25vbl9vd25lcigmYW1uLT5sb2NrKTsKPiArCXVwX3JlYWQo JmFtbi0+bG9jayk7Cj4gICB9Cj4gICAKPiAgIC8qKgo+CgpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwphbWQtZ2Z4IG1haWxpbmcgbGlzdAphbWQtZ2Z4QGxp c3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFu L2xpc3RpbmZvL2FtZC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f69.google.com (mail-wm0-f69.google.com [74.125.82.69]) by kanga.kvack.org (Postfix) with ESMTP id 2AAFF6B2F76 for ; Fri, 24 Aug 2018 07:43:21 -0400 (EDT) Received: by mail-wm0-f69.google.com with SMTP id m129-v6so989192wma.8 for ; Fri, 24 Aug 2018 04:43:21 -0700 (PDT) Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id z6-v6sor2626882wrq.54.2018.08.24.04.43.19 for (Google Transport Security); Fri, 24 Aug 2018 04:43:19 -0700 (PDT) Reply-To: christian.koenig@amd.com Subject: Re: [PATCH] mm, oom: distinguish blockable mode for mmu notifiers References: <20180716115058.5559-1-mhocko@kernel.org> <8cbfb09f-0c5a-8d43-1f5e-f3ff7612e289@I-love.SAKURA.ne.jp> <20180824113248.GH29735@dhcp22.suse.cz> From: =?UTF-8?Q?Christian_K=c3=b6nig?= Message-ID: Date: Fri, 24 Aug 2018 13:43:16 +0200 MIME-Version: 1.0 In-Reply-To: <20180824113248.GH29735@dhcp22.suse.cz> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko , Tetsuo Handa , =?UTF-8?Q?Christian_K=c3=b6nig?= Cc: kvm@vger.kernel.org, =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , David Airlie , Joonas Lahtinen , Sudeep Dutt , dri-devel@lists.freedesktop.org, linux-mm@kvack.org, Andrea Arcangeli , "David (ChunMing) Zhou" , Dimitri Sivanich , linux-rdma@vger.kernel.org, amd-gfx@lists.freedesktop.org, Jason Gunthorpe , Doug Ledford , David Rientjes , xen-devel@lists.xenproject.org, intel-gfx@lists.freedesktop.org, Jani Nikula , Leon Romanovsky , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Rodrigo Vivi , Boris Ostrovsky , Juergen Gross , Mike Marciniszyn , Dennis Dalessandro , LKML , Ashutosh Dixit , Alex Deucher , Paolo Bonzini , Andrew Morton , Felix Kuehling Am 24.08.2018 um 13:32 schrieb Michal Hocko: > On Fri 24-08-18 19:54:19, Tetsuo Handa wrote: >> Two more worries for this patch. >> >> >> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c >>> @@ -178,12 +178,18 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn) >>> * >>> * @amn: our notifier >>> */ >>> -static void amdgpu_mn_read_lock(struct amdgpu_mn *amn) >>> +static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable) >>> { >>> - mutex_lock(&amn->read_lock); >>> + if (blockable) >>> + mutex_lock(&amn->read_lock); >>> + else if (!mutex_trylock(&amn->read_lock)) >>> + return -EAGAIN; >>> + >>> if (atomic_inc_return(&amn->recursion) == 1) >>> down_read_non_owner(&amn->lock); >> Why don't we need to use trylock here if blockable == false ? >> Want comment why it is safe to use blocking lock here. > Hmm, I am pretty sure I have checked the code but it was quite confusing > so I might have missed something. Double checking now, it seems that > this read_lock is not used anywhere else and it is not _the_ lock we are > interested about. It is the amn->lock (amdgpu_mn_lock) which matters as > it is taken in exclusive mode for expensive operations. The write side of the lock is only taken in the command submission IOCTL. So you actually don't need to change anything here (even the proposed changes are overkill) since we can't tear down the struct_mm while an IOCTL is still using. > Is that correct Christian? If this is correct then we need to update the > locking here. I am struggling to grasp the ref counting part. Why cannot > all readers simply take the lock rather than rely on somebody else to > take it? 1ed3d2567c800 didn't really help me to understand the locking > scheme here so any help would be appreciated. That won't work like this there might be multiple invalidate_range_start()/invalidate_range_end() pairs open at the same time. E.g. the lock might be taken recursively and that is illegal for a rw_semaphore. Regards, Christian. > > I am wondering why we cannot do > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c > index e55508b39496..93034178673d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c > @@ -180,14 +180,11 @@ void amdgpu_mn_unlock(struct amdgpu_mn *mn) > */ > static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable) > { > - if (blockable) > - mutex_lock(&amn->read_lock); > - else if (!mutex_trylock(&amn->read_lock)) > - return -EAGAIN; > - > - if (atomic_inc_return(&amn->recursion) == 1) > - down_read_non_owner(&amn->lock); > - mutex_unlock(&amn->read_lock); > + if (!down_read_trylock(&amn->lock)) { > + if (!blockable) > + return -EAGAIN; > + down_read(amn->lock); > + } > > return 0; > } > @@ -199,8 +196,7 @@ static int amdgpu_mn_read_lock(struct amdgpu_mn *amn, bool blockable) > */ > static void amdgpu_mn_read_unlock(struct amdgpu_mn *amn) > { > - if (atomic_dec_return(&amn->recursion) == 0) > - up_read_non_owner(&amn->lock); > + up_read(&amn->lock); > } > > /** >