All of lore.kernel.org
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-block@nongnu.org, marcandre.lureau@gmail.com,
	qemu-devel@nongnu.org, stefanha@redhat.com
Subject: Re: [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands
Date: Wed, 22 Jan 2020 13:15:06 +0100	[thread overview]
Message-ID: <874kwnvgad.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20200122101021.GB5268@linux.fritz.box> (Kevin Wolf's message of "Wed, 22 Jan 2020 11:10:21 +0100")

Kevin Wolf <kwolf@redhat.com> writes:

> Am 22.01.2020 um 07:32 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > This patch adds a new 'coroutine' flag to QMP command definitions that
>> > tells the QMP dispatcher that the command handler is safe to be run in a
>> > coroutine.
>> 
>> I'm afraid I missed this question in my review of v3: when is a handler
>> *not* safe to be run in a coroutine?
>
> That's a hard one to answer fully.

You're welcome ;)

> Basically, I think the biggest problem is with calling functions that
> change their behaviour if run in a coroutine compared to running them
> outside of coroutine context. In most cases the differences like having
> a nested event loop instead of yielding are just fine, but they are
> still subtly different.
>
> I know this is vague, but I can assure you that problematic cases exist.
> I hit one of them with my initial hack that just moved everything into a
> coroutine. It was related to graph modifications and bdrv_drain and
> resulted in a hang. For the specifics, I would have to try and reproduce
> the problem again.

Interesting.

Is coroutine-incompatible command handler code necessary or accidental?

By "necessary" I mean there are (and likely always will be) commands
that need to do stuff that cannot or should not be done on coroutine
context.

"Accidental" is the opposite: coroutine-incompatibility can be regarded
as a fixable flaw.

How widespread is coroutine-incompatibility?  Common, or just a few
commands?

If coroutine-incompatibility is accidental, then your code to drop out
of coroutine context can be regarded as a temporary work-around.  Such
work-arounds receive a modest extra ugliness & complexity budget.

If coroutine-incompatibility is rare, we'll eventually want "mark the
known-bad ones with 'coroutine': false" instead of "mark the known-good
ones with 'coroutine': true".  I'm okay with marking the known-good ones
initially, and flipping only later.

Inability to recognize coroutine-incompatibility by inspection worries
me.  Can you think of ways to identify causes other than testing things
to death?



  reply	other threads:[~2020-01-22 12:16 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-21 18:11 [PATCH v4 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
2020-01-21 18:11 ` [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf
2020-01-22  6:32   ` Markus Armbruster
2020-01-22 10:10     ` Kevin Wolf
2020-01-22 12:15       ` Markus Armbruster [this message]
2020-01-22 14:35         ` Kevin Wolf
2020-03-05 15:30       ` Markus Armbruster
2020-03-05 16:06         ` Kevin Wolf
2020-03-06  7:25           ` Markus Armbruster
2020-03-06  9:52             ` Kevin Wolf
2020-03-06 12:38               ` Markus Armbruster
2020-03-06 14:26                 ` Kevin Wolf
2020-02-17  7:08   ` Markus Armbruster
2020-01-21 18:11 ` [PATCH v4 2/4] vl: Initialise main loop earlier Kevin Wolf
2020-01-21 18:11 ` [PATCH v4 3/4] qmp: Move dispatcher to a coroutine Kevin Wolf
2020-02-17 11:08   ` Markus Armbruster
2020-02-17 12:34     ` Kevin Wolf
2020-02-18 14:12       ` Markus Armbruster
2020-02-18 15:29         ` Kevin Wolf
2020-02-19  9:03           ` Markus Armbruster
2020-02-19 10:22             ` Kevin Wolf
2020-02-19 14:21               ` Markus Armbruster
2020-02-19 14:26                 ` Markus Armbruster
2020-02-19 15:27                 ` Kevin Wolf
2020-01-21 18:11 ` [PATCH v4 4/4] block: Mark 'block_resize' as coroutine Kevin Wolf
2020-02-05 14:00 ` [PATCH v4 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
2020-02-12 11:40   ` Kevin Wolf

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=874kwnvgad.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=marcandre.lureau@gmail.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.