From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>,
dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
Date: Wed, 15 Jul 2020 15:37:53 +0100 [thread overview]
Message-ID: <159482387319.13728.9618623288194653161@build.alporthouse.com> (raw)
In-Reply-To: <CAKMK7uGtGkYnq+Fe1jD7t315OOgRCiZhqvpTjoOLuYhuV3Qy3A@mail.gmail.com>
Quoting Daniel Vetter (2020-07-15 15:03:34)
> On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > There's a further problem in that we call INIT_LIST_HEAD on the
> > dma_fence_cb before the signal callback. So even if list_empty_careful()
> > confirms the dma_fence_cb to be completely decoupled, the containing
> > struct may still be inuse.
>
> The kerneldoc of dma_fence_remove_callback() already has a very stern
> warning that this will blow up if you don't hold a full reference or
> otherwise control the lifetime of this stuff. So I don't think we have
> to worry about any of that. Or do you mean something else?
It's the struct dma_fence_cb itself that may be freed/reused. Consider
dma_fence_default_wait(). That uses struct default_wait_cb on the stack,
so in order to ensure that the callback is completed the list_empty
check has to remain under the spinlock, or else
dma_fence_default_wait_cb() can still be dereferencing wait->task as the
function returns.
So currently it is:
signed long
dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
struct default_wait_cb cb;
unsigned long flags;
signed long ret = timeout ? timeout : 1;
spin_lock_irqsave(fence->lock, flags);
if (intr && signal_pending(current)) {
ret = -ERESTARTSYS;
goto out;
}
if (!__dma_fence_enable_signaling(fence))
goto out;
if (!timeout) {
ret = 0;
goto out;
}
cb.base.func = dma_fence_default_wait_cb;
cb.task = current;
list_add(&cb.base.node, &fence->cb_list);
while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
if (intr)
__set_current_state(TASK_INTERRUPTIBLE);
else
__set_current_state(TASK_UNINTERRUPTIBLE);
spin_unlock_irqrestore(fence->lock, flags);
ret = schedule_timeout(ret);
spin_lock_irqsave(fence->lock, flags);
if (ret > 0 && intr && signal_pending(current))
ret = -ERESTARTSYS;
}
if (!list_empty(&cb.base.node))
list_del(&cb.base.node);
__set_current_state(TASK_RUNNING);
out:
spin_unlock_irqrestore(fence->lock, flags);
return ret;
}
but it could be written as:
signed long
dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
struct default_wait_cb cb;
int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
cb.task = current;
if (dma_fence_add_callback(fence, &cb.base, dma_fence_default_wait_cb))
return timeout ? timeout : 1;
for (;;) {
set_current_state(state);
if (dma_fence_is_signaled(fence)) {
timeout = timeout ? timeout : 1;
break;
}
if (signal_pending_state(state, current)) {
timeout = -ERESTARTSYS;
break;
}
if (!timeout)
break;
timeout = schedule_timeout(timeout);
}
__set_current_state(TASK_RUNNING);
dma_fence_remove_callback(fence, &cb.base);
return timeout;
}
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-07-15 14:38 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-15 10:49 [PATCH 1/2] dma-buf/dma-fence: Trim dma_fence_add_callback() Chris Wilson
2020-07-15 10:49 ` [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback Chris Wilson
2020-07-15 12:10 ` [Intel-gfx] " Daniel Vetter
2020-07-15 12:21 ` Chris Wilson
2020-07-15 12:40 ` Chris Wilson
2020-07-15 14:03 ` Daniel Vetter
2020-07-15 14:37 ` Chris Wilson [this message]
2020-07-15 14:46 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=159482387319.13728.9618623288194653161@build.alporthouse.com \
--to=chris@chris-wilson.co.uk \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).