[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.11 3/4] coroutine: Cancel aio_co_schedule(
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH for-2.11 3/4] coroutine: Cancel aio_co_schedule() on direct entry |
Date: |
Tue, 28 Nov 2017 12:09:02 -0500 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Tue, Nov 28, 2017 at 05:51:21PM +0100, Paolo Bonzini wrote:
> On 28/11/2017 17:42, Jeff Cody wrote:
> > On Tue, Nov 28, 2017 at 05:28:50PM +0100, Kevin Wolf wrote:
> >> Am 28.11.2017 um 17:14 hat Paolo Bonzini geschrieben:
> >>> On 28/11/2017 16:43, Kevin Wolf wrote:
> >>>> + /* Make sure that a coroutine that can alternatively reentered from
> >>>> two
> >>>> + * different sources isn't reentered more than once when the first
> >>>> caller
> >>>> + * uses aio_co_schedule() and the other one enters to coroutine
> >>>> directly.
> >>>> + * This is achieved by cancelling the pending aio_co_schedule().
> >>>> + *
> >>>> + * The other way round, if aio_co_schedule() would be called after
> >>>> this
> >>>> + * point, this would be a problem, too, but in practice it doesn't
> >>>> happen
> >>>> + * because we're holding the AioContext lock here and
> >>>> aio_co_schedule()
> >>>> + * callers must do the same.
> >>>
> >>> No, this is not true. aio_co_schedule is thread-safe.
> >>
> >> Hm... With the reproducer we were specfically looking at
> >> qmp_block_job_cancel(), which does take the AioContext locks. But it
> >> might not be as universal as I thought.
> >>
> >> To be honest, I just wasn't sure what to do with this case anyway. It
> >> means that the coroutine is already running when someone else schedules
> >> it. We don't really know whether we have to enter it a second time or
> >> not.
> >>
> >> So if it can indeed happen in practice, we need to think a bit more
> >> about this.
> >
> > It would be nice if, on coroutine termination, we could unschedule all
> > pending executions for that coroutine. I think use-after-free is the main
> > concern for someone else calling aio_co_schedule() while the coroutine is
> > currently running.
>
> Yes, terminating a scheduled coroutine is a bug; same for scheduling a
> terminated coroutine, both orders are wrong. However, "unscheduling" is
> not the solution; you would just be papering over the issue.
>
Maybe we should at least add an abort on coroutine termination if there are
still outstanding schedules, as that is preferable to operating in the
weeds.
> aio_co_schedule() on a running coroutine can only happen when the
> coroutine is going to yield soon.
>
That is a bit vague. What is "soon", and how does an external caller know
if a coroutine is going to yield in this timeframe?
Jeff
- Re: [Qemu-devel] [PATCH for-2.11 1/4] Revert "coroutine: abort if we try to schedule or enter a pending coroutine", (continued)
[Qemu-devel] [PATCH for-2.11 3/4] coroutine: Cancel aio_co_schedule() on direct entry, Kevin Wolf, 2017/11/28
Re: [Qemu-devel] [PATCH for-2.11 3/4] coroutine: Cancel aio_co_schedule() on direct entry, Kevin Wolf, 2017/11/28
Re: [Qemu-devel] [PATCH for-2.11 3/4] coroutine: Cancel aio_co_schedule() on direct entry, Paolo Bonzini, 2017/11/28
Re: [Qemu-devel] [PATCH for-2.11 3/4] coroutine: Cancel aio_co_schedule() on direct entry, Fam Zheng, 2017/11/28
Re: [Qemu-devel] [PATCH for-2.11 3/4] coroutine: Cancel aio_co_schedule() on direct entry, Eric Blake, 2017/11/28
[Qemu-devel] [PATCH for-2.11 2/4] Revert "blockjob: do not allow coroutine double entry or entry-after-completion", Kevin Wolf, 2017/11/28
[Qemu-devel] [PATCH for-2.11 4/4] block: Expect graph changes in bdrv_parent_drained_begin/end, Kevin Wolf, 2017/11/28
Re: [Qemu-devel] [PATCH for-2.11 0/4] Fix qemu-iotests failures, Jeff Cody, 2017/11/28