Re: [Qemu-block] [PATCH 2/5] coroutine: abort if we try to enter coroutine scheduled for another ctx

[Jeff Cody]

On Mon, Nov 20, 2017 at 11:28:26AM +0000, Stefan Hajnoczi wrote:
> On Sun, Nov 19, 2017 at 09:46:43PM -0500, Jeff Cody wrote:
> > diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
> > index cb98892..931cdc9 100644
> > --- a/include/qemu/coroutine_int.h
> > +++ b/include/qemu/coroutine_int.h
> > @@ -53,6 +53,9 @@ struct Coroutine {
> >  
> >      /* Only used when the coroutine has yielded.  */
> >      AioContext *ctx;
> > +
> > +    int scheduled;
> 
> s/int/bool/
> 

OK.

> > diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> > index d6095c1..2edab63 100644
> > --- a/util/qemu-coroutine.c
> > +++ b/util/qemu-coroutine.c
> > @@ -109,6 +109,15 @@ void qemu_aio_coroutine_enter(AioContext *ctx, 
> > Coroutine *co)
> >  
> >      trace_qemu_aio_coroutine_enter(ctx, self, co, co->entry_arg);
> >  
> > +    /* if the Coroutine has already been scheduled, entering it again will
> > +     * cause us to enter it twice, potentially even after the coroutine has
> > +     * been deleted */
> > +    if (co->scheduled == 1) {
> > +        fprintf(stderr, "Cannot enter a co-routine that has already "
> > +                        "been scheduled to run in a different 
> > AioContext\n");
> 
> This error message is too specific, the AioContext doesn't have to be
> different from the current one:
> 
> block/blkdebug.c:        aio_co_schedule(qemu_get_current_aio_context(), 
> qemu_coroutine_self());
> 
> If something calls qemu_aio_coroutine_enter() on the coroutine it might
> be from the same AioContext - but still an error condition worth failing
> loudly on.
> 

Good point.


> I suggest simplifying the error message:
> 
>   fprintf(stderr, "Cannot enter a co-routine that has already "
>                   "been scheduled\n");

OK. I'll also change the wording here to what you have above, as well:

  void aio_co_schedule(AioContext *ctx, Coroutine *co)
  {
       trace_aio_co_schedule(ctx, co);
  +    if (co->scheduled == 1) {
  +        fprintf(stderr,
  +                "Cannot schedule a co-routine that is already scheduled\n");
  +        abort();
  +    }
  +    co->scheduled = 1;
       QSLIST_INSERT_HEAD_ATOMIC(&ctx->scheduled_coroutines,
                                 co, co_scheduled_next);


Jeff