Re: [Qemu-devel] [PATCH 0/9] monitor: enable OOB by default

[Peter Xu <peterx@redhat.com>]

On Mon, Jul 16, 2018 at 07:18:28PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Based-on: <20180703085358.13941-1-armbru@redhat.com>
> 
> Now in master.
> 
> > This work is based on Markus's latest out-of-band fixes:
> >   "[PATCH v2 00/32] ] qmp: Fixes and cleanups around OOB commands"
> >
> > Major stuff: some cleanups that were previously suggested by Markus or
> > Eric.  Meanwhile fix up the flow control issue.  Since my proposal
> > here is to drop COMMAND_DROPPED event, then we don't need to introduce
> > per-monitor event emission API any more.  Though Markus told me that
> > he might use that code somewhere else, so I'll post that per-monitor
> > event code out separately as RFC later.
> >
> > Patch 1-3: cleanups.
> 
> I expect these to be ready in the next version.
> 
> Since it's just cleanup, there's no need to rush these into 3.0 unless
> they enable something we want in 3.0.
> 
> > Patch 4-7: solve the flow control issue reported by Markus that
> > response queue might overflow even if we have limitation on the
> > request queue.  Firstly we drop the COMMAND_DROP event since that
> > won't work for response queue (since queuing an event will need to
> > append to the response queue itself), then we use monitor suspend and
> > resume to control the flow.  Last, we apply that to response queue
> > too.
> 
> These need work.  We need to agree on how exactly flow control should
> work.  Moreover, we need to reconcile your work with Marc-André's
> "[PATCH 00/12] RFC: monitor: various code simplification and fixes"
> (which I haven't fully reviewed yet).

Sure.

> 
> > Patch 8-9: the original patches to enable out-of-band by default.
> 
> I figure these patches are good; we just need to decide whether we're
> ready to enable OOB.  Let's review the known issues.
> 
> * We might want to make "id" mandatory with exec-oob
> 
>   Best to do that right in the first release that fully supports OOB.

Yeah, I'd say I would prefer it that way, but after all we're having
that optional now in master, so it's fine for me in either way.

> 
> * OOB offered but rejected by client is not obviously the same as
>   OOB not offered
> 
>   I still need to digest and reply to your
>   Message-ID: <20180629090115.GH2455@xz-mi>

As a summary of the reply - I think the only difference is where we
run the chardev IOs for the monitor:

- When OOB not offered: we run chardev IOs in main thread

- When OOB offered but client rejected: we run chardev IOs in iothread

The rest of the things should be all the same.

> 
> * COMMAND_DROPPED is broken by design, and
> * Flow control limits only request queue; response buffer can still
>   grow without bounds
> 
>   You proposed to drop COMMAND_DROPPED, and do flow control by corking
>   input, see above.
> 
>   Getting rid of broken COMMAND_DROPPED is a blocker.  Fully functional
>   flow control isn't, since the QMP client is trusted.
> 
> * We lack test coverage for flow control
> * test-qmp-cmds neglects to cover the OOB additions to qmp-dispatch.c
> 
>   I'm inclined to treating lack of test coverage as a blocker.

I will look at this one before repost after 3.0.

> 
> * scripts/qmp/ doesn't support OOB, yet.  qmp-shell.py in particular.
> 
>   Not a blocker.

I'll possibly temporarily put this one aside.  If not, I'll leave a
FIXME there (or I'll just do).

> 
> Whatever we don't address right away we should at least mark FIXME in
> the source code.
> 
> Assuming my list is complete, and my assessments correct, then we're
> quite close to the point where we can enable OOB.  But since rc1 is
> tomorrow already, I feel enabling it in 3.0 has become unrealistic.  I
> understand we need it sooner rather than later for postcopy recovery.
> However, the current state should be servicable for teaching OOB to
> libvirt: just follow the recommendation to supply "id" (libvirt always
> does anyway), pretend COMMAND_DROPPED doesn't exist, and pretend flow
> control isn't an issue.

I guess the thing is not about COMMAND_DROPPED; it's about we're going
to drop the x-oob parameter.  Now we can only use that parameter to
enable out-of-band, and after we enable it by default we possibly want
to remove that x-oob parameter.  So we'd better provide a constant way
for libvirt to enable out-of-band first (and now I'll assume the
"exec-oob" interface won't change again).  But yes I think we can do
that after 3.0.

Thanks,

-- 
Peter Xu