Re: [Qemu-devel] [PATCH 04/12] Revert "qmp: isolate responses into io thread"

[Peter Xu]

On Thu, Jul 12, 2018 at 03:32:51PM +0200, Marc-André Lureau wrote:

[...]

> >
> > However, Peter has argued for keeping the response queue:
> >
> >   For my own opinion, I'll see it a bit awkward (as I mentioned) to
> >   revert the response queue part.  Again, it's mostly working well
> >   there, we just fix things up when needed.  It's not really a big part
> >   of code to maintain, and it still looks sane to me to have such an
> >   isolation so that we can have the dispatcher totally separate from the
> >   chardev IO path (I'd say if I design a QMP interface from scratch,
> >   I'll do that from the first day).  So I am not against reverting that
> >   part at all, I just don't see much gain from that as well, especially
> >   if we want to make QMP more modular in the future.
> >
> > The argument worth considering here is "isolating the dispatcher from
> > the chardev IO path".  Opinions?
> 
> Imho, this is extra unneeded work. The queue is necessary on the
> receive side, to dispatch between iothread and main loop, but there
> isn't such need for sending the response stream.
> 
> Also, there is already a sending "buffer queue": it's "outbuf". To me,
> they are duplicating the response buffering, thus making things
> unnecessarily more complicated.

Yeah actually this reminded me about the fact that we are still using
unlimited buffer size for the out_buf.  IMHO we should make it a
limited size after 3.0, then AFAICT if without current qmp response
queue we'll need to introduce some similar thing to cache responses
then when the out_buf is full.

IMHO the response queue looks more like the correct place that we
should do the flow control, and the out_buf could be majorly used to
do the JSON->string convertion (so basically IMHO the out_buf
limitation should be the size of the maximum JSON object that we'll
have).

> 
> If we needed, we could modify monitor_flush_locked() to add a sending
> watch only. This would have mostly the same effect as scheduling the
> bh for processing the qmp_responses queue. But since chardev sending
> is thread-safe, it's not a problem to save the watch handler switch,
> and try sending directly from the iothread.

Actually this worries me a bit on complexity - if we remove the
response queue then the response IO can be run either in main thread
or in iothread (e.g., after we add a watch).  For me, it's not really
as straightforward as we just only run IOs only in the iothread
always.  Again, we'd better do enough testings to make sure it's
always working well.  I just hope we can just avoid bothering with it.

> 
> it's not critical, so we can delay this cleanup to 3.1. (I wish it
> would have been merged early during this cycle, like most of my
> pending series, but hey, we have more important things to do ;)

Fully agree on this.

Thanks!

-- 
Peter Xu