| Message ID | 1477398311-5218-2-git-send-email-deathsimple@vodafone.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 25, 2016 at 02:25:08PM +0200, Christian König wrote: > From: Christian König <christian.koenig@amd.com> > > Kernel functions taking a timeout usually return 1 on success even > when they get a zero timeout. Which? The canonical example of schedule_timeout() doesn't behave like this. > Signen-off-by: Christian König <christian.koenig@amd.com> > Reviewed-by: Chunming Zhou <david1.zhou@amd.com> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/dma-buf/fence.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c > index 4d51f9e..fb915ab 100644 > --- a/drivers/dma-buf/fence.c > +++ b/drivers/dma-buf/fence.c > @@ -335,18 +335,20 @@ fence_default_wait_cb(struct fence *fence, struct fence_cb *cb) > * @timeout: [in] timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT > * > * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the > - * remaining timeout in jiffies on success. > + * remaining timeout in jiffies on success. If timeout is zero the value one is > + * returned if the fence is already signaled for consistency with other > + * functions taking a jiffies timeout. > */ > signed long > fence_default_wait(struct fence *fence, bool intr, signed long timeout) > { > struct default_wait_cb cb; > unsigned long flags; > - signed long ret = timeout; > + signed long ret = timeout ? timeout : 1; > bool was_set; This is horrible. reservation_object_wait_timeout_rcu(), we pass it a timeout of zero to do a safe probe on all attached fences. The first fence happens to be signaled, so now we set the timeout to 1 jiffie. Any subsequent incomplete fence will then wait for that jiffie before reporting 0. When we report to userspace, we convert the timeout into an error, usually EBUSY or ETIME. Applying that same convention here makes this all much cleaner... -Chris
Am 26.10.2016 um 10:49 schrieb Chris Wilson: > On Tue, Oct 25, 2016 at 02:25:08PM +0200, Christian König wrote: >> From: Christian König <christian.koenig@amd.com> >> >> Kernel functions taking a timeout usually return 1 on success even >> when they get a zero timeout. > Which? The canonical example of schedule_timeout() doesn't behave like > this. wait_event_timeout() for example, as well as to the fence_wait_timeout() and reservation_object_wait_timeout_rcu() functions build on top of this one. Regards, Christian. > >> Signen-off-by: Christian König <christian.koenig@amd.com> >> Reviewed-by: Chunming Zhou <david1.zhou@amd.com> >> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> >> --- >> drivers/dma-buf/fence.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c >> index 4d51f9e..fb915ab 100644 >> --- a/drivers/dma-buf/fence.c >> +++ b/drivers/dma-buf/fence.c >> @@ -335,18 +335,20 @@ fence_default_wait_cb(struct fence *fence, struct fence_cb *cb) >> * @timeout: [in] timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT >> * >> * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the >> - * remaining timeout in jiffies on success. >> + * remaining timeout in jiffies on success. If timeout is zero the value one is >> + * returned if the fence is already signaled for consistency with other >> + * functions taking a jiffies timeout. >> */ >> signed long >> fence_default_wait(struct fence *fence, bool intr, signed long timeout) >> { >> struct default_wait_cb cb; >> unsigned long flags; >> - signed long ret = timeout; >> + signed long ret = timeout ? timeout : 1; >> bool was_set; > This is horrible. > > reservation_object_wait_timeout_rcu(), we pass it a timeout of zero to > do a safe probe on all attached fences. > > The first fence happens to be signaled, so now we set the timeout to 1 > jiffie. Any subsequent incomplete fence will then wait for that jiffie > before reporting 0. > > When we report to userspace, we convert the timeout into an error, > usually EBUSY or ETIME. Applying that same convention here makes this > all much cleaner... > -Chris >
diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c index 4d51f9e..fb915ab 100644 --- a/drivers/dma-buf/fence.c +++ b/drivers/dma-buf/fence.c @@ -335,18 +335,20 @@ fence_default_wait_cb(struct fence *fence, struct fence_cb *cb) * @timeout: [in] timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT * * Returns -ERESTARTSYS if interrupted, 0 if the wait timed out, or the - * remaining timeout in jiffies on success. + * remaining timeout in jiffies on success. If timeout is zero the value one is + * returned if the fence is already signaled for consistency with other + * functions taking a jiffies timeout. */ signed long fence_default_wait(struct fence *fence, bool intr, signed long timeout) { struct default_wait_cb cb; unsigned long flags; - signed long ret = timeout; + signed long ret = timeout ? timeout : 1; bool was_set; if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags)) - return timeout; + return ret; spin_lock_irqsave(fence->lock, flags);