From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 07/11] dma-buf: Restart reservation_object_get_fences_rcu() after writes Date: Fri, 23 Sep 2016 15:03:35 +0200 Message-ID: <20160923130335.GH3988@dvetter-linux.ger.corp.intel.com> References: <20160829070834.22296-1-chris@chris-wilson.co.uk> <20160829070834.22296-7-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20160829070834.22296-7-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Sumit Semwal , linaro-mm-sig@lists.linaro.org, Alex Deucher , Christian =?iso-8859-1?Q?K=F6nig?= , linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org T24gTW9uLCBBdWcgMjksIDIwMTYgYXQgMDg6MDg6MzBBTSArMDEwMCwgQ2hyaXMgV2lsc29uIHdy b3RlOgo+IEluIG9yZGVyIHRvIGJlIGNvbXBsZXRlbHkgZ2VuZXJpYywgd2UgaGF2ZSB0byBkb3Vi bGUgY2hlY2sgdGhlIHJlYWQKPiBzZXFsb2NrIGFmdGVyIGFjcXVpcmluZyBhIHJlZmVyZW5jZSB0 byB0aGUgZmVuY2UuIElmIHRoZSBkcml2ZXIgaXMKPiBhbGxvY2F0aW5nIGZlbmNlcyBmcm9tIGEg U0xBQl9ERVNUUk9ZX0JZX1JDVSwgb3Igc2ltaWxhciBmcmVlbGlzdCwgdGhlbgo+IHdpdGhpbiBh biBSQ1UgZ3JhY2UgcGVyaW9kIGEgZmVuY2UgbWF5IGJlIGZyZWVkIGFuZCByZWFsbG9jYXRlZC4g VGhlIFJDVQo+IHJlYWQgc2lkZSBjcml0aWNhbCBzZWN0aW9uIGRvZXMgbm90IHByZXZlbnQgdGhp cyByZWFsbG9jYXRpb24sIGluc3RlYWQKPiB3ZSBoYXZlIHRvIGluc3BlY3QgdGhlIHJlc2VydmF0 aW9uJ3Mgc2VxbG9jayB0byBkb3VibGUgY2hlY2sgaWYgdGhlCj4gZmVuY2VzIGhhdmUgYmVlbiBy ZWFzc2lnbmVkIGFzIHdlIHdlcmUgYWNxdWlyaW5nIG91ciByZWZlcmVuY2UuCj4gCj4gU2lnbmVk LW9mZi1ieTogQ2hyaXMgV2lsc29uIDxjaHJpc0BjaHJpcy13aWxzb24uY28udWs+Cj4gQ2M6IERh bmllbCBWZXR0ZXIgPGRhbmllbC52ZXR0ZXJAZmZ3bGwuY2g+Cj4gQ2M6IE1hYXJ0ZW4gTGFua2hv cnN0IDxtYWFydGVuLmxhbmtob3JzdEBsaW51eC5pbnRlbC5jb20+Cj4gQ2M6IENocmlzdGlhbiBL w7ZuaWcgPGNocmlzdGlhbi5rb2VuaWdAYW1kLmNvbT4KPiBDYzogQWxleCBEZXVjaGVyIDxhbGV4 YW5kZXIuZGV1Y2hlckBhbWQuY29tPgo+IENjOiBTdW1pdCBTZW13YWwgPHN1bWl0LnNlbXdhbEBs aW5hcm8ub3JnPgo+IENjOiBsaW51eC1tZWRpYUB2Z2VyLmtlcm5lbC5vcmcKPiBDYzogZHJpLWRl dmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwo+IENjOiBsaW5hcm8tbW0tc2lnQGxpc3RzLmxpbmFy by5vcmcKPiAtLS0KPiAgZHJpdmVycy9kbWEtYnVmL3Jlc2VydmF0aW9uLmMgfCA3MSArKysrKysr KysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCj4gIDEgZmlsZSBjaGFuZ2VkLCAz MSBpbnNlcnRpb25zKCspLCA0MCBkZWxldGlvbnMoLSkKPiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVy cy9kbWEtYnVmL3Jlc2VydmF0aW9uLmMgYi9kcml2ZXJzL2RtYS1idWYvcmVzZXJ2YXRpb24uYwo+ IGluZGV4IDcyM2Q4YWY5ODhlNS4uMTBmZDQ0MWRkNGVkIDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMv ZG1hLWJ1Zi9yZXNlcnZhdGlvbi5jCj4gKysrIGIvZHJpdmVycy9kbWEtYnVmL3Jlc2VydmF0aW9u LmMKPiBAQCAtMjgwLDE4ICsyODAsMjQgQEAgaW50IHJlc2VydmF0aW9uX29iamVjdF9nZXRfZmVu Y2VzX3JjdShzdHJ1Y3QgcmVzZXJ2YXRpb25fb2JqZWN0ICpvYmosCj4gIAkJCQkgICAgICB1bnNp Z25lZCAqcHNoYXJlZF9jb3VudCwKPiAgCQkJCSAgICAgIHN0cnVjdCBmZW5jZSAqKipwc2hhcmVk KQo+ICB7Cj4gLQl1bnNpZ25lZCBzaGFyZWRfY291bnQgPSAwOwo+IC0JdW5zaWduZWQgcmV0cnkg PSAxOwo+IC0Jc3RydWN0IGZlbmNlICoqc2hhcmVkID0gTlVMTCwgKmZlbmNlX2V4Y2wgPSBOVUxM Owo+IC0JaW50IHJldCA9IDA7Cj4gKwlzdHJ1Y3QgZmVuY2UgKipzaGFyZWQgPSBOVUxMOwo+ICsJ c3RydWN0IGZlbmNlICpmZW5jZV9leGNsOwo+ICsJdW5zaWduZWQgc2hhcmVkX2NvdW50Owo+ICsJ aW50IHJldCA9IDE7CgpQZXJzb25hbGx5IEknZCBnbyB3aXRoIHJldCA9IC1FQlVTWSBoZXJlLCBi dXQgdGhhdCdzIGEgYmlrZXNoZWQuCgpSZXZpZXdlZC1ieTogRGFuaWVsIFZldHRlciA8ZGFuaWVs LnZldHRlckBmZndsbC5jaD4KPiAgCj4gLQl3aGlsZSAocmV0cnkpIHsKPiArCWRvIHsKPiAgCQlz dHJ1Y3QgcmVzZXJ2YXRpb25fb2JqZWN0X2xpc3QgKmZvYmo7Cj4gIAkJdW5zaWduZWQgc2VxOwo+ ICsJCXVuc2lnbmVkIGk7Cj4gIAo+IC0JCXNlcSA9IHJlYWRfc2VxY291bnRfYmVnaW4oJm9iai0+ c2VxKTsKPiArCQlzaGFyZWRfY291bnQgPSBpID0gMDsKPiAgCj4gIAkJcmN1X3JlYWRfbG9jaygp Owo+ICsJCXNlcSA9IHJlYWRfc2VxY291bnRfYmVnaW4oJm9iai0+c2VxKTsKPiArCj4gKwkJZmVu Y2VfZXhjbCA9IHJjdV9kZXJlZmVyZW5jZShvYmotPmZlbmNlX2V4Y2wpOwo+ICsJCWlmIChmZW5j ZV9leGNsICYmICFmZW5jZV9nZXRfcmN1KGZlbmNlX2V4Y2wpKQo+ICsJCQlnb3RvIHVubG9jazsK PiAgCj4gIAkJZm9iaiA9IHJjdV9kZXJlZmVyZW5jZShvYmotPmZlbmNlKTsKPiAgCQlpZiAoZm9i aikgewo+IEBAIC0zMDksNTIgKzMxNSwzNyBAQCBpbnQgcmVzZXJ2YXRpb25fb2JqZWN0X2dldF9m ZW5jZXNfcmN1KHN0cnVjdCByZXNlcnZhdGlvbl9vYmplY3QgKm9iaiwKPiAgCQkJCX0KPiAgCj4g IAkJCQlyZXQgPSAtRU5PTUVNOwo+IC0JCQkJc2hhcmVkX2NvdW50ID0gMDsKPiAgCQkJCWJyZWFr Owo+ICAJCQl9Cj4gIAkJCXNoYXJlZCA9IG5zaGFyZWQ7Cj4gLQkJCW1lbWNweShzaGFyZWQsIGZv YmotPnNoYXJlZCwgc3opOwo+ICAJCQlzaGFyZWRfY291bnQgPSBmb2JqLT5zaGFyZWRfY291bnQ7 Cj4gLQkJfSBlbHNlCj4gLQkJCXNoYXJlZF9jb3VudCA9IDA7Cj4gLQkJZmVuY2VfZXhjbCA9IHJj dV9kZXJlZmVyZW5jZShvYmotPmZlbmNlX2V4Y2wpOwo+IC0KPiAtCQlyZXRyeSA9IHJlYWRfc2Vx Y291bnRfcmV0cnkoJm9iai0+c2VxLCBzZXEpOwo+IC0JCWlmIChyZXRyeSkKPiAtCQkJZ290byB1 bmxvY2s7Cj4gLQo+IC0JCWlmICghZmVuY2VfZXhjbCB8fCBmZW5jZV9nZXRfcmN1KGZlbmNlX2V4 Y2wpKSB7Cj4gLQkJCXVuc2lnbmVkIGk7Cj4gIAo+ICAJCQlmb3IgKGkgPSAwOyBpIDwgc2hhcmVk X2NvdW50OyArK2kpIHsKPiAtCQkJCWlmIChmZW5jZV9nZXRfcmN1KHNoYXJlZFtpXSkpCj4gLQkJ CQkJY29udGludWU7Cj4gLQo+IC0JCQkJLyogdWggb2gsIHJlZmNvdW50IGZhaWxlZCwgYWJvcnQg YW5kIHJldHJ5ICovCj4gLQkJCQl3aGlsZSAoaS0tKQo+IC0JCQkJCWZlbmNlX3B1dChzaGFyZWRb aV0pOwo+IC0KPiAtCQkJCWlmIChmZW5jZV9leGNsKSB7Cj4gLQkJCQkJZmVuY2VfcHV0KGZlbmNl X2V4Y2wpOwo+IC0JCQkJCWZlbmNlX2V4Y2wgPSBOVUxMOwo+IC0JCQkJfQo+IC0KPiAtCQkJCXJl dHJ5ID0gMTsKPiAtCQkJCWJyZWFrOwo+ICsJCQkJc2hhcmVkW2ldID0gcmN1X2RlcmVmZXJlbmNl KGZvYmotPnNoYXJlZFtpXSk7Cj4gKwkJCQlpZiAoIWZlbmNlX2dldF9yY3Uoc2hhcmVkW2ldKSkK PiArCQkJCQlicmVhazsKPiAgCQkJfQo+IC0JCX0gZWxzZQo+IC0JCQlyZXRyeSA9IDE7Cj4gKwkJ fQo+ICsKPiArCQlpZiAoaSAhPSBzaGFyZWRfY291bnQgfHwgcmVhZF9zZXFjb3VudF9yZXRyeSgm b2JqLT5zZXEsIHNlcSkpIHsKPiArCQkJd2hpbGUgKGktLSkKPiArCQkJCWZlbmNlX3B1dChzaGFy ZWRbaV0pOwo+ICsJCQlmZW5jZV9wdXQoZmVuY2VfZXhjbCk7Cj4gKwkJCWdvdG8gdW5sb2NrOwo+ ICsJCX0KPiAgCj4gKwkJcmV0ID0gMDsKPiAgdW5sb2NrOgo+ICAJCXJjdV9yZWFkX3VubG9jaygp Owo+IC0JfQo+IC0JKnBzaGFyZWRfY291bnQgPSBzaGFyZWRfY291bnQ7Cj4gLQlpZiAoc2hhcmVk X2NvdW50KQo+IC0JCSpwc2hhcmVkID0gc2hhcmVkOwo+IC0JZWxzZSB7Cj4gLQkJKnBzaGFyZWQg PSBOVUxMOwo+ICsJfSB3aGlsZSAocmV0KTsKPiArCj4gKwlpZiAoIXNoYXJlZF9jb3VudCkgewo+ ICAJCWtmcmVlKHNoYXJlZCk7Cj4gKwkJc2hhcmVkID0gTlVMTDsKPiAgCX0KPiArCj4gKwkqcHNo YXJlZF9jb3VudCA9IHNoYXJlZF9jb3VudDsKPiArCSpwc2hhcmVkID0gc2hhcmVkOwo+ICAJKnBm ZW5jZV9leGNsID0gZmVuY2VfZXhjbDsKPiAgCj4gIAlyZXR1cm4gcmV0Owo+IC0tIAo+IDIuOS4z Cj4gCgotLSAKRGFuaWVsIFZldHRlcgpTb2Z0d2FyZSBFbmdpbmVlciwgSW50ZWwgQ29ycG9yYXRp b24KaHR0cDovL2Jsb2cuZmZ3bGwuY2gKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX18KSW50ZWwtZ2Z4IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJl ZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGlu Zm8vaW50ZWwtZ2Z4Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-lf0-f66.google.com ([209.85.215.66]:35466 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755454AbcIWNDj (ORCPT ); Fri, 23 Sep 2016 09:03:39 -0400 Received: by mail-lf0-f66.google.com with SMTP id s64so5672704lfs.2 for ; Fri, 23 Sep 2016 06:03:38 -0700 (PDT) Date: Fri, 23 Sep 2016 15:03:35 +0200 From: Daniel Vetter To: Chris Wilson Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, Daniel Vetter , Maarten Lankhorst , Christian =?iso-8859-1?Q?K=F6nig?= , Alex Deucher , Sumit Semwal , linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org Subject: Re: [PATCH 07/11] dma-buf: Restart reservation_object_get_fences_rcu() after writes Message-ID: <20160923130335.GH3988@dvetter-linux.ger.corp.intel.com> References: <20160829070834.22296-1-chris@chris-wilson.co.uk> <20160829070834.22296-7-chris@chris-wilson.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160829070834.22296-7-chris@chris-wilson.co.uk> Sender: linux-media-owner@vger.kernel.org List-ID: On Mon, Aug 29, 2016 at 08:08:30AM +0100, Chris Wilson wrote: > In order to be completely generic, we have to double check the read > seqlock after acquiring a reference to the fence. If the driver is > allocating fences from a SLAB_DESTROY_BY_RCU, or similar freelist, then > within an RCU grace period a fence may be freed and reallocated. The RCU > read side critical section does not prevent this reallocation, instead > we have to inspect the reservation's seqlock to double check if the > fences have been reassigned as we were acquiring our reference. > > Signed-off-by: Chris Wilson > Cc: Daniel Vetter > Cc: Maarten Lankhorst > Cc: Christian König > Cc: Alex Deucher > Cc: Sumit Semwal > Cc: linux-media@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linaro-mm-sig@lists.linaro.org > --- > drivers/dma-buf/reservation.c | 71 +++++++++++++++++++------------------------ > 1 file changed, 31 insertions(+), 40 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 723d8af988e5..10fd441dd4ed 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -280,18 +280,24 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, > unsigned *pshared_count, > struct fence ***pshared) > { > - unsigned shared_count = 0; > - unsigned retry = 1; > - struct fence **shared = NULL, *fence_excl = NULL; > - int ret = 0; > + struct fence **shared = NULL; > + struct fence *fence_excl; > + unsigned shared_count; > + int ret = 1; Personally I'd go with ret = -EBUSY here, but that's a bikeshed. Reviewed-by: Daniel Vetter > > - while (retry) { > + do { > struct reservation_object_list *fobj; > unsigned seq; > + unsigned i; > > - seq = read_seqcount_begin(&obj->seq); > + shared_count = i = 0; > > rcu_read_lock(); > + seq = read_seqcount_begin(&obj->seq); > + > + fence_excl = rcu_dereference(obj->fence_excl); > + if (fence_excl && !fence_get_rcu(fence_excl)) > + goto unlock; > > fobj = rcu_dereference(obj->fence); > if (fobj) { > @@ -309,52 +315,37 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, > } > > ret = -ENOMEM; > - shared_count = 0; > break; > } > shared = nshared; > - memcpy(shared, fobj->shared, sz); > shared_count = fobj->shared_count; > - } else > - shared_count = 0; > - fence_excl = rcu_dereference(obj->fence_excl); > - > - retry = read_seqcount_retry(&obj->seq, seq); > - if (retry) > - goto unlock; > - > - if (!fence_excl || fence_get_rcu(fence_excl)) { > - unsigned i; > > for (i = 0; i < shared_count; ++i) { > - if (fence_get_rcu(shared[i])) > - continue; > - > - /* uh oh, refcount failed, abort and retry */ > - while (i--) > - fence_put(shared[i]); > - > - if (fence_excl) { > - fence_put(fence_excl); > - fence_excl = NULL; > - } > - > - retry = 1; > - break; > + shared[i] = rcu_dereference(fobj->shared[i]); > + if (!fence_get_rcu(shared[i])) > + break; > } > - } else > - retry = 1; > + } > + > + if (i != shared_count || read_seqcount_retry(&obj->seq, seq)) { > + while (i--) > + fence_put(shared[i]); > + fence_put(fence_excl); > + goto unlock; > + } > > + ret = 0; > unlock: > rcu_read_unlock(); > - } > - *pshared_count = shared_count; > - if (shared_count) > - *pshared = shared; > - else { > - *pshared = NULL; > + } while (ret); > + > + if (!shared_count) { > kfree(shared); > + shared = NULL; > } > + > + *pshared_count = shared_count; > + *pshared = shared; > *pfence_excl = fence_excl; > > return ret; > -- > 2.9.3 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch