| Message ID | 20171121140850.23401-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1eH9Ed-0007n3-NL; Tue, 21 Nov 2017 14:09:12 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751286AbdKUOIz (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 21 Nov 2017 09:08:55 -0500 Received: from mail-qt0-f196.google.com ([209.85.216.196]:33597 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751105AbdKUOIy (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 21 Nov 2017 09:08:54 -0500 Received: by mail-qt0-f196.google.com with SMTP id r58so19349618qtc.0; Tue, 21 Nov 2017 06:08:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=6H4Q1jFafiMdTYq+lD68oKGM4b4ecOAFjSapfszNOUA=; b=eR/1tqU4ILivCJYsEIzjferCco9m8NJO8QkNDOVTrvwOQWpTISUP3ta0M+2zl3bPoX g2pNLCEy6Z8Kbd4cbfjqEWwG967An1nwkEyAE6j9qHAOleZ+u4qzvL1rIRPznLDW1M0+ 2Sdlbc0+TGayrsGyMryH1Yj+4iQ8g2tReIND1teB0nfqjPgap29WMMhiWKgDVSkBnbYc AvaGFs6zuLFc5CYDVfZ3TcJo/cv81SDB/ovsNFxEjMqFdem3XmcNBMjKpsOSaylO3Utv CWx6ixx5NeyrvfEQVAfZgtfQN8Frl4+S/t5OQ+v4UURJPgXl3m1ed2AnlvfoG1iKIauE bihQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=6H4Q1jFafiMdTYq+lD68oKGM4b4ecOAFjSapfszNOUA=; b=W0N+ENqiGx32G3faQVb8gS357r8lw8mFcPZPceBQ5RnjrZRpakjZiGWAh++EWxi/lF X8AZt9LEh03pgy5PfDRfVeIl1y2TH+cza1M8WuLZYXHCcAnwKjrZ6tWCbxo51FPbWG53 nbRPogKIZjZFtobaJNfiZyUNorQUoXthGmyMBZPMubXJ4uUBBL24DEgWUso8ZYilZaxc WZIVy4dHwoyklq90jq7L85VZ45OGz2uHkdFFA4PxUstHHhBuG1Ip2Co435QmGVvhpAtw kX5i5CwS/MdJ2fGh2sSlSc2v8PSfSOUQaYpKAaRDQ/Lzb+jfmK/cpr+nNG9uDD6iP+BN /6dA== X-Gm-Message-State: AJaThX6AwXM09zyygDaS7u0TbC8U++z4r+VTay56P1BZ53NTSbckXkrk OsG7NwY/hp3PGOtQ0MAhGzU= X-Google-Smtp-Source: AGs4zMZIrlsYeWwVusVWzZzjs9qAYW7n031oA/YsVZ/qs9wZAGvuA/8zI7l1XXND/VO24DmuT6F0ew== X-Received: by 10.200.9.86 with SMTP id z22mr11567066qth.96.1511273333533; Tue, 21 Nov 2017 06:08:53 -0800 (PST) Received: from localhost ([144.121.20.162]) by smtp.gmail.com with ESMTPSA id x1sm2079926qti.91.2017.11.21.06.08.52 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 21 Nov 2017 06:08:53 -0800 (PST) From: Rob Clark <robdclark@gmail.com> To: dri-devel@lists.freedesktop.org Cc: Rob Clark <robdclark@gmail.com>, Sumit Semwal <sumit.semwal@linaro.org>, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org Subject: [PATCH] reservation: don't wait when timeout=0 Date: Tue, 21 Nov 2017 09:08:46 -0500 Message-Id: <20171121140850.23401-1-robdclark@gmail.com> X-Mailer: git-send-email 2.13.6 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Rob Clark
Nov. 21, 2017, 2:08 p.m. UTC
If we are testing if a reservation object's fences have been
signaled with timeout=0 (non-blocking), we need to pass 0 for
timeout to dma_fence_wait_timeout().
Plus bonus spelling correction.
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
drivers/dma-buf/reservation.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
Comments
Am 21.11.2017 um 15:08 schrieb Rob Clark: > If we are testing if a reservation object's fences have been > signaled with timeout=0 (non-blocking), we need to pass 0 for > timeout to dma_fence_wait_timeout(). > > Plus bonus spelling correction. > > Signed-off-by: Rob Clark <robdclark@gmail.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/reservation.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index dec3a815455d..71f51140a9ad 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu); > * > * RETURNS > * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or > - * greater than zer on success. > + * greater than zero on success. > */ > long reservation_object_wait_timeout_rcu(struct reservation_object *obj, > bool wait_all, bool intr, > @@ -483,7 +483,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, > goto retry; > } > > - ret = dma_fence_wait_timeout(fence, intr, ret); > + /* > + * Note that dma_fence_wait_timeout() will return 1 if > + * the fence is already signaled, so in the wait_all > + * case when we go through the retry loop again, ret > + * will be greater than 0 and we don't want this to > + * cause _wait_timeout() to block > + */ > + ret = dma_fence_wait_timeout(fence, intr, timeout ? ret : 0); > dma_fence_put(fence); > if (ret > 0 && wait_all && (i + 1 < shared_count)) > goto retry;
Quoting Rob Clark (2017-11-21 14:08:46) > If we are testing if a reservation object's fences have been > signaled with timeout=0 (non-blocking), we need to pass 0 for > timeout to dma_fence_wait_timeout(). > > Plus bonus spelling correction. > > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > drivers/dma-buf/reservation.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index dec3a815455d..71f51140a9ad 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu); > * > * RETURNS > * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or > - * greater than zer on success. > + * greater than zero on success. > */ > long reservation_object_wait_timeout_rcu(struct reservation_object *obj, > bool wait_all, bool intr, > @@ -483,7 +483,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, > goto retry; > } > > - ret = dma_fence_wait_timeout(fence, intr, ret); > + /* > + * Note that dma_fence_wait_timeout() will return 1 if > + * the fence is already signaled, so in the wait_all > + * case when we go through the retry loop again, ret > + * will be greater than 0 and we don't want this to > + * cause _wait_timeout() to block > + */ > + ret = dma_fence_wait_timeout(fence, intr, timeout ? ret : 0); One should ask if we should just fix the interface to stop returning incorrect results (stop "correcting" a completion with 0 jiffies remaining as 1). A timeout can be distinguished by -ETIME (or your pick of errno). -Chris
On Tue, Nov 21, 2017 at 9:38 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Rob Clark (2017-11-21 14:08:46) >> If we are testing if a reservation object's fences have been >> signaled with timeout=0 (non-blocking), we need to pass 0 for >> timeout to dma_fence_wait_timeout(). >> >> Plus bonus spelling correction. >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> >> --- >> drivers/dma-buf/reservation.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c >> index dec3a815455d..71f51140a9ad 100644 >> --- a/drivers/dma-buf/reservation.c >> +++ b/drivers/dma-buf/reservation.c >> @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu); >> * >> * RETURNS >> * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or >> - * greater than zer on success. >> + * greater than zero on success. >> */ >> long reservation_object_wait_timeout_rcu(struct reservation_object *obj, >> bool wait_all, bool intr, >> @@ -483,7 +483,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, >> goto retry; >> } >> >> - ret = dma_fence_wait_timeout(fence, intr, ret); >> + /* >> + * Note that dma_fence_wait_timeout() will return 1 if >> + * the fence is already signaled, so in the wait_all >> + * case when we go through the retry loop again, ret >> + * will be greater than 0 and we don't want this to >> + * cause _wait_timeout() to block >> + */ >> + ret = dma_fence_wait_timeout(fence, intr, timeout ? ret : 0); > > One should ask if we should just fix the interface to stop returning > incorrect results (stop "correcting" a completion with 0 jiffies remaining > as 1). A timeout can be distinguished by -ETIME (or your pick of errno). perhaps -EBUSY, if we go that route (although maybe it should be a follow-on patch, this one is suitable for backport to stable/lts if one should so choose..) I think current approach was chosen to match schedule_timeout() and other such functions that take a timeout in jiffies. Not making a judgement on whether that is a good or bad reason.. BR, -R > -Chris
Am 21.11.2017 um 15:59 schrieb Rob Clark: > On Tue, Nov 21, 2017 at 9:38 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> Quoting Rob Clark (2017-11-21 14:08:46) >>> If we are testing if a reservation object's fences have been >>> signaled with timeout=0 (non-blocking), we need to pass 0 for >>> timeout to dma_fence_wait_timeout(). >>> >>> Plus bonus spelling correction. >>> >>> Signed-off-by: Rob Clark <robdclark@gmail.com> >>> --- >>> drivers/dma-buf/reservation.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c >>> index dec3a815455d..71f51140a9ad 100644 >>> --- a/drivers/dma-buf/reservation.c >>> +++ b/drivers/dma-buf/reservation.c >>> @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu); >>> * >>> * RETURNS >>> * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or >>> - * greater than zer on success. >>> + * greater than zero on success. >>> */ >>> long reservation_object_wait_timeout_rcu(struct reservation_object *obj, >>> bool wait_all, bool intr, >>> @@ -483,7 +483,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, >>> goto retry; >>> } >>> >>> - ret = dma_fence_wait_timeout(fence, intr, ret); >>> + /* >>> + * Note that dma_fence_wait_timeout() will return 1 if >>> + * the fence is already signaled, so in the wait_all >>> + * case when we go through the retry loop again, ret >>> + * will be greater than 0 and we don't want this to >>> + * cause _wait_timeout() to block >>> + */ >>> + ret = dma_fence_wait_timeout(fence, intr, timeout ? ret : 0); >> One should ask if we should just fix the interface to stop returning >> incorrect results (stop "correcting" a completion with 0 jiffies remaining >> as 1). A timeout can be distinguished by -ETIME (or your pick of errno). > perhaps -EBUSY, if we go that route (although maybe it should be a > follow-on patch, this one is suitable for backport to stable/lts if > one should so choose..) > > I think current approach was chosen to match schedule_timeout() and > other such functions that take a timeout in jiffies. Not making a > judgement on whether that is a good or bad reason.. We intentionally switched away from that to be in sync with the wait_event_* interface. Returning 1 when a function with a zero timeout succeeds is actually quite common in the kernel. Regards, Christian. > BR, > -R > >> -Chris > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoting Christian König (2017-11-21 15:49:55) > Am 21.11.2017 um 15:59 schrieb Rob Clark: > > On Tue, Nov 21, 2017 at 9:38 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> Quoting Rob Clark (2017-11-21 14:08:46) > >>> If we are testing if a reservation object's fences have been > >>> signaled with timeout=0 (non-blocking), we need to pass 0 for > >>> timeout to dma_fence_wait_timeout(). > >>> > >>> Plus bonus spelling correction. > >>> > >>> Signed-off-by: Rob Clark <robdclark@gmail.com> > >>> --- > >>> drivers/dma-buf/reservation.c | 11 +++++++++-- > >>> 1 file changed, 9 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > >>> index dec3a815455d..71f51140a9ad 100644 > >>> --- a/drivers/dma-buf/reservation.c > >>> +++ b/drivers/dma-buf/reservation.c > >>> @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu); > >>> * > >>> * RETURNS > >>> * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or > >>> - * greater than zer on success. > >>> + * greater than zero on success. > >>> */ > >>> long reservation_object_wait_timeout_rcu(struct reservation_object *obj, > >>> bool wait_all, bool intr, > >>> @@ -483,7 +483,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, > >>> goto retry; > >>> } > >>> > >>> - ret = dma_fence_wait_timeout(fence, intr, ret); > >>> + /* > >>> + * Note that dma_fence_wait_timeout() will return 1 if > >>> + * the fence is already signaled, so in the wait_all > >>> + * case when we go through the retry loop again, ret > >>> + * will be greater than 0 and we don't want this to > >>> + * cause _wait_timeout() to block > >>> + */ > >>> + ret = dma_fence_wait_timeout(fence, intr, timeout ? ret : 0); > >> One should ask if we should just fix the interface to stop returning > >> incorrect results (stop "correcting" a completion with 0 jiffies remaining > >> as 1). A timeout can be distinguished by -ETIME (or your pick of errno). > > perhaps -EBUSY, if we go that route (although maybe it should be a > > follow-on patch, this one is suitable for backport to stable/lts if > > one should so choose..) > > > > I think current approach was chosen to match schedule_timeout() and > > other such functions that take a timeout in jiffies. Not making a > > judgement on whether that is a good or bad reason.. > > We intentionally switched away from that to be in sync with the > wait_event_* interface. > > Returning 1 when a function with a zero timeout succeeds is actually > quite common in the kernel. We actually had this conversation last time, and outside of that it isn't. Looking at all the convolution to first return 1, then undo the damage in the caller, it looks pretty silly. -Chris
Am 21.11.2017 um 16:58 schrieb Chris Wilson: > Quoting Christian König (2017-11-21 15:49:55) >> Am 21.11.2017 um 15:59 schrieb Rob Clark: >>> On Tue, Nov 21, 2017 at 9:38 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>> Quoting Rob Clark (2017-11-21 14:08:46) >>>>> If we are testing if a reservation object's fences have been >>>>> signaled with timeout=0 (non-blocking), we need to pass 0 for >>>>> timeout to dma_fence_wait_timeout(). >>>>> >>>>> Plus bonus spelling correction. >>>>> >>>>> Signed-off-by: Rob Clark <robdclark@gmail.com> >>>>> --- >>>>> drivers/dma-buf/reservation.c | 11 +++++++++-- >>>>> 1 file changed, 9 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c >>>>> index dec3a815455d..71f51140a9ad 100644 >>>>> --- a/drivers/dma-buf/reservation.c >>>>> +++ b/drivers/dma-buf/reservation.c >>>>> @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu); >>>>> * >>>>> * RETURNS >>>>> * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or >>>>> - * greater than zer on success. >>>>> + * greater than zero on success. >>>>> */ >>>>> long reservation_object_wait_timeout_rcu(struct reservation_object *obj, >>>>> bool wait_all, bool intr, >>>>> @@ -483,7 +483,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, >>>>> goto retry; >>>>> } >>>>> >>>>> - ret = dma_fence_wait_timeout(fence, intr, ret); >>>>> + /* >>>>> + * Note that dma_fence_wait_timeout() will return 1 if >>>>> + * the fence is already signaled, so in the wait_all >>>>> + * case when we go through the retry loop again, ret >>>>> + * will be greater than 0 and we don't want this to >>>>> + * cause _wait_timeout() to block >>>>> + */ >>>>> + ret = dma_fence_wait_timeout(fence, intr, timeout ? ret : 0); >>>> One should ask if we should just fix the interface to stop returning >>>> incorrect results (stop "correcting" a completion with 0 jiffies remaining >>>> as 1). A timeout can be distinguished by -ETIME (or your pick of errno). >>> perhaps -EBUSY, if we go that route (although maybe it should be a >>> follow-on patch, this one is suitable for backport to stable/lts if >>> one should so choose..) >>> >>> I think current approach was chosen to match schedule_timeout() and >>> other such functions that take a timeout in jiffies. Not making a >>> judgement on whether that is a good or bad reason.. >> We intentionally switched away from that to be in sync with the >> wait_event_* interface. >> >> Returning 1 when a function with a zero timeout succeeds is actually >> quite common in the kernel. > We actually had this conversation last time, and outside of that it > isn't. Looking at all the convolution to first return 1, then undo the > damage in the caller, it looks pretty silly. I don't find that very intuitive either, but you would also have to handle the error code in the calling function as well. And it is what the whole kernel does all over the place with it's wait_event_* and scheduler timeouts as well. Regards, Christian. > -Chris
diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index dec3a815455d..71f51140a9ad 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -420,7 +420,7 @@ EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu); * * RETURNS * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or - * greater than zer on success. + * greater than zero on success. */ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, bool wait_all, bool intr, @@ -483,7 +483,14 @@ long reservation_object_wait_timeout_rcu(struct reservation_object *obj, goto retry; } - ret = dma_fence_wait_timeout(fence, intr, ret); + /* + * Note that dma_fence_wait_timeout() will return 1 if + * the fence is already signaled, so in the wait_all + * case when we go through the retry loop again, ret + * will be greater than 0 and we don't want this to + * cause _wait_timeout() to block + */ + ret = dma_fence_wait_timeout(fence, intr, timeout ? ret : 0); dma_fence_put(fence); if (ret > 0 && wait_all && (i + 1 < shared_count)) goto retry;