Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Tue, Sep 19, 2017 at 10:19:21AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Mon, Sep 18, 2017 at 06:09:29PM +0200, Marc-André Lureau wrote:
> > > On Mon, Sep 18, 2017 at 1:26 PM, Dr. David Alan Gilbert
> > >  wrote:
> > > > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > >> Hi
> > > >>
> > > >> On Mon, Sep 18, 2017 at 12:55 PM, Dr. David Alan Gilbert
> > > >>  wrote:
> > > >> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > >> >> Hi
> > > >> >>
> > > >> >> On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  
> > > >> >> wrote:
> > > >> >> > On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
> > > >> >> >> Hi
> > > >> >> >>
> > > >> >> >> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  
> > > >> >> >> wrote:
> > > >> >> >> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan 
> > > >> >> >> > Gilbert wrote:
> > > >> >> >> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > >> >> >> >> > Hi
> > > >> >> >> >> >
> > > >> >> >> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu 
> > > >> >> >> >> >  wrote:
> > > >> >> >> >> > > This series was born from this one:
> > > >> >> >> >> > >
> > > >> >> >> >> > >   
> > > >> >> >> >> > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> > > >> >> >> >> > >
> > > >> >> >> >> > > The design comes from Markus, and also the whole-bunch-of 
> > > >> >> >> >> > > discussions
> > > >> >> >> >> > > in previous thread.  My heartful thanks to Markus, 
> > > >> >> >> >> > > Daniel, Dave,
> > > >> >> >> >> > > Stefan, etc. on discussing the topic (...again!), 
> > > >> >> >> >> > > providing shiny
> > > >> >> >> >> > > ideas and suggestions.  Finally we got such a solution 
> > > >> >> >> >> > > that seems to
> > > >> >> >> >> > > satisfy everyone.
> > > >> >> >> >> > >
> > > >> >> >> >> > > I re-started the versioning since this series is totally 
> > > >> >> >> >> > > different
> > > >> >> >> >> > > from previous one.  Now it's version 1.
> > > >> >> >> >> > >
> > > >> >> >> >> > > In case new reviewers come along the way without reading 
> > > >> >> >> >> > > previous
> > > >> >> >> >> > > discussions, I will try to do a summary on what this is 
> > > >> >> >> >> > > all about.
> > > >> >> >> >> > >
> > > >> >> >> >> > > What is OOB execution?
> > > >> >> >> >> > > ==
> > > >> >> >> >> > >
> > > >> >> >> >> > > It's the shortcut of Out-Of-Band execution, its name is 
> > > >> >> >> >> > > given by
> > > >> >> >> >> > > Markus.  It's a way to quickly execute a QMP request.  
> > > >> >> >> >> > > Say, originally
> > > >> >> >> >> > > QMP is going throw these steps:
> > > >> >> >> >> > >
> > > >> >> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> > > >> >> >> >> > >   /|\(2)(3) |
> > > >> >> >> >> > >(1) |   \|/ (4)
> > > >> >> >> >> > >+-  main thread  +
> > > >> >> >> >> > >
> > > >> >> >> >> > > The requests are executed by the so-called QMP-dispatcher 
> > > >> >> >> >> > > after the
> > > >> >> >> >> > > JSON is parsed.  If OOB is on, we run the command 
> > > >> >> >> >> > > directly in the
> > > >> >> >> >> > > parser and quickly returns.
> > > >> >> >> >> >
> > > >> >> >> >> > All commands should have the "id" field mandatory in this 
> > > >> >> >> >> > case, else
> > > >> >> >> >> > the client will not distinguish the replies coming from the 
> > > >> >> >> >> > last/oob
> > > >> >> >> >> > and the previous commands.
> > > >> >> >> >> >
> > > >> >> >> >> > This should probably be enforced upfront by client 
> > > >> >> >> >> > capability checks,
> > > >> >> >> >> > more below.
> > > >> >> >> >
> > > >> >> >> > Hmm yes since the oob commands are actually running in async 
> > > >> >> >> > way,
> > > >> >> >> > request ID should be needed here.  However I'm not sure whether
> > > >> >> >> > enabling the whole "request ID" thing is too big for this "try 
> > > >> >> >> > to be
> > > >> >> >> > small" oob change... And IMHO it suites better to be part of 
> > > >> >> >> > the whole
> > > >> >> >> > async work (no matter which implementation we'll use).
> > > >> >> >> >
> > > >> >> >> > How about this: we make "id" mandatory for "run-oob" requests 
> > > >> >> >> > only.
> > > >> >> >> > For oob commands, they will always have ID then no ordering 
> > > >> >> >> > issue, and
> > > >> >> >> > we can do it async; for the rest of non-oob commands, we still 
> > > >> >> >> > allow
> > > >> >> >> > them to go without ID, and since they are not oob, they'll 
> > > >> >> >> > always be
> > > >> >> >> > done in order as well.  Would this work?
> > > >> >> >>
> > > >> >> >> This mixed-mode is imho more complicated to deal with than 
> > > >> >> >> having the
> > > >> >> >> protocol enforced one way or the other, but that should work.
> > > >> >> >>
> 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> On Mon, Sep 18, 2017 at 1:26 PM, Dr. David Alan Gilbert
>  wrote:
> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> Hi
> >>
> >> On Mon, Sep 18, 2017 at 12:55 PM, Dr. David Alan Gilbert
> >>  wrote:
> >> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> >> Hi
> >> >>
> >> >> On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  wrote:
> >> >> > On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
> >> >> >> Hi
> >> >> >>
> >> >> >> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
> >> >> >> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert 
> >> >> >> > wrote:
> >> >> >> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> >> >> >> > Hi
> >> >> >> >> >
> >> >> >> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  
> >> >> >> >> > wrote:
> >> >> >> >> > > This series was born from this one:
> >> >> >> >> > >
> >> >> >> >> > >   
> >> >> >> >> > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> >> >> >> >> > >
> >> >> >> >> > > The design comes from Markus, and also the whole-bunch-of 
> >> >> >> >> > > discussions
> >> >> >> >> > > in previous thread.  My heartful thanks to Markus, Daniel, 
> >> >> >> >> > > Dave,
> >> >> >> >> > > Stefan, etc. on discussing the topic (...again!), providing 
> >> >> >> >> > > shiny
> >> >> >> >> > > ideas and suggestions.  Finally we got such a solution that 
> >> >> >> >> > > seems to
> >> >> >> >> > > satisfy everyone.
> >> >> >> >> > >
> >> >> >> >> > > I re-started the versioning since this series is totally 
> >> >> >> >> > > different
> >> >> >> >> > > from previous one.  Now it's version 1.
> >> >> >> >> > >
> >> >> >> >> > > In case new reviewers come along the way without reading 
> >> >> >> >> > > previous
> >> >> >> >> > > discussions, I will try to do a summary on what this is all 
> >> >> >> >> > > about.
> >> >> >> >> > >
> >> >> >> >> > > What is OOB execution?
> >> >> >> >> > > ==
> >> >> >> >> > >
> >> >> >> >> > > It's the shortcut of Out-Of-Band execution, its name is given 
> >> >> >> >> > > by
> >> >> >> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, 
> >> >> >> >> > > originally
> >> >> >> >> > > QMP is going throw these steps:
> >> >> >> >> > >
> >> >> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> >> >> >> >> > >   /|\(2)(3) |
> >> >> >> >> > >(1) |   \|/ (4)
> >> >> >> >> > >+-  main thread  +
> >> >> >> >> > >
> >> >> >> >> > > The requests are executed by the so-called QMP-dispatcher 
> >> >> >> >> > > after the
> >> >> >> >> > > JSON is parsed.  If OOB is on, we run the command directly in 
> >> >> >> >> > > the
> >> >> >> >> > > parser and quickly returns.
> >> >> >> >> >
> >> >> >> >> > All commands should have the "id" field mandatory in this case, 
> >> >> >> >> > else
> >> >> >> >> > the client will not distinguish the replies coming from the 
> >> >> >> >> > last/oob
> >> >> >> >> > and the previous commands.
> >> >> >> >> >
> >> >> >> >> > This should probably be enforced upfront by client capability 
> >> >> >> >> > checks,
> >> >> >> >> > more below.
> >> >> >> >
> >> >> >> > Hmm yes since the oob commands are actually running in async way,
> >> >> >> > request ID should be needed here.  However I'm not sure whether
> >> >> >> > enabling the whole "request ID" thing is too big for this "try to 
> >> >> >> > be
> >> >> >> > small" oob change... And IMHO it suites better to be part of the 
> >> >> >> > whole
> >> >> >> > async work (no matter which implementation we'll use).
> >> >> >> >
> >> >> >> > How about this: we make "id" mandatory for "run-oob" requests only.
> >> >> >> > For oob commands, they will always have ID then no ordering issue, 
> >> >> >> > and
> >> >> >> > we can do it async; for the rest of non-oob commands, we still 
> >> >> >> > allow
> >> >> >> > them to go without ID, and since they are not oob, they'll always 
> >> >> >> > be
> >> >> >> > done in order as well.  Would this work?
> >> >> >>
> >> >> >> This mixed-mode is imho more complicated to deal with than having the
> >> >> >> protocol enforced one way or the other, but that should work.
> >> >> >>
> >> >> >> >
> >> >> >> >> >
> >> >> >> >> > > Yeah I know in current code the parser calls dispatcher 
> >> >> >> >> > > directly
> >> >> >> >> > > (please see handle_qmp_command()).  However it's not true 
> >> >> >> >> > > again after
> >> >> >> >> > > this series (parser will has its own IO thread, and 
> >> >> >> >> > > dispatcher will
> >> >> >> >> > > still be run in main thread).  So this OOB does brings 
> >> >> >> >> > > something
> >> >> >> >> > > different.
> >> >> >> >> > >
> >> >> >> >> > > There are more details on why OOB and the 
> >> >> >> >> > > 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Tue, Sep 19, 2017 at 10:13:52AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Mon, Sep 18, 2017 at 11:40:40AM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (pet...@redhat.com) wrote:
> > > > On Fri, Sep 15, 2017 at 04:17:07PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > > > > > On Fri, Sep 15, 2017 at 01:29:13PM +0100, Daniel P. Berrange wrote:
> > > > > > > On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert 
> > > > > > > wrote:
> > > > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan 
> > > > > > > > > Gilbert wrote:
> > > > > > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu 
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan 
> > > > > > > > > > > > > Hajnoczi wrote:
> > > > > > > > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, 
> > > > > > > > > > > > > > Marc-André Lureau wrote:
> > > > > > > > > > > > > > > There should be a limit in the number of requests 
> > > > > > > > > > > > > > > the thread can
> > > > > > > > > > > > > > > queue. Before the patch, the limit was enforced 
> > > > > > > > > > > > > > > by system socket
> > > > > > > > > > > > > > > buffering I think. Now, should oob commands still 
> > > > > > > > > > > > > > > be processed even if
> > > > > > > > > > > > > > > the queue is full? If so, the thread can't be 
> > > > > > > > > > > > > > > suspended.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I agree.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Memory usage must be bounded.  The number of 
> > > > > > > > > > > > > > requests is less important
> > > > > > > > > > > > > > than the amount of memory consumed by them.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Existing QMP clients that send multiple QMP 
> > > > > > > > > > > > > > commands without waiting for
> > > > > > > > > > > > > > replies need to rethink their strategy because OOB 
> > > > > > > > > > > > > > commands cannot be
> > > > > > > > > > > > > > processed if queued non-OOB commands consume too 
> > > > > > > > > > > > > > much memory.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Thanks for pointing out this.  Yes the memory usage 
> > > > > > > > > > > > > problem is valid,
> > > > > > > > > > > > > as Markus pointed out as well in previous discussions 
> > > > > > > > > > > > > (in "Flow
> > > > > > > > > > > > > Control" section of that long reply).  Hopefully this 
> > > > > > > > > > > > > series basically
> > > > > > > > > > > > > can work from design prospective, then I'll add this 
> > > > > > > > > > > > > flow control in
> > > > > > > > > > > > > next version.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Regarding to what we should do if the limit is 
> > > > > > > > > > > > > reached: Markus
> > > > > > > > > > > > > provided a few options, but the one I prefer most is 
> > > > > > > > > > > > > that we don't
> > > > > > > > > > > > > respond, but send an event showing that a command is 
> > > > > > > > > > > > > dropped.
> > > > > > > > > > > > > However, I would like it not queued, but a direct 
> > > > > > > > > > > > > reply (after all,
> > > > > > > > > > > > > it's an event, and we should not need to care much on 
> > > > > > > > > > > > > ordering of it).
> > > > > > > > > > > > > Then we can get rid of the babysitting of those "to 
> > > > > > > > > > > > > be failed"
> > > > > > > > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I think I also missed at least a unit test for this 
> > > > > > > > > > > > > new interface.
> > > > > > > > > > > > > Again, I'll add it after the whole idea is proved 
> > > > > > > > > > > > > solid.  Thanks,
> > > > > > > > > > > > 
> > > > > > > > > > > > Another solution: the server reports available receive 
> > > > > > > > > > > > buffer space to
> > > > > > > > > > > > the client.  The server only guarantees immediate OOB 
> > > > > > > > > > > > processing when
> > > > > > > > > > > > the client stays within the receive buffer size.
> > > > > > > > > > > > 
> > > > > > > > > > > > Clients wishing to take advantage of OOB must query the 
> > > > > > > > > > > > receive buffer
> > > > > > > > > > > > size and make sure to leave enough room.
> > > > > > > > > > > 
> > > > > > > > > > > I don't think having to query it ahead of time is 
> > > > > > > > > > > particularly nice,
> > > > > > > > > > > and of course it is inherantly racy.
> > > > > > > > > > > 
> > > > > > > > > > > I would just have QEMU emit an event when it pausing 
> > > > > > > > > > > processing of the
> > > > > > > > > > > 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> On Mon, Sep 18, 2017 at 06:09:29PM +0200, Marc-André Lureau wrote:
> > On Mon, Sep 18, 2017 at 1:26 PM, Dr. David Alan Gilbert
> >  wrote:
> > > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > >> Hi
> > >>
> > >> On Mon, Sep 18, 2017 at 12:55 PM, Dr. David Alan Gilbert
> > >>  wrote:
> > >> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > >> >> Hi
> > >> >>
> > >> >> On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  wrote:
> > >> >> > On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
> > >> >> >> Hi
> > >> >> >>
> > >> >> >> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  
> > >> >> >> wrote:
> > >> >> >> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert 
> > >> >> >> > wrote:
> > >> >> >> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > >> >> >> >> > Hi
> > >> >> >> >> >
> > >> >> >> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  
> > >> >> >> >> > wrote:
> > >> >> >> >> > > This series was born from this one:
> > >> >> >> >> > >
> > >> >> >> >> > >   
> > >> >> >> >> > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> > >> >> >> >> > >
> > >> >> >> >> > > The design comes from Markus, and also the whole-bunch-of 
> > >> >> >> >> > > discussions
> > >> >> >> >> > > in previous thread.  My heartful thanks to Markus, Daniel, 
> > >> >> >> >> > > Dave,
> > >> >> >> >> > > Stefan, etc. on discussing the topic (...again!), providing 
> > >> >> >> >> > > shiny
> > >> >> >> >> > > ideas and suggestions.  Finally we got such a solution that 
> > >> >> >> >> > > seems to
> > >> >> >> >> > > satisfy everyone.
> > >> >> >> >> > >
> > >> >> >> >> > > I re-started the versioning since this series is totally 
> > >> >> >> >> > > different
> > >> >> >> >> > > from previous one.  Now it's version 1.
> > >> >> >> >> > >
> > >> >> >> >> > > In case new reviewers come along the way without reading 
> > >> >> >> >> > > previous
> > >> >> >> >> > > discussions, I will try to do a summary on what this is all 
> > >> >> >> >> > > about.
> > >> >> >> >> > >
> > >> >> >> >> > > What is OOB execution?
> > >> >> >> >> > > ==
> > >> >> >> >> > >
> > >> >> >> >> > > It's the shortcut of Out-Of-Band execution, its name is 
> > >> >> >> >> > > given by
> > >> >> >> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, 
> > >> >> >> >> > > originally
> > >> >> >> >> > > QMP is going throw these steps:
> > >> >> >> >> > >
> > >> >> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> > >> >> >> >> > >   /|\(2)(3) |
> > >> >> >> >> > >(1) |   \|/ (4)
> > >> >> >> >> > >+-  main thread  +
> > >> >> >> >> > >
> > >> >> >> >> > > The requests are executed by the so-called QMP-dispatcher 
> > >> >> >> >> > > after the
> > >> >> >> >> > > JSON is parsed.  If OOB is on, we run the command directly 
> > >> >> >> >> > > in the
> > >> >> >> >> > > parser and quickly returns.
> > >> >> >> >> >
> > >> >> >> >> > All commands should have the "id" field mandatory in this 
> > >> >> >> >> > case, else
> > >> >> >> >> > the client will not distinguish the replies coming from the 
> > >> >> >> >> > last/oob
> > >> >> >> >> > and the previous commands.
> > >> >> >> >> >
> > >> >> >> >> > This should probably be enforced upfront by client capability 
> > >> >> >> >> > checks,
> > >> >> >> >> > more below.
> > >> >> >> >
> > >> >> >> > Hmm yes since the oob commands are actually running in async way,
> > >> >> >> > request ID should be needed here.  However I'm not sure whether
> > >> >> >> > enabling the whole "request ID" thing is too big for this "try 
> > >> >> >> > to be
> > >> >> >> > small" oob change... And IMHO it suites better to be part of the 
> > >> >> >> > whole
> > >> >> >> > async work (no matter which implementation we'll use).
> > >> >> >> >
> > >> >> >> > How about this: we make "id" mandatory for "run-oob" requests 
> > >> >> >> > only.
> > >> >> >> > For oob commands, they will always have ID then no ordering 
> > >> >> >> > issue, and
> > >> >> >> > we can do it async; for the rest of non-oob commands, we still 
> > >> >> >> > allow
> > >> >> >> > them to go without ID, and since they are not oob, they'll 
> > >> >> >> > always be
> > >> >> >> > done in order as well.  Would this work?
> > >> >> >>
> > >> >> >> This mixed-mode is imho more complicated to deal with than having 
> > >> >> >> the
> > >> >> >> protocol enforced one way or the other, but that should work.
> > >> >> >>
> > >> >> >> >
> > >> >> >> >> >
> > >> >> >> >> > > Yeah I know in current code the parser calls dispatcher 
> > >> >> >> >> > > directly
> > >> >> >> >> > > (please see handle_qmp_command()).  However it's not true 
> > >> >> >> >> > > again after
> > >> >> >> >> > > this series (parser will has its own 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> On Mon, Sep 18, 2017 at 11:40:40AM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (pet...@redhat.com) wrote:
> > > On Fri, Sep 15, 2017 at 04:17:07PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > > > > On Fri, Sep 15, 2017 at 01:29:13PM +0100, Daniel P. Berrange wrote:
> > > > > > On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert 
> > > > > > wrote:
> > > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan 
> > > > > > > > Gilbert wrote:
> > > > > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi 
> > > > > > > > > > wrote:
> > > > > > > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan 
> > > > > > > > > > > > Hajnoczi wrote:
> > > > > > > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André 
> > > > > > > > > > > > > Lureau wrote:
> > > > > > > > > > > > > > There should be a limit in the number of requests 
> > > > > > > > > > > > > > the thread can
> > > > > > > > > > > > > > queue. Before the patch, the limit was enforced by 
> > > > > > > > > > > > > > system socket
> > > > > > > > > > > > > > buffering I think. Now, should oob commands still 
> > > > > > > > > > > > > > be processed even if
> > > > > > > > > > > > > > the queue is full? If so, the thread can't be 
> > > > > > > > > > > > > > suspended.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I agree.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Memory usage must be bounded.  The number of requests 
> > > > > > > > > > > > > is less important
> > > > > > > > > > > > > than the amount of memory consumed by them.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Existing QMP clients that send multiple QMP commands 
> > > > > > > > > > > > > without waiting for
> > > > > > > > > > > > > replies need to rethink their strategy because OOB 
> > > > > > > > > > > > > commands cannot be
> > > > > > > > > > > > > processed if queued non-OOB commands consume too much 
> > > > > > > > > > > > > memory.
> > > > > > > > > > > > 
> > > > > > > > > > > > Thanks for pointing out this.  Yes the memory usage 
> > > > > > > > > > > > problem is valid,
> > > > > > > > > > > > as Markus pointed out as well in previous discussions 
> > > > > > > > > > > > (in "Flow
> > > > > > > > > > > > Control" section of that long reply).  Hopefully this 
> > > > > > > > > > > > series basically
> > > > > > > > > > > > can work from design prospective, then I'll add this 
> > > > > > > > > > > > flow control in
> > > > > > > > > > > > next version.
> > > > > > > > > > > > 
> > > > > > > > > > > > Regarding to what we should do if the limit is reached: 
> > > > > > > > > > > > Markus
> > > > > > > > > > > > provided a few options, but the one I prefer most is 
> > > > > > > > > > > > that we don't
> > > > > > > > > > > > respond, but send an event showing that a command is 
> > > > > > > > > > > > dropped.
> > > > > > > > > > > > However, I would like it not queued, but a direct reply 
> > > > > > > > > > > > (after all,
> > > > > > > > > > > > it's an event, and we should not need to care much on 
> > > > > > > > > > > > ordering of it).
> > > > > > > > > > > > Then we can get rid of the babysitting of those "to be 
> > > > > > > > > > > > failed"
> > > > > > > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > > > > > > 
> > > > > > > > > > > > I think I also missed at least a unit test for this new 
> > > > > > > > > > > > interface.
> > > > > > > > > > > > Again, I'll add it after the whole idea is proved 
> > > > > > > > > > > > solid.  Thanks,
> > > > > > > > > > > 
> > > > > > > > > > > Another solution: the server reports available receive 
> > > > > > > > > > > buffer space to
> > > > > > > > > > > the client.  The server only guarantees immediate OOB 
> > > > > > > > > > > processing when
> > > > > > > > > > > the client stays within the receive buffer size.
> > > > > > > > > > > 
> > > > > > > > > > > Clients wishing to take advantage of OOB must query the 
> > > > > > > > > > > receive buffer
> > > > > > > > > > > size and make sure to leave enough room.
> > > > > > > > > > 
> > > > > > > > > > I don't think having to query it ahead of time is 
> > > > > > > > > > particularly nice,
> > > > > > > > > > and of course it is inherantly racy.
> > > > > > > > > > 
> > > > > > > > > > I would just have QEMU emit an event when it pausing 
> > > > > > > > > > processing of the
> > > > > > > > > > incoming commands due to a full queue.  If the event 
> > > > > > > > > > includes the ID
> > > > > > > > > > of the last queued command, the client will know which (if 
> > > > > > > > > > any) of
> > > > > > > > > > its outstanding commands are delayed. Another even 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Mon, Sep 18, 2017 at 06:09:29PM +0200, Marc-André Lureau wrote:
> On Mon, Sep 18, 2017 at 1:26 PM, Dr. David Alan Gilbert
>  wrote:
> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> Hi
> >>
> >> On Mon, Sep 18, 2017 at 12:55 PM, Dr. David Alan Gilbert
> >>  wrote:
> >> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> >> Hi
> >> >>
> >> >> On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  wrote:
> >> >> > On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
> >> >> >> Hi
> >> >> >>
> >> >> >> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
> >> >> >> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert 
> >> >> >> > wrote:
> >> >> >> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> >> >> >> > Hi
> >> >> >> >> >
> >> >> >> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  
> >> >> >> >> > wrote:
> >> >> >> >> > > This series was born from this one:
> >> >> >> >> > >
> >> >> >> >> > >   
> >> >> >> >> > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> >> >> >> >> > >
> >> >> >> >> > > The design comes from Markus, and also the whole-bunch-of 
> >> >> >> >> > > discussions
> >> >> >> >> > > in previous thread.  My heartful thanks to Markus, Daniel, 
> >> >> >> >> > > Dave,
> >> >> >> >> > > Stefan, etc. on discussing the topic (...again!), providing 
> >> >> >> >> > > shiny
> >> >> >> >> > > ideas and suggestions.  Finally we got such a solution that 
> >> >> >> >> > > seems to
> >> >> >> >> > > satisfy everyone.
> >> >> >> >> > >
> >> >> >> >> > > I re-started the versioning since this series is totally 
> >> >> >> >> > > different
> >> >> >> >> > > from previous one.  Now it's version 1.
> >> >> >> >> > >
> >> >> >> >> > > In case new reviewers come along the way without reading 
> >> >> >> >> > > previous
> >> >> >> >> > > discussions, I will try to do a summary on what this is all 
> >> >> >> >> > > about.
> >> >> >> >> > >
> >> >> >> >> > > What is OOB execution?
> >> >> >> >> > > ==
> >> >> >> >> > >
> >> >> >> >> > > It's the shortcut of Out-Of-Band execution, its name is given 
> >> >> >> >> > > by
> >> >> >> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, 
> >> >> >> >> > > originally
> >> >> >> >> > > QMP is going throw these steps:
> >> >> >> >> > >
> >> >> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> >> >> >> >> > >   /|\(2)(3) |
> >> >> >> >> > >(1) |   \|/ (4)
> >> >> >> >> > >+-  main thread  +
> >> >> >> >> > >
> >> >> >> >> > > The requests are executed by the so-called QMP-dispatcher 
> >> >> >> >> > > after the
> >> >> >> >> > > JSON is parsed.  If OOB is on, we run the command directly in 
> >> >> >> >> > > the
> >> >> >> >> > > parser and quickly returns.
> >> >> >> >> >
> >> >> >> >> > All commands should have the "id" field mandatory in this case, 
> >> >> >> >> > else
> >> >> >> >> > the client will not distinguish the replies coming from the 
> >> >> >> >> > last/oob
> >> >> >> >> > and the previous commands.
> >> >> >> >> >
> >> >> >> >> > This should probably be enforced upfront by client capability 
> >> >> >> >> > checks,
> >> >> >> >> > more below.
> >> >> >> >
> >> >> >> > Hmm yes since the oob commands are actually running in async way,
> >> >> >> > request ID should be needed here.  However I'm not sure whether
> >> >> >> > enabling the whole "request ID" thing is too big for this "try to 
> >> >> >> > be
> >> >> >> > small" oob change... And IMHO it suites better to be part of the 
> >> >> >> > whole
> >> >> >> > async work (no matter which implementation we'll use).
> >> >> >> >
> >> >> >> > How about this: we make "id" mandatory for "run-oob" requests only.
> >> >> >> > For oob commands, they will always have ID then no ordering issue, 
> >> >> >> > and
> >> >> >> > we can do it async; for the rest of non-oob commands, we still 
> >> >> >> > allow
> >> >> >> > them to go without ID, and since they are not oob, they'll always 
> >> >> >> > be
> >> >> >> > done in order as well.  Would this work?
> >> >> >>
> >> >> >> This mixed-mode is imho more complicated to deal with than having the
> >> >> >> protocol enforced one way or the other, but that should work.
> >> >> >>
> >> >> >> >
> >> >> >> >> >
> >> >> >> >> > > Yeah I know in current code the parser calls dispatcher 
> >> >> >> >> > > directly
> >> >> >> >> > > (please see handle_qmp_command()).  However it's not true 
> >> >> >> >> > > again after
> >> >> >> >> > > this series (parser will has its own IO thread, and 
> >> >> >> >> > > dispatcher will
> >> >> >> >> > > still be run in main thread).  So this OOB does brings 
> >> >> >> >> > > something
> >> >> >> >> > > different.
> >> >> >> >> > >
> >> >> >> >> > > There are more details on why OOB and the 
> >> >> >> >> > > 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Mon, Sep 18, 2017 at 11:40:40AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Fri, Sep 15, 2017 at 04:17:07PM +0100, Dr. David Alan Gilbert wrote:
> > > * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > > > On Fri, Sep 15, 2017 at 01:29:13PM +0100, Daniel P. Berrange wrote:
> > > > > On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert 
> > > > > wrote:
> > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert 
> > > > > > > wrote:
> > > > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi 
> > > > > > > > > wrote:
> > > > > > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André 
> > > > > > > > > > > > Lureau wrote:
> > > > > > > > > > > > > There should be a limit in the number of requests the 
> > > > > > > > > > > > > thread can
> > > > > > > > > > > > > queue. Before the patch, the limit was enforced by 
> > > > > > > > > > > > > system socket
> > > > > > > > > > > > > buffering I think. Now, should oob commands still be 
> > > > > > > > > > > > > processed even if
> > > > > > > > > > > > > the queue is full? If so, the thread can't be 
> > > > > > > > > > > > > suspended.
> > > > > > > > > > > > 
> > > > > > > > > > > > I agree.
> > > > > > > > > > > > 
> > > > > > > > > > > > Memory usage must be bounded.  The number of requests 
> > > > > > > > > > > > is less important
> > > > > > > > > > > > than the amount of memory consumed by them.
> > > > > > > > > > > > 
> > > > > > > > > > > > Existing QMP clients that send multiple QMP commands 
> > > > > > > > > > > > without waiting for
> > > > > > > > > > > > replies need to rethink their strategy because OOB 
> > > > > > > > > > > > commands cannot be
> > > > > > > > > > > > processed if queued non-OOB commands consume too much 
> > > > > > > > > > > > memory.
> > > > > > > > > > > 
> > > > > > > > > > > Thanks for pointing out this.  Yes the memory usage 
> > > > > > > > > > > problem is valid,
> > > > > > > > > > > as Markus pointed out as well in previous discussions (in 
> > > > > > > > > > > "Flow
> > > > > > > > > > > Control" section of that long reply).  Hopefully this 
> > > > > > > > > > > series basically
> > > > > > > > > > > can work from design prospective, then I'll add this flow 
> > > > > > > > > > > control in
> > > > > > > > > > > next version.
> > > > > > > > > > > 
> > > > > > > > > > > Regarding to what we should do if the limit is reached: 
> > > > > > > > > > > Markus
> > > > > > > > > > > provided a few options, but the one I prefer most is that 
> > > > > > > > > > > we don't
> > > > > > > > > > > respond, but send an event showing that a command is 
> > > > > > > > > > > dropped.
> > > > > > > > > > > However, I would like it not queued, but a direct reply 
> > > > > > > > > > > (after all,
> > > > > > > > > > > it's an event, and we should not need to care much on 
> > > > > > > > > > > ordering of it).
> > > > > > > > > > > Then we can get rid of the babysitting of those "to be 
> > > > > > > > > > > failed"
> > > > > > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > > > > > 
> > > > > > > > > > > I think I also missed at least a unit test for this new 
> > > > > > > > > > > interface.
> > > > > > > > > > > Again, I'll add it after the whole idea is proved solid.  
> > > > > > > > > > > Thanks,
> > > > > > > > > > 
> > > > > > > > > > Another solution: the server reports available receive 
> > > > > > > > > > buffer space to
> > > > > > > > > > the client.  The server only guarantees immediate OOB 
> > > > > > > > > > processing when
> > > > > > > > > > the client stays within the receive buffer size.
> > > > > > > > > > 
> > > > > > > > > > Clients wishing to take advantage of OOB must query the 
> > > > > > > > > > receive buffer
> > > > > > > > > > size and make sure to leave enough room.
> > > > > > > > > 
> > > > > > > > > I don't think having to query it ahead of time is 
> > > > > > > > > particularly nice,
> > > > > > > > > and of course it is inherantly racy.
> > > > > > > > > 
> > > > > > > > > I would just have QEMU emit an event when it pausing 
> > > > > > > > > processing of the
> > > > > > > > > incoming commands due to a full queue.  If the event includes 
> > > > > > > > > the ID
> > > > > > > > > of the last queued command, the client will know which (if 
> > > > > > > > > any) of
> > > > > > > > > its outstanding commands are delayed. Another even can be 
> > > > > > > > > sent when
> > > > > > > > > it restarts reading.
> > > > > > > > 
> > > > > > > > Hmm and now we're implementing flow control!
> > > > > > > > 
> > > > > > > > a) What exactly is the current 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Marc-André Lureau]

On Mon, Sep 18, 2017 at 1:26 PM, Dr. David Alan Gilbert
 wrote:
> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> Hi
>>
>> On Mon, Sep 18, 2017 at 12:55 PM, Dr. David Alan Gilbert
>>  wrote:
>> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> >> Hi
>> >>
>> >> On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  wrote:
>> >> > On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
>> >> >> Hi
>> >> >>
>> >> >> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
>> >> >> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert 
>> >> >> > wrote:
>> >> >> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> >> >> >> > Hi
>> >> >> >> >
>> >> >> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  
>> >> >> >> > wrote:
>> >> >> >> > > This series was born from this one:
>> >> >> >> > >
>> >> >> >> > >   
>> >> >> >> > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
>> >> >> >> > >
>> >> >> >> > > The design comes from Markus, and also the whole-bunch-of 
>> >> >> >> > > discussions
>> >> >> >> > > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
>> >> >> >> > > Stefan, etc. on discussing the topic (...again!), providing 
>> >> >> >> > > shiny
>> >> >> >> > > ideas and suggestions.  Finally we got such a solution that 
>> >> >> >> > > seems to
>> >> >> >> > > satisfy everyone.
>> >> >> >> > >
>> >> >> >> > > I re-started the versioning since this series is totally 
>> >> >> >> > > different
>> >> >> >> > > from previous one.  Now it's version 1.
>> >> >> >> > >
>> >> >> >> > > In case new reviewers come along the way without reading 
>> >> >> >> > > previous
>> >> >> >> > > discussions, I will try to do a summary on what this is all 
>> >> >> >> > > about.
>> >> >> >> > >
>> >> >> >> > > What is OOB execution?
>> >> >> >> > > ==
>> >> >> >> > >
>> >> >> >> > > It's the shortcut of Out-Of-Band execution, its name is given by
>> >> >> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, 
>> >> >> >> > > originally
>> >> >> >> > > QMP is going throw these steps:
>> >> >> >> > >
>> >> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
>> >> >> >> > >   /|\(2)(3) |
>> >> >> >> > >(1) |   \|/ (4)
>> >> >> >> > >+-  main thread  +
>> >> >> >> > >
>> >> >> >> > > The requests are executed by the so-called QMP-dispatcher after 
>> >> >> >> > > the
>> >> >> >> > > JSON is parsed.  If OOB is on, we run the command directly in 
>> >> >> >> > > the
>> >> >> >> > > parser and quickly returns.
>> >> >> >> >
>> >> >> >> > All commands should have the "id" field mandatory in this case, 
>> >> >> >> > else
>> >> >> >> > the client will not distinguish the replies coming from the 
>> >> >> >> > last/oob
>> >> >> >> > and the previous commands.
>> >> >> >> >
>> >> >> >> > This should probably be enforced upfront by client capability 
>> >> >> >> > checks,
>> >> >> >> > more below.
>> >> >> >
>> >> >> > Hmm yes since the oob commands are actually running in async way,
>> >> >> > request ID should be needed here.  However I'm not sure whether
>> >> >> > enabling the whole "request ID" thing is too big for this "try to be
>> >> >> > small" oob change... And IMHO it suites better to be part of the 
>> >> >> > whole
>> >> >> > async work (no matter which implementation we'll use).
>> >> >> >
>> >> >> > How about this: we make "id" mandatory for "run-oob" requests only.
>> >> >> > For oob commands, they will always have ID then no ordering issue, 
>> >> >> > and
>> >> >> > we can do it async; for the rest of non-oob commands, we still allow
>> >> >> > them to go without ID, and since they are not oob, they'll always be
>> >> >> > done in order as well.  Would this work?
>> >> >>
>> >> >> This mixed-mode is imho more complicated to deal with than having the
>> >> >> protocol enforced one way or the other, but that should work.
>> >> >>
>> >> >> >
>> >> >> >> >
>> >> >> >> > > Yeah I know in current code the parser calls dispatcher directly
>> >> >> >> > > (please see handle_qmp_command()).  However it's not true again 
>> >> >> >> > > after
>> >> >> >> > > this series (parser will has its own IO thread, and dispatcher 
>> >> >> >> > > will
>> >> >> >> > > still be run in main thread).  So this OOB does brings something
>> >> >> >> > > different.
>> >> >> >> > >
>> >> >> >> > > There are more details on why OOB and the 
>> >> >> >> > > difference/relationship
>> >> >> >> > > between OOB, async QMP, block/general jobs, etc.. but IMHO 
>> >> >> >> > > that's
>> >> >> >> > > slightly out of topic (and believe me, it's not easy for me to
>> >> >> >> > > summarize that).  For more information, please refers to [1].
>> >> >> >> > >
>> >> >> >> > > Summary ends here.
>> >> >> >> > >
>> >> >> >> > > Some Implementation Details
>> >> >> >> 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Eric Blake]

On 09/18/2017 05:55 AM, Dr. David Alan Gilbert wrote:

>>> I think we have other quite simple ways to solve the "unexpected
>>> reply" and "per-client-id duplication" issues you have mentioned.
>>>
>>> Firstly, when client gets unexpected replies ("id" field not in its
>>> own request queue), the client should just ignore that reply, which
>>> seems natural to me.

That's probably reasonable, if we document it.

>>
>> The trouble is that it may legitimately use the same "id" value for
>> new requests. And I don't see a simple way to handle that without
>> races.
> 
> Under what circumstances can it reuse the same ID for new requests?

Libvirt uses distinct id's for every message on a single connection, but
there is always the possibility that it will use id 'libvirt-0' on one
connection, then restart libvirtd, then use id 'libvirt-0' on the second
connection (there's nothing that I saw in libvirt code that saves the
current 'mon->nextSerial' value in XML to survive libvirtd restarts).

> Can't we simply tell it not to?

Since use of OOB handling will require client opt-in, yes, we can make
part of the opt-in process be a contract that the client has to do a
better job of avoiding duplicate id's across reconnects, if we think
that is easier to maintain.

>>>
>>> Then, if client disconnected and reconnected, it should not have the
>>> problem to generate duplicated id for request, since it should know
>>> what requests it has sent already.  A simplest case I can think of is,
>>> the ID should contains the following tuple:
>>
>> If you assume the "same" client will recover its state, yes.
>>
>>>
>>>   (client name, client unique ID, request ID)
>>>
>>> Here "client name" can be something like "libvirt", which is the name
>>> of client application;
>>>
>>> "client unique ID" can be anything generated when client starts, it
>>> identifies a single client session, maybe a UUID.
>>>
>>> "request ID" can be a unsigned integer starts from zero, and increases
>>> each time the client sends one request.
>>
>> This is introducing  session handling, and can be done in server side
>> only without changes in the protocol I believe.

The 'id' field can be _any_ JSON object - libvirt currently sends a
string, but could just as easily send a dict, and then libvirt could
supply whatever it wanted in the dict, including uuids, to ensure that
future reconnects don't reuse the id of a previous connection.  But
right now, 'id' is opaque to qemu (and should stay that way) - if qemu
is going to do any work to ensure that it doesn't send a stale reply to
a new connection, then that has to be tracked externally from whatever
'id' is passed in; or we just document that clients wanting to use OOB
handling have to be careful of their choice of 'id' (and leave it up to
the client to avoid confusion, because qemu doesn't care about it).

I'm also fine with requiring that clients that opt-in to OOB handling be
documented as having to ignore unknown 'id' responses, since we already
document that clients must be able to ignore unknown 'event' messages.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
> 
> On Mon, Sep 18, 2017 at 12:55 PM, Dr. David Alan Gilbert
>  wrote:
> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> Hi
> >>
> >> On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  wrote:
> >> > On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
> >> >> Hi
> >> >>
> >> >> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
> >> >> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert 
> >> >> > wrote:
> >> >> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> >> >> > Hi
> >> >> >> >
> >> >> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  
> >> >> >> > wrote:
> >> >> >> > > This series was born from this one:
> >> >> >> > >
> >> >> >> > >   
> >> >> >> > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> >> >> >> > >
> >> >> >> > > The design comes from Markus, and also the whole-bunch-of 
> >> >> >> > > discussions
> >> >> >> > > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
> >> >> >> > > Stefan, etc. on discussing the topic (...again!), providing shiny
> >> >> >> > > ideas and suggestions.  Finally we got such a solution that 
> >> >> >> > > seems to
> >> >> >> > > satisfy everyone.
> >> >> >> > >
> >> >> >> > > I re-started the versioning since this series is totally 
> >> >> >> > > different
> >> >> >> > > from previous one.  Now it's version 1.
> >> >> >> > >
> >> >> >> > > In case new reviewers come along the way without reading previous
> >> >> >> > > discussions, I will try to do a summary on what this is all 
> >> >> >> > > about.
> >> >> >> > >
> >> >> >> > > What is OOB execution?
> >> >> >> > > ==
> >> >> >> > >
> >> >> >> > > It's the shortcut of Out-Of-Band execution, its name is given by
> >> >> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, 
> >> >> >> > > originally
> >> >> >> > > QMP is going throw these steps:
> >> >> >> > >
> >> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> >> >> >> > >   /|\(2)(3) |
> >> >> >> > >(1) |   \|/ (4)
> >> >> >> > >+-  main thread  +
> >> >> >> > >
> >> >> >> > > The requests are executed by the so-called QMP-dispatcher after 
> >> >> >> > > the
> >> >> >> > > JSON is parsed.  If OOB is on, we run the command directly in the
> >> >> >> > > parser and quickly returns.
> >> >> >> >
> >> >> >> > All commands should have the "id" field mandatory in this case, 
> >> >> >> > else
> >> >> >> > the client will not distinguish the replies coming from the 
> >> >> >> > last/oob
> >> >> >> > and the previous commands.
> >> >> >> >
> >> >> >> > This should probably be enforced upfront by client capability 
> >> >> >> > checks,
> >> >> >> > more below.
> >> >> >
> >> >> > Hmm yes since the oob commands are actually running in async way,
> >> >> > request ID should be needed here.  However I'm not sure whether
> >> >> > enabling the whole "request ID" thing is too big for this "try to be
> >> >> > small" oob change... And IMHO it suites better to be part of the whole
> >> >> > async work (no matter which implementation we'll use).
> >> >> >
> >> >> > How about this: we make "id" mandatory for "run-oob" requests only.
> >> >> > For oob commands, they will always have ID then no ordering issue, and
> >> >> > we can do it async; for the rest of non-oob commands, we still allow
> >> >> > them to go without ID, and since they are not oob, they'll always be
> >> >> > done in order as well.  Would this work?
> >> >>
> >> >> This mixed-mode is imho more complicated to deal with than having the
> >> >> protocol enforced one way or the other, but that should work.
> >> >>
> >> >> >
> >> >> >> >
> >> >> >> > > Yeah I know in current code the parser calls dispatcher directly
> >> >> >> > > (please see handle_qmp_command()).  However it's not true again 
> >> >> >> > > after
> >> >> >> > > this series (parser will has its own IO thread, and dispatcher 
> >> >> >> > > will
> >> >> >> > > still be run in main thread).  So this OOB does brings something
> >> >> >> > > different.
> >> >> >> > >
> >> >> >> > > There are more details on why OOB and the difference/relationship
> >> >> >> > > between OOB, async QMP, block/general jobs, etc.. but IMHO that's
> >> >> >> > > slightly out of topic (and believe me, it's not easy for me to
> >> >> >> > > summarize that).  For more information, please refers to [1].
> >> >> >> > >
> >> >> >> > > Summary ends here.
> >> >> >> > >
> >> >> >> > > Some Implementation Details
> >> >> >> > > ===
> >> >> >> > >
> >> >> >> > > Again, I mentioned that the old QMP workflow is this:
> >> >> >> > >
> >> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> >> >> >> > >   /|\(2)(3) |
> >> >> >> > >(1) |  

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Marc-André Lureau]

Hi

On Mon, Sep 18, 2017 at 12:55 PM, Dr. David Alan Gilbert
 wrote:
> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> Hi
>>
>> On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  wrote:
>> > On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
>> >> Hi
>> >>
>> >> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
>> >> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert wrote:
>> >> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> >> >> > Hi
>> >> >> >
>> >> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  wrote:
>> >> >> > > This series was born from this one:
>> >> >> > >
>> >> >> > >   
>> >> >> > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
>> >> >> > >
>> >> >> > > The design comes from Markus, and also the whole-bunch-of 
>> >> >> > > discussions
>> >> >> > > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
>> >> >> > > Stefan, etc. on discussing the topic (...again!), providing shiny
>> >> >> > > ideas and suggestions.  Finally we got such a solution that seems 
>> >> >> > > to
>> >> >> > > satisfy everyone.
>> >> >> > >
>> >> >> > > I re-started the versioning since this series is totally different
>> >> >> > > from previous one.  Now it's version 1.
>> >> >> > >
>> >> >> > > In case new reviewers come along the way without reading previous
>> >> >> > > discussions, I will try to do a summary on what this is all about.
>> >> >> > >
>> >> >> > > What is OOB execution?
>> >> >> > > ==
>> >> >> > >
>> >> >> > > It's the shortcut of Out-Of-Band execution, its name is given by
>> >> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, 
>> >> >> > > originally
>> >> >> > > QMP is going throw these steps:
>> >> >> > >
>> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
>> >> >> > >   /|\(2)(3) |
>> >> >> > >(1) |   \|/ (4)
>> >> >> > >+-  main thread  +
>> >> >> > >
>> >> >> > > The requests are executed by the so-called QMP-dispatcher after the
>> >> >> > > JSON is parsed.  If OOB is on, we run the command directly in the
>> >> >> > > parser and quickly returns.
>> >> >> >
>> >> >> > All commands should have the "id" field mandatory in this case, else
>> >> >> > the client will not distinguish the replies coming from the last/oob
>> >> >> > and the previous commands.
>> >> >> >
>> >> >> > This should probably be enforced upfront by client capability checks,
>> >> >> > more below.
>> >> >
>> >> > Hmm yes since the oob commands are actually running in async way,
>> >> > request ID should be needed here.  However I'm not sure whether
>> >> > enabling the whole "request ID" thing is too big for this "try to be
>> >> > small" oob change... And IMHO it suites better to be part of the whole
>> >> > async work (no matter which implementation we'll use).
>> >> >
>> >> > How about this: we make "id" mandatory for "run-oob" requests only.
>> >> > For oob commands, they will always have ID then no ordering issue, and
>> >> > we can do it async; for the rest of non-oob commands, we still allow
>> >> > them to go without ID, and since they are not oob, they'll always be
>> >> > done in order as well.  Would this work?
>> >>
>> >> This mixed-mode is imho more complicated to deal with than having the
>> >> protocol enforced one way or the other, but that should work.
>> >>
>> >> >
>> >> >> >
>> >> >> > > Yeah I know in current code the parser calls dispatcher directly
>> >> >> > > (please see handle_qmp_command()).  However it's not true again 
>> >> >> > > after
>> >> >> > > this series (parser will has its own IO thread, and dispatcher will
>> >> >> > > still be run in main thread).  So this OOB does brings something
>> >> >> > > different.
>> >> >> > >
>> >> >> > > There are more details on why OOB and the difference/relationship
>> >> >> > > between OOB, async QMP, block/general jobs, etc.. but IMHO that's
>> >> >> > > slightly out of topic (and believe me, it's not easy for me to
>> >> >> > > summarize that).  For more information, please refers to [1].
>> >> >> > >
>> >> >> > > Summary ends here.
>> >> >> > >
>> >> >> > > Some Implementation Details
>> >> >> > > ===
>> >> >> > >
>> >> >> > > Again, I mentioned that the old QMP workflow is this:
>> >> >> > >
>> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
>> >> >> > >   /|\(2)(3) |
>> >> >> > >(1) |   \|/ (4)
>> >> >> > >+-  main thread  +
>> >> >> > >
>> >> >> > > What this series does is, firstly:
>> >> >> > >
>> >> >> > >   JSON Parser QMP Dispatcher --> Respond
>> >> >> > >   /|\ |   /|\   (4) |
>> >> >> > >|  | (2)| (3)|  (5)
>> >> >> > >(1) |  +->  | 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
> 
> On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  wrote:
> > On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
> >> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert wrote:
> >> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> >> > Hi
> >> >> >
> >> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  wrote:
> >> >> > > This series was born from this one:
> >> >> > >
> >> >> > >   
> >> >> > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> >> >> > >
> >> >> > > The design comes from Markus, and also the whole-bunch-of 
> >> >> > > discussions
> >> >> > > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
> >> >> > > Stefan, etc. on discussing the topic (...again!), providing shiny
> >> >> > > ideas and suggestions.  Finally we got such a solution that seems to
> >> >> > > satisfy everyone.
> >> >> > >
> >> >> > > I re-started the versioning since this series is totally different
> >> >> > > from previous one.  Now it's version 1.
> >> >> > >
> >> >> > > In case new reviewers come along the way without reading previous
> >> >> > > discussions, I will try to do a summary on what this is all about.
> >> >> > >
> >> >> > > What is OOB execution?
> >> >> > > ==
> >> >> > >
> >> >> > > It's the shortcut of Out-Of-Band execution, its name is given by
> >> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, 
> >> >> > > originally
> >> >> > > QMP is going throw these steps:
> >> >> > >
> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> >> >> > >   /|\(2)(3) |
> >> >> > >(1) |   \|/ (4)
> >> >> > >+-  main thread  +
> >> >> > >
> >> >> > > The requests are executed by the so-called QMP-dispatcher after the
> >> >> > > JSON is parsed.  If OOB is on, we run the command directly in the
> >> >> > > parser and quickly returns.
> >> >> >
> >> >> > All commands should have the "id" field mandatory in this case, else
> >> >> > the client will not distinguish the replies coming from the last/oob
> >> >> > and the previous commands.
> >> >> >
> >> >> > This should probably be enforced upfront by client capability checks,
> >> >> > more below.
> >> >
> >> > Hmm yes since the oob commands are actually running in async way,
> >> > request ID should be needed here.  However I'm not sure whether
> >> > enabling the whole "request ID" thing is too big for this "try to be
> >> > small" oob change... And IMHO it suites better to be part of the whole
> >> > async work (no matter which implementation we'll use).
> >> >
> >> > How about this: we make "id" mandatory for "run-oob" requests only.
> >> > For oob commands, they will always have ID then no ordering issue, and
> >> > we can do it async; for the rest of non-oob commands, we still allow
> >> > them to go without ID, and since they are not oob, they'll always be
> >> > done in order as well.  Would this work?
> >>
> >> This mixed-mode is imho more complicated to deal with than having the
> >> protocol enforced one way or the other, but that should work.
> >>
> >> >
> >> >> >
> >> >> > > Yeah I know in current code the parser calls dispatcher directly
> >> >> > > (please see handle_qmp_command()).  However it's not true again 
> >> >> > > after
> >> >> > > this series (parser will has its own IO thread, and dispatcher will
> >> >> > > still be run in main thread).  So this OOB does brings something
> >> >> > > different.
> >> >> > >
> >> >> > > There are more details on why OOB and the difference/relationship
> >> >> > > between OOB, async QMP, block/general jobs, etc.. but IMHO that's
> >> >> > > slightly out of topic (and believe me, it's not easy for me to
> >> >> > > summarize that).  For more information, please refers to [1].
> >> >> > >
> >> >> > > Summary ends here.
> >> >> > >
> >> >> > > Some Implementation Details
> >> >> > > ===
> >> >> > >
> >> >> > > Again, I mentioned that the old QMP workflow is this:
> >> >> > >
> >> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> >> >> > >   /|\(2)(3) |
> >> >> > >(1) |   \|/ (4)
> >> >> > >+-  main thread  +
> >> >> > >
> >> >> > > What this series does is, firstly:
> >> >> > >
> >> >> > >   JSON Parser QMP Dispatcher --> Respond
> >> >> > >   /|\ |   /|\   (4) |
> >> >> > >|  | (2)| (3)|  (5)
> >> >> > >(1) |  +->  |   \|/
> >> >> > >+-  main thread  <---+
> >> >> > >
> >> >> > > And further:
> >> >> > >
> >> >> > >queue/kick
> >> >> > >  JSON Parser ==> QMP 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> On Fri, Sep 15, 2017 at 04:17:07PM +0100, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > > On Fri, Sep 15, 2017 at 01:29:13PM +0100, Daniel P. Berrange wrote:
> > > > On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert 
> > > > > > wrote:
> > > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > > > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi 
> > > > > > > > > > wrote:
> > > > > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André 
> > > > > > > > > > > Lureau wrote:
> > > > > > > > > > > > There should be a limit in the number of requests the 
> > > > > > > > > > > > thread can
> > > > > > > > > > > > queue. Before the patch, the limit was enforced by 
> > > > > > > > > > > > system socket
> > > > > > > > > > > > buffering I think. Now, should oob commands still be 
> > > > > > > > > > > > processed even if
> > > > > > > > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > > > > > > > 
> > > > > > > > > > > I agree.
> > > > > > > > > > > 
> > > > > > > > > > > Memory usage must be bounded.  The number of requests is 
> > > > > > > > > > > less important
> > > > > > > > > > > than the amount of memory consumed by them.
> > > > > > > > > > > 
> > > > > > > > > > > Existing QMP clients that send multiple QMP commands 
> > > > > > > > > > > without waiting for
> > > > > > > > > > > replies need to rethink their strategy because OOB 
> > > > > > > > > > > commands cannot be
> > > > > > > > > > > processed if queued non-OOB commands consume too much 
> > > > > > > > > > > memory.
> > > > > > > > > > 
> > > > > > > > > > Thanks for pointing out this.  Yes the memory usage problem 
> > > > > > > > > > is valid,
> > > > > > > > > > as Markus pointed out as well in previous discussions (in 
> > > > > > > > > > "Flow
> > > > > > > > > > Control" section of that long reply).  Hopefully this 
> > > > > > > > > > series basically
> > > > > > > > > > can work from design prospective, then I'll add this flow 
> > > > > > > > > > control in
> > > > > > > > > > next version.
> > > > > > > > > > 
> > > > > > > > > > Regarding to what we should do if the limit is reached: 
> > > > > > > > > > Markus
> > > > > > > > > > provided a few options, but the one I prefer most is that 
> > > > > > > > > > we don't
> > > > > > > > > > respond, but send an event showing that a command is 
> > > > > > > > > > dropped.
> > > > > > > > > > However, I would like it not queued, but a direct reply 
> > > > > > > > > > (after all,
> > > > > > > > > > it's an event, and we should not need to care much on 
> > > > > > > > > > ordering of it).
> > > > > > > > > > Then we can get rid of the babysitting of those "to be 
> > > > > > > > > > failed"
> > > > > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > > > > 
> > > > > > > > > > I think I also missed at least a unit test for this new 
> > > > > > > > > > interface.
> > > > > > > > > > Again, I'll add it after the whole idea is proved solid.  
> > > > > > > > > > Thanks,
> > > > > > > > > 
> > > > > > > > > Another solution: the server reports available receive buffer 
> > > > > > > > > space to
> > > > > > > > > the client.  The server only guarantees immediate OOB 
> > > > > > > > > processing when
> > > > > > > > > the client stays within the receive buffer size.
> > > > > > > > > 
> > > > > > > > > Clients wishing to take advantage of OOB must query the 
> > > > > > > > > receive buffer
> > > > > > > > > size and make sure to leave enough room.
> > > > > > > > 
> > > > > > > > I don't think having to query it ahead of time is particularly 
> > > > > > > > nice,
> > > > > > > > and of course it is inherantly racy.
> > > > > > > > 
> > > > > > > > I would just have QEMU emit an event when it pausing processing 
> > > > > > > > of the
> > > > > > > > incoming commands due to a full queue.  If the event includes 
> > > > > > > > the ID
> > > > > > > > of the last queued command, the client will know which (if any) 
> > > > > > > > of
> > > > > > > > its outstanding commands are delayed. Another even can be sent 
> > > > > > > > when
> > > > > > > > it restarts reading.
> > > > > > > 
> > > > > > > Hmm and now we're implementing flow control!
> > > > > > > 
> > > > > > > a) What exactly is the current semantics/buffer sizes?
> > > > > > > b) When do clients send multiple QMP commands on one channel 
> > > > > > > without
> > > > > > > waiting for the response to the previous command?
> > > > > > > c) Would one queue entry for each class of commands/channel work
> > > > > > >   (Where a class of commands is 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Marc-André Lureau]

Hi

On Mon, Sep 18, 2017 at 10:37 AM, Peter Xu  wrote:
> On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
>> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert wrote:
>> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> >> > Hi
>> >> >
>> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  wrote:
>> >> > > This series was born from this one:
>> >> > >
>> >> > >   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
>> >> > >
>> >> > > The design comes from Markus, and also the whole-bunch-of discussions
>> >> > > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
>> >> > > Stefan, etc. on discussing the topic (...again!), providing shiny
>> >> > > ideas and suggestions.  Finally we got such a solution that seems to
>> >> > > satisfy everyone.
>> >> > >
>> >> > > I re-started the versioning since this series is totally different
>> >> > > from previous one.  Now it's version 1.
>> >> > >
>> >> > > In case new reviewers come along the way without reading previous
>> >> > > discussions, I will try to do a summary on what this is all about.
>> >> > >
>> >> > > What is OOB execution?
>> >> > > ==
>> >> > >
>> >> > > It's the shortcut of Out-Of-Band execution, its name is given by
>> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, originally
>> >> > > QMP is going throw these steps:
>> >> > >
>> >> > >   JSON Parser --> QMP Dispatcher --> Respond
>> >> > >   /|\(2)(3) |
>> >> > >(1) |   \|/ (4)
>> >> > >+-  main thread  +
>> >> > >
>> >> > > The requests are executed by the so-called QMP-dispatcher after the
>> >> > > JSON is parsed.  If OOB is on, we run the command directly in the
>> >> > > parser and quickly returns.
>> >> >
>> >> > All commands should have the "id" field mandatory in this case, else
>> >> > the client will not distinguish the replies coming from the last/oob
>> >> > and the previous commands.
>> >> >
>> >> > This should probably be enforced upfront by client capability checks,
>> >> > more below.
>> >
>> > Hmm yes since the oob commands are actually running in async way,
>> > request ID should be needed here.  However I'm not sure whether
>> > enabling the whole "request ID" thing is too big for this "try to be
>> > small" oob change... And IMHO it suites better to be part of the whole
>> > async work (no matter which implementation we'll use).
>> >
>> > How about this: we make "id" mandatory for "run-oob" requests only.
>> > For oob commands, they will always have ID then no ordering issue, and
>> > we can do it async; for the rest of non-oob commands, we still allow
>> > them to go without ID, and since they are not oob, they'll always be
>> > done in order as well.  Would this work?
>>
>> This mixed-mode is imho more complicated to deal with than having the
>> protocol enforced one way or the other, but that should work.
>>
>> >
>> >> >
>> >> > > Yeah I know in current code the parser calls dispatcher directly
>> >> > > (please see handle_qmp_command()).  However it's not true again after
>> >> > > this series (parser will has its own IO thread, and dispatcher will
>> >> > > still be run in main thread).  So this OOB does brings something
>> >> > > different.
>> >> > >
>> >> > > There are more details on why OOB and the difference/relationship
>> >> > > between OOB, async QMP, block/general jobs, etc.. but IMHO that's
>> >> > > slightly out of topic (and believe me, it's not easy for me to
>> >> > > summarize that).  For more information, please refers to [1].
>> >> > >
>> >> > > Summary ends here.
>> >> > >
>> >> > > Some Implementation Details
>> >> > > ===
>> >> > >
>> >> > > Again, I mentioned that the old QMP workflow is this:
>> >> > >
>> >> > >   JSON Parser --> QMP Dispatcher --> Respond
>> >> > >   /|\(2)(3) |
>> >> > >(1) |   \|/ (4)
>> >> > >+-  main thread  +
>> >> > >
>> >> > > What this series does is, firstly:
>> >> > >
>> >> > >   JSON Parser QMP Dispatcher --> Respond
>> >> > >   /|\ |   /|\   (4) |
>> >> > >|  | (2)| (3)|  (5)
>> >> > >(1) |  +->  |   \|/
>> >> > >+-  main thread  <---+
>> >> > >
>> >> > > And further:
>> >> > >
>> >> > >queue/kick
>> >> > >  JSON Parser ==> QMP Dispatcher --> Respond
>> >> > >  /|\ | (3)   /|\(4)|
>> >> > >   (1) |  | (2)||  (5)
>> >> > >   | \|/   |   \|/
>> >> > > IO thread main thread  <---+
>> >> >
>> >> > Is the queue per monitor or per 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Fri, Sep 15, 2017 at 04:17:07PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > On Fri, Sep 15, 2017 at 01:29:13PM +0100, Daniel P. Berrange wrote:
> > > On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert 
> > > > > wrote:
> > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi 
> > > > > > > > > wrote:
> > > > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau 
> > > > > > > > > > wrote:
> > > > > > > > > > > There should be a limit in the number of requests the 
> > > > > > > > > > > thread can
> > > > > > > > > > > queue. Before the patch, the limit was enforced by system 
> > > > > > > > > > > socket
> > > > > > > > > > > buffering I think. Now, should oob commands still be 
> > > > > > > > > > > processed even if
> > > > > > > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > > > > > > 
> > > > > > > > > > I agree.
> > > > > > > > > > 
> > > > > > > > > > Memory usage must be bounded.  The number of requests is 
> > > > > > > > > > less important
> > > > > > > > > > than the amount of memory consumed by them.
> > > > > > > > > > 
> > > > > > > > > > Existing QMP clients that send multiple QMP commands 
> > > > > > > > > > without waiting for
> > > > > > > > > > replies need to rethink their strategy because OOB commands 
> > > > > > > > > > cannot be
> > > > > > > > > > processed if queued non-OOB commands consume too much 
> > > > > > > > > > memory.
> > > > > > > > > 
> > > > > > > > > Thanks for pointing out this.  Yes the memory usage problem 
> > > > > > > > > is valid,
> > > > > > > > > as Markus pointed out as well in previous discussions (in 
> > > > > > > > > "Flow
> > > > > > > > > Control" section of that long reply).  Hopefully this series 
> > > > > > > > > basically
> > > > > > > > > can work from design prospective, then I'll add this flow 
> > > > > > > > > control in
> > > > > > > > > next version.
> > > > > > > > > 
> > > > > > > > > Regarding to what we should do if the limit is reached: Markus
> > > > > > > > > provided a few options, but the one I prefer most is that we 
> > > > > > > > > don't
> > > > > > > > > respond, but send an event showing that a command is dropped.
> > > > > > > > > However, I would like it not queued, but a direct reply 
> > > > > > > > > (after all,
> > > > > > > > > it's an event, and we should not need to care much on 
> > > > > > > > > ordering of it).
> > > > > > > > > Then we can get rid of the babysitting of those "to be failed"
> > > > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > > > 
> > > > > > > > > I think I also missed at least a unit test for this new 
> > > > > > > > > interface.
> > > > > > > > > Again, I'll add it after the whole idea is proved solid.  
> > > > > > > > > Thanks,
> > > > > > > > 
> > > > > > > > Another solution: the server reports available receive buffer 
> > > > > > > > space to
> > > > > > > > the client.  The server only guarantees immediate OOB 
> > > > > > > > processing when
> > > > > > > > the client stays within the receive buffer size.
> > > > > > > > 
> > > > > > > > Clients wishing to take advantage of OOB must query the receive 
> > > > > > > > buffer
> > > > > > > > size and make sure to leave enough room.
> > > > > > > 
> > > > > > > I don't think having to query it ahead of time is particularly 
> > > > > > > nice,
> > > > > > > and of course it is inherantly racy.
> > > > > > > 
> > > > > > > I would just have QEMU emit an event when it pausing processing 
> > > > > > > of the
> > > > > > > incoming commands due to a full queue.  If the event includes the 
> > > > > > > ID
> > > > > > > of the last queued command, the client will know which (if any) of
> > > > > > > its outstanding commands are delayed. Another even can be sent 
> > > > > > > when
> > > > > > > it restarts reading.
> > > > > > 
> > > > > > Hmm and now we're implementing flow control!
> > > > > > 
> > > > > > a) What exactly is the current semantics/buffer sizes?
> > > > > > b) When do clients send multiple QMP commands on one channel without
> > > > > > waiting for the response to the previous command?
> > > > > > c) Would one queue entry for each class of commands/channel work
> > > > > >   (Where a class of commands is currently 'normal' and 'oob')
> > > > > 
> > > > > I do wonder if we need to worry about request limiting at all from the
> > > > > client side.  For non-OOB commands clients will wait for a reply 
> > > > > before
> > > > > sending a 2nd non-OOB command, so you'll never get a deep queue for.
> > > > > 
> > > > > OOB 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Fri, Sep 15, 2017 at 01:14:47PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
> > On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert wrote:
> >> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> >> > Hi
> >> >
> >> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  wrote:
> >> > > This series was born from this one:
> >> > >
> >> > >   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> >> > >
> >> > > The design comes from Markus, and also the whole-bunch-of discussions
> >> > > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
> >> > > Stefan, etc. on discussing the topic (...again!), providing shiny
> >> > > ideas and suggestions.  Finally we got such a solution that seems to
> >> > > satisfy everyone.
> >> > >
> >> > > I re-started the versioning since this series is totally different
> >> > > from previous one.  Now it's version 1.
> >> > >
> >> > > In case new reviewers come along the way without reading previous
> >> > > discussions, I will try to do a summary on what this is all about.
> >> > >
> >> > > What is OOB execution?
> >> > > ==
> >> > >
> >> > > It's the shortcut of Out-Of-Band execution, its name is given by
> >> > > Markus.  It's a way to quickly execute a QMP request.  Say, originally
> >> > > QMP is going throw these steps:
> >> > >
> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> >> > >   /|\(2)(3) |
> >> > >(1) |   \|/ (4)
> >> > >+-  main thread  +
> >> > >
> >> > > The requests are executed by the so-called QMP-dispatcher after the
> >> > > JSON is parsed.  If OOB is on, we run the command directly in the
> >> > > parser and quickly returns.
> >> >
> >> > All commands should have the "id" field mandatory in this case, else
> >> > the client will not distinguish the replies coming from the last/oob
> >> > and the previous commands.
> >> >
> >> > This should probably be enforced upfront by client capability checks,
> >> > more below.
> >
> > Hmm yes since the oob commands are actually running in async way,
> > request ID should be needed here.  However I'm not sure whether
> > enabling the whole "request ID" thing is too big for this "try to be
> > small" oob change... And IMHO it suites better to be part of the whole
> > async work (no matter which implementation we'll use).
> >
> > How about this: we make "id" mandatory for "run-oob" requests only.
> > For oob commands, they will always have ID then no ordering issue, and
> > we can do it async; for the rest of non-oob commands, we still allow
> > them to go without ID, and since they are not oob, they'll always be
> > done in order as well.  Would this work?
> 
> This mixed-mode is imho more complicated to deal with than having the
> protocol enforced one way or the other, but that should work.
> 
> >
> >> >
> >> > > Yeah I know in current code the parser calls dispatcher directly
> >> > > (please see handle_qmp_command()).  However it's not true again after
> >> > > this series (parser will has its own IO thread, and dispatcher will
> >> > > still be run in main thread).  So this OOB does brings something
> >> > > different.
> >> > >
> >> > > There are more details on why OOB and the difference/relationship
> >> > > between OOB, async QMP, block/general jobs, etc.. but IMHO that's
> >> > > slightly out of topic (and believe me, it's not easy for me to
> >> > > summarize that).  For more information, please refers to [1].
> >> > >
> >> > > Summary ends here.
> >> > >
> >> > > Some Implementation Details
> >> > > ===
> >> > >
> >> > > Again, I mentioned that the old QMP workflow is this:
> >> > >
> >> > >   JSON Parser --> QMP Dispatcher --> Respond
> >> > >   /|\(2)(3) |
> >> > >(1) |   \|/ (4)
> >> > >+-  main thread  +
> >> > >
> >> > > What this series does is, firstly:
> >> > >
> >> > >   JSON Parser QMP Dispatcher --> Respond
> >> > >   /|\ |   /|\   (4) |
> >> > >|  | (2)| (3)|  (5)
> >> > >(1) |  +->  |   \|/
> >> > >+-  main thread  <---+
> >> > >
> >> > > And further:
> >> > >
> >> > >queue/kick
> >> > >  JSON Parser ==> QMP Dispatcher --> Respond
> >> > >  /|\ | (3)   /|\(4)|
> >> > >   (1) |  | (2)||  (5)
> >> > >   | \|/   |   \|/
> >> > > IO thread main thread  <---+
> >> >
> >> > Is the queue per monitor or per client?
> >
> > The queue is currently global. I think yes maybe at least we can do it
> > per monitor, but I am not sure whether that is urgent or can be
> > postponed.  After all 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefa...@redhat.com) wrote:
> On Fri, Sep 15, 2017 at 01:29:13PM +0100, Daniel P. Berrange wrote:
> > On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau 
> > > > > > > > > wrote:
> > > > > > > > > > There should be a limit in the number of requests the 
> > > > > > > > > > thread can
> > > > > > > > > > queue. Before the patch, the limit was enforced by system 
> > > > > > > > > > socket
> > > > > > > > > > buffering I think. Now, should oob commands still be 
> > > > > > > > > > processed even if
> > > > > > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > > > > > 
> > > > > > > > > I agree.
> > > > > > > > > 
> > > > > > > > > Memory usage must be bounded.  The number of requests is less 
> > > > > > > > > important
> > > > > > > > > than the amount of memory consumed by them.
> > > > > > > > > 
> > > > > > > > > Existing QMP clients that send multiple QMP commands without 
> > > > > > > > > waiting for
> > > > > > > > > replies need to rethink their strategy because OOB commands 
> > > > > > > > > cannot be
> > > > > > > > > processed if queued non-OOB commands consume too much memory.
> > > > > > > > 
> > > > > > > > Thanks for pointing out this.  Yes the memory usage problem is 
> > > > > > > > valid,
> > > > > > > > as Markus pointed out as well in previous discussions (in "Flow
> > > > > > > > Control" section of that long reply).  Hopefully this series 
> > > > > > > > basically
> > > > > > > > can work from design prospective, then I'll add this flow 
> > > > > > > > control in
> > > > > > > > next version.
> > > > > > > > 
> > > > > > > > Regarding to what we should do if the limit is reached: Markus
> > > > > > > > provided a few options, but the one I prefer most is that we 
> > > > > > > > don't
> > > > > > > > respond, but send an event showing that a command is dropped.
> > > > > > > > However, I would like it not queued, but a direct reply (after 
> > > > > > > > all,
> > > > > > > > it's an event, and we should not need to care much on ordering 
> > > > > > > > of it).
> > > > > > > > Then we can get rid of the babysitting of those "to be failed"
> > > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > > 
> > > > > > > > I think I also missed at least a unit test for this new 
> > > > > > > > interface.
> > > > > > > > Again, I'll add it after the whole idea is proved solid.  
> > > > > > > > Thanks,
> > > > > > > 
> > > > > > > Another solution: the server reports available receive buffer 
> > > > > > > space to
> > > > > > > the client.  The server only guarantees immediate OOB processing 
> > > > > > > when
> > > > > > > the client stays within the receive buffer size.
> > > > > > > 
> > > > > > > Clients wishing to take advantage of OOB must query the receive 
> > > > > > > buffer
> > > > > > > size and make sure to leave enough room.
> > > > > > 
> > > > > > I don't think having to query it ahead of time is particularly nice,
> > > > > > and of course it is inherantly racy.
> > > > > > 
> > > > > > I would just have QEMU emit an event when it pausing processing of 
> > > > > > the
> > > > > > incoming commands due to a full queue.  If the event includes the ID
> > > > > > of the last queued command, the client will know which (if any) of
> > > > > > its outstanding commands are delayed. Another even can be sent when
> > > > > > it restarts reading.
> > > > > 
> > > > > Hmm and now we're implementing flow control!
> > > > > 
> > > > > a) What exactly is the current semantics/buffer sizes?
> > > > > b) When do clients send multiple QMP commands on one channel without
> > > > > waiting for the response to the previous command?
> > > > > c) Would one queue entry for each class of commands/channel work
> > > > >   (Where a class of commands is currently 'normal' and 'oob')
> > > > 
> > > > I do wonder if we need to worry about request limiting at all from the
> > > > client side.  For non-OOB commands clients will wait for a reply before
> > > > sending a 2nd non-OOB command, so you'll never get a deep queue for.
> > > > 
> > > > OOB commands are supposed to be things which can be handled quickly
> > > > without blocking, so even if a client sent several commands at once
> > > > without waiting for replies, they're going to be processed quickly,
> > > > so whether we temporarily block reading off the wire is a minor
> > > > detail.
> > > 
> > > Lets just define it so that it can't - you send an OOB 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Stefan Hajnoczi]

On Fri, Sep 15, 2017 at 01:29:13PM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau 
> > > > > > > > wrote:
> > > > > > > > > There should be a limit in the number of requests the thread 
> > > > > > > > > can
> > > > > > > > > queue. Before the patch, the limit was enforced by system 
> > > > > > > > > socket
> > > > > > > > > buffering I think. Now, should oob commands still be 
> > > > > > > > > processed even if
> > > > > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > > > > 
> > > > > > > > I agree.
> > > > > > > > 
> > > > > > > > Memory usage must be bounded.  The number of requests is less 
> > > > > > > > important
> > > > > > > > than the amount of memory consumed by them.
> > > > > > > > 
> > > > > > > > Existing QMP clients that send multiple QMP commands without 
> > > > > > > > waiting for
> > > > > > > > replies need to rethink their strategy because OOB commands 
> > > > > > > > cannot be
> > > > > > > > processed if queued non-OOB commands consume too much memory.
> > > > > > > 
> > > > > > > Thanks for pointing out this.  Yes the memory usage problem is 
> > > > > > > valid,
> > > > > > > as Markus pointed out as well in previous discussions (in "Flow
> > > > > > > Control" section of that long reply).  Hopefully this series 
> > > > > > > basically
> > > > > > > can work from design prospective, then I'll add this flow control 
> > > > > > > in
> > > > > > > next version.
> > > > > > > 
> > > > > > > Regarding to what we should do if the limit is reached: Markus
> > > > > > > provided a few options, but the one I prefer most is that we don't
> > > > > > > respond, but send an event showing that a command is dropped.
> > > > > > > However, I would like it not queued, but a direct reply (after 
> > > > > > > all,
> > > > > > > it's an event, and we should not need to care much on ordering of 
> > > > > > > it).
> > > > > > > Then we can get rid of the babysitting of those "to be failed"
> > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > 
> > > > > > > I think I also missed at least a unit test for this new interface.
> > > > > > > Again, I'll add it after the whole idea is proved solid.  Thanks,
> > > > > > 
> > > > > > Another solution: the server reports available receive buffer space 
> > > > > > to
> > > > > > the client.  The server only guarantees immediate OOB processing 
> > > > > > when
> > > > > > the client stays within the receive buffer size.
> > > > > > 
> > > > > > Clients wishing to take advantage of OOB must query the receive 
> > > > > > buffer
> > > > > > size and make sure to leave enough room.
> > > > > 
> > > > > I don't think having to query it ahead of time is particularly nice,
> > > > > and of course it is inherantly racy.
> > > > > 
> > > > > I would just have QEMU emit an event when it pausing processing of the
> > > > > incoming commands due to a full queue.  If the event includes the ID
> > > > > of the last queued command, the client will know which (if any) of
> > > > > its outstanding commands are delayed. Another even can be sent when
> > > > > it restarts reading.
> > > > 
> > > > Hmm and now we're implementing flow control!
> > > > 
> > > > a) What exactly is the current semantics/buffer sizes?
> > > > b) When do clients send multiple QMP commands on one channel without
> > > > waiting for the response to the previous command?
> > > > c) Would one queue entry for each class of commands/channel work
> > > >   (Where a class of commands is currently 'normal' and 'oob')
> > > 
> > > I do wonder if we need to worry about request limiting at all from the
> > > client side.  For non-OOB commands clients will wait for a reply before
> > > sending a 2nd non-OOB command, so you'll never get a deep queue for.
> > > 
> > > OOB commands are supposed to be things which can be handled quickly
> > > without blocking, so even if a client sent several commands at once
> > > without waiting for replies, they're going to be processed quickly,
> > > so whether we temporarily block reading off the wire is a minor
> > > detail.
> > 
> > Lets just define it so that it can't - you send an OOB command and wait
> > for it's response before sending another on that channel.
> > 
> > > IOW, I think we could just have a fixed 10 command queue and apps just
> > > pretend that there's an infinite queue and nothing bad would happen from
> > > the app's POV.
> > 
> > Can you justify 10 as 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Daniel P. Berrange]

On Fri, Sep 15, 2017 at 03:29:46PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berra...@redhat.com) wrote:
> > On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau 
> > > > > > > > > wrote:
> > > > > > > > > > There should be a limit in the number of requests the 
> > > > > > > > > > thread can
> > > > > > > > > > queue. Before the patch, the limit was enforced by system 
> > > > > > > > > > socket
> > > > > > > > > > buffering I think. Now, should oob commands still be 
> > > > > > > > > > processed even if
> > > > > > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > > > > > 
> > > > > > > > > I agree.
> > > > > > > > > 
> > > > > > > > > Memory usage must be bounded.  The number of requests is less 
> > > > > > > > > important
> > > > > > > > > than the amount of memory consumed by them.
> > > > > > > > > 
> > > > > > > > > Existing QMP clients that send multiple QMP commands without 
> > > > > > > > > waiting for
> > > > > > > > > replies need to rethink their strategy because OOB commands 
> > > > > > > > > cannot be
> > > > > > > > > processed if queued non-OOB commands consume too much memory.
> > > > > > > > 
> > > > > > > > Thanks for pointing out this.  Yes the memory usage problem is 
> > > > > > > > valid,
> > > > > > > > as Markus pointed out as well in previous discussions (in "Flow
> > > > > > > > Control" section of that long reply).  Hopefully this series 
> > > > > > > > basically
> > > > > > > > can work from design prospective, then I'll add this flow 
> > > > > > > > control in
> > > > > > > > next version.
> > > > > > > > 
> > > > > > > > Regarding to what we should do if the limit is reached: Markus
> > > > > > > > provided a few options, but the one I prefer most is that we 
> > > > > > > > don't
> > > > > > > > respond, but send an event showing that a command is dropped.
> > > > > > > > However, I would like it not queued, but a direct reply (after 
> > > > > > > > all,
> > > > > > > > it's an event, and we should not need to care much on ordering 
> > > > > > > > of it).
> > > > > > > > Then we can get rid of the babysitting of those "to be failed"
> > > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > > 
> > > > > > > > I think I also missed at least a unit test for this new 
> > > > > > > > interface.
> > > > > > > > Again, I'll add it after the whole idea is proved solid.  
> > > > > > > > Thanks,
> > > > > > > 
> > > > > > > Another solution: the server reports available receive buffer 
> > > > > > > space to
> > > > > > > the client.  The server only guarantees immediate OOB processing 
> > > > > > > when
> > > > > > > the client stays within the receive buffer size.
> > > > > > > 
> > > > > > > Clients wishing to take advantage of OOB must query the receive 
> > > > > > > buffer
> > > > > > > size and make sure to leave enough room.
> > > > > > 
> > > > > > I don't think having to query it ahead of time is particularly nice,
> > > > > > and of course it is inherantly racy.
> > > > > > 
> > > > > > I would just have QEMU emit an event when it pausing processing of 
> > > > > > the
> > > > > > incoming commands due to a full queue.  If the event includes the ID
> > > > > > of the last queued command, the client will know which (if any) of
> > > > > > its outstanding commands are delayed. Another even can be sent when
> > > > > > it restarts reading.
> > > > > 
> > > > > Hmm and now we're implementing flow control!
> > > > > 
> > > > > a) What exactly is the current semantics/buffer sizes?
> > > > > b) When do clients send multiple QMP commands on one channel without
> > > > > waiting for the response to the previous command?
> > > > > c) Would one queue entry for each class of commands/channel work
> > > > >   (Where a class of commands is currently 'normal' and 'oob')
> > > > 
> > > > I do wonder if we need to worry about request limiting at all from the
> > > > client side.  For non-OOB commands clients will wait for a reply before
> > > > sending a 2nd non-OOB command, so you'll never get a deep queue for.
> > > > 
> > > > OOB commands are supposed to be things which can be handled quickly
> > > > without blocking, so even if a client sent several commands at once
> > > > without waiting for replies, they're going to be processed quickly,
> > > > so whether we temporarily block reading off the wire is a minor
> > > > detail.
> > > 
> > > Lets just define it so that it can't - you send an 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau 
> > > > > > > > wrote:
> > > > > > > > > There should be a limit in the number of requests the thread 
> > > > > > > > > can
> > > > > > > > > queue. Before the patch, the limit was enforced by system 
> > > > > > > > > socket
> > > > > > > > > buffering I think. Now, should oob commands still be 
> > > > > > > > > processed even if
> > > > > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > > > > 
> > > > > > > > I agree.
> > > > > > > > 
> > > > > > > > Memory usage must be bounded.  The number of requests is less 
> > > > > > > > important
> > > > > > > > than the amount of memory consumed by them.
> > > > > > > > 
> > > > > > > > Existing QMP clients that send multiple QMP commands without 
> > > > > > > > waiting for
> > > > > > > > replies need to rethink their strategy because OOB commands 
> > > > > > > > cannot be
> > > > > > > > processed if queued non-OOB commands consume too much memory.
> > > > > > > 
> > > > > > > Thanks for pointing out this.  Yes the memory usage problem is 
> > > > > > > valid,
> > > > > > > as Markus pointed out as well in previous discussions (in "Flow
> > > > > > > Control" section of that long reply).  Hopefully this series 
> > > > > > > basically
> > > > > > > can work from design prospective, then I'll add this flow control 
> > > > > > > in
> > > > > > > next version.
> > > > > > > 
> > > > > > > Regarding to what we should do if the limit is reached: Markus
> > > > > > > provided a few options, but the one I prefer most is that we don't
> > > > > > > respond, but send an event showing that a command is dropped.
> > > > > > > However, I would like it not queued, but a direct reply (after 
> > > > > > > all,
> > > > > > > it's an event, and we should not need to care much on ordering of 
> > > > > > > it).
> > > > > > > Then we can get rid of the babysitting of those "to be failed"
> > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > 
> > > > > > > I think I also missed at least a unit test for this new interface.
> > > > > > > Again, I'll add it after the whole idea is proved solid.  Thanks,
> > > > > > 
> > > > > > Another solution: the server reports available receive buffer space 
> > > > > > to
> > > > > > the client.  The server only guarantees immediate OOB processing 
> > > > > > when
> > > > > > the client stays within the receive buffer size.
> > > > > > 
> > > > > > Clients wishing to take advantage of OOB must query the receive 
> > > > > > buffer
> > > > > > size and make sure to leave enough room.
> > > > > 
> > > > > I don't think having to query it ahead of time is particularly nice,
> > > > > and of course it is inherantly racy.
> > > > > 
> > > > > I would just have QEMU emit an event when it pausing processing of the
> > > > > incoming commands due to a full queue.  If the event includes the ID
> > > > > of the last queued command, the client will know which (if any) of
> > > > > its outstanding commands are delayed. Another even can be sent when
> > > > > it restarts reading.
> > > > 
> > > > Hmm and now we're implementing flow control!
> > > > 
> > > > a) What exactly is the current semantics/buffer sizes?
> > > > b) When do clients send multiple QMP commands on one channel without
> > > > waiting for the response to the previous command?
> > > > c) Would one queue entry for each class of commands/channel work
> > > >   (Where a class of commands is currently 'normal' and 'oob')
> > > 
> > > I do wonder if we need to worry about request limiting at all from the
> > > client side.  For non-OOB commands clients will wait for a reply before
> > > sending a 2nd non-OOB command, so you'll never get a deep queue for.
> > > 
> > > OOB commands are supposed to be things which can be handled quickly
> > > without blocking, so even if a client sent several commands at once
> > > without waiting for replies, they're going to be processed quickly,
> > > so whether we temporarily block reading off the wire is a minor
> > > detail.
> > 
> > Lets just define it so that it can't - you send an OOB command and wait
> > for it's response before sending another on that channel.
> > 
> > > IOW, I think we could just have a fixed 10 command queue and apps just
> > > pretend that there's an infinite queue and nothing bad would happen from
> > > the app's POV.
> > 
> > Can you justify 10 as opposed to 1?
> 
> 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Daniel P. Berrange]

On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berra...@redhat.com) wrote:
> > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
> > > > > > > > There should be a limit in the number of requests the thread can
> > > > > > > > queue. Before the patch, the limit was enforced by system socket
> > > > > > > > buffering I think. Now, should oob commands still be processed 
> > > > > > > > even if
> > > > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > > > 
> > > > > > > I agree.
> > > > > > > 
> > > > > > > Memory usage must be bounded.  The number of requests is less 
> > > > > > > important
> > > > > > > than the amount of memory consumed by them.
> > > > > > > 
> > > > > > > Existing QMP clients that send multiple QMP commands without 
> > > > > > > waiting for
> > > > > > > replies need to rethink their strategy because OOB commands 
> > > > > > > cannot be
> > > > > > > processed if queued non-OOB commands consume too much memory.
> > > > > > 
> > > > > > Thanks for pointing out this.  Yes the memory usage problem is 
> > > > > > valid,
> > > > > > as Markus pointed out as well in previous discussions (in "Flow
> > > > > > Control" section of that long reply).  Hopefully this series 
> > > > > > basically
> > > > > > can work from design prospective, then I'll add this flow control in
> > > > > > next version.
> > > > > > 
> > > > > > Regarding to what we should do if the limit is reached: Markus
> > > > > > provided a few options, but the one I prefer most is that we don't
> > > > > > respond, but send an event showing that a command is dropped.
> > > > > > However, I would like it not queued, but a direct reply (after all,
> > > > > > it's an event, and we should not need to care much on ordering of 
> > > > > > it).
> > > > > > Then we can get rid of the babysitting of those "to be failed"
> > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > 
> > > > > > I think I also missed at least a unit test for this new interface.
> > > > > > Again, I'll add it after the whole idea is proved solid.  Thanks,
> > > > > 
> > > > > Another solution: the server reports available receive buffer space to
> > > > > the client.  The server only guarantees immediate OOB processing when
> > > > > the client stays within the receive buffer size.
> > > > > 
> > > > > Clients wishing to take advantage of OOB must query the receive buffer
> > > > > size and make sure to leave enough room.
> > > > 
> > > > I don't think having to query it ahead of time is particularly nice,
> > > > and of course it is inherantly racy.
> > > > 
> > > > I would just have QEMU emit an event when it pausing processing of the
> > > > incoming commands due to a full queue.  If the event includes the ID
> > > > of the last queued command, the client will know which (if any) of
> > > > its outstanding commands are delayed. Another even can be sent when
> > > > it restarts reading.
> > > 
> > > Hmm and now we're implementing flow control!
> > > 
> > > a) What exactly is the current semantics/buffer sizes?
> > > b) When do clients send multiple QMP commands on one channel without
> > > waiting for the response to the previous command?
> > > c) Would one queue entry for each class of commands/channel work
> > >   (Where a class of commands is currently 'normal' and 'oob')
> > 
> > I do wonder if we need to worry about request limiting at all from the
> > client side.  For non-OOB commands clients will wait for a reply before
> > sending a 2nd non-OOB command, so you'll never get a deep queue for.
> > 
> > OOB commands are supposed to be things which can be handled quickly
> > without blocking, so even if a client sent several commands at once
> > without waiting for replies, they're going to be processed quickly,
> > so whether we temporarily block reading off the wire is a minor
> > detail.
> 
> Lets just define it so that it can't - you send an OOB command and wait
> for it's response before sending another on that channel.
> 
> > IOW, I think we could just have a fixed 10 command queue and apps just
> > pretend that there's an infinite queue and nothing bad would happen from
> > the app's POV.
> 
> Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are
being processed sequentially by their thread. A >1 length queue would only
matter for non-OOB commands if an app was filling the pipeline with non-OOB
requests, as then that could block reading of OOB commands. 


Regards,
Daniel
-- 
|: https://berrange.com  -o-

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
> > > > > > > There should be a limit in the number of requests the thread can
> > > > > > > queue. Before the patch, the limit was enforced by system socket
> > > > > > > buffering I think. Now, should oob commands still be processed 
> > > > > > > even if
> > > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > > 
> > > > > > I agree.
> > > > > > 
> > > > > > Memory usage must be bounded.  The number of requests is less 
> > > > > > important
> > > > > > than the amount of memory consumed by them.
> > > > > > 
> > > > > > Existing QMP clients that send multiple QMP commands without 
> > > > > > waiting for
> > > > > > replies need to rethink their strategy because OOB commands cannot 
> > > > > > be
> > > > > > processed if queued non-OOB commands consume too much memory.
> > > > > 
> > > > > Thanks for pointing out this.  Yes the memory usage problem is valid,
> > > > > as Markus pointed out as well in previous discussions (in "Flow
> > > > > Control" section of that long reply).  Hopefully this series basically
> > > > > can work from design prospective, then I'll add this flow control in
> > > > > next version.
> > > > > 
> > > > > Regarding to what we should do if the limit is reached: Markus
> > > > > provided a few options, but the one I prefer most is that we don't
> > > > > respond, but send an event showing that a command is dropped.
> > > > > However, I would like it not queued, but a direct reply (after all,
> > > > > it's an event, and we should not need to care much on ordering of it).
> > > > > Then we can get rid of the babysitting of those "to be failed"
> > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > 
> > > > > I think I also missed at least a unit test for this new interface.
> > > > > Again, I'll add it after the whole idea is proved solid.  Thanks,
> > > > 
> > > > Another solution: the server reports available receive buffer space to
> > > > the client.  The server only guarantees immediate OOB processing when
> > > > the client stays within the receive buffer size.
> > > > 
> > > > Clients wishing to take advantage of OOB must query the receive buffer
> > > > size and make sure to leave enough room.
> > > 
> > > I don't think having to query it ahead of time is particularly nice,
> > > and of course it is inherantly racy.
> > > 
> > > I would just have QEMU emit an event when it pausing processing of the
> > > incoming commands due to a full queue.  If the event includes the ID
> > > of the last queued command, the client will know which (if any) of
> > > its outstanding commands are delayed. Another even can be sent when
> > > it restarts reading.
> > 
> > Hmm and now we're implementing flow control!
> > 
> > a) What exactly is the current semantics/buffer sizes?
> > b) When do clients send multiple QMP commands on one channel without
> > waiting for the response to the previous command?
> > c) Would one queue entry for each class of commands/channel work
> >   (Where a class of commands is currently 'normal' and 'oob')
> 
> I do wonder if we need to worry about request limiting at all from the
> client side.  For non-OOB commands clients will wait for a reply before
> sending a 2nd non-OOB command, so you'll never get a deep queue for.
> 
> OOB commands are supposed to be things which can be handled quickly
> without blocking, so even if a client sent several commands at once
> without waiting for replies, they're going to be processed quickly,
> so whether we temporarily block reading off the wire is a minor
> detail.

Lets just define it so that it can't - you send an OOB command and wait
for it's response before sending another on that channel.

> IOW, I think we could just have a fixed 10 command queue and apps just
> pretend that there's an infinite queue and nothing bad would happen from
> the app's POV.

Can you justify 10 as opposed to 1?

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Daniel P. Berrange]

On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berra...@redhat.com) wrote:
> > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
> > > > > > There should be a limit in the number of requests the thread can
> > > > > > queue. Before the patch, the limit was enforced by system socket
> > > > > > buffering I think. Now, should oob commands still be processed even 
> > > > > > if
> > > > > > the queue is full? If so, the thread can't be suspended.
> > > > > 
> > > > > I agree.
> > > > > 
> > > > > Memory usage must be bounded.  The number of requests is less 
> > > > > important
> > > > > than the amount of memory consumed by them.
> > > > > 
> > > > > Existing QMP clients that send multiple QMP commands without waiting 
> > > > > for
> > > > > replies need to rethink their strategy because OOB commands cannot be
> > > > > processed if queued non-OOB commands consume too much memory.
> > > > 
> > > > Thanks for pointing out this.  Yes the memory usage problem is valid,
> > > > as Markus pointed out as well in previous discussions (in "Flow
> > > > Control" section of that long reply).  Hopefully this series basically
> > > > can work from design prospective, then I'll add this flow control in
> > > > next version.
> > > > 
> > > > Regarding to what we should do if the limit is reached: Markus
> > > > provided a few options, but the one I prefer most is that we don't
> > > > respond, but send an event showing that a command is dropped.
> > > > However, I would like it not queued, but a direct reply (after all,
> > > > it's an event, and we should not need to care much on ordering of it).
> > > > Then we can get rid of the babysitting of those "to be failed"
> > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > 
> > > > I think I also missed at least a unit test for this new interface.
> > > > Again, I'll add it after the whole idea is proved solid.  Thanks,
> > > 
> > > Another solution: the server reports available receive buffer space to
> > > the client.  The server only guarantees immediate OOB processing when
> > > the client stays within the receive buffer size.
> > > 
> > > Clients wishing to take advantage of OOB must query the receive buffer
> > > size and make sure to leave enough room.
> > 
> > I don't think having to query it ahead of time is particularly nice,
> > and of course it is inherantly racy.
> > 
> > I would just have QEMU emit an event when it pausing processing of the
> > incoming commands due to a full queue.  If the event includes the ID
> > of the last queued command, the client will know which (if any) of
> > its outstanding commands are delayed. Another even can be sent when
> > it restarts reading.
> 
> Hmm and now we're implementing flow control!
> 
> a) What exactly is the current semantics/buffer sizes?
> b) When do clients send multiple QMP commands on one channel without
> waiting for the response to the previous command?
> c) Would one queue entry for each class of commands/channel work
>   (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the
client side.  For non-OOB commands clients will wait for a reply before
sending a 2nd non-OOB command, so you'll never get a deep queue for.

OOB commands are supposed to be things which can be handled quickly
without blocking, so even if a client sent several commands at once
without waiting for replies, they're going to be processed quickly,
so whether we temporarily block reading off the wire is a minor
detail.

IOW, I think we could just have a fixed 10 command queue and apps just
pretend that there's an infinite queue and nothing bad would happen from
the app's POV.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
> > > > > There should be a limit in the number of requests the thread can
> > > > > queue. Before the patch, the limit was enforced by system socket
> > > > > buffering I think. Now, should oob commands still be processed even if
> > > > > the queue is full? If so, the thread can't be suspended.
> > > > 
> > > > I agree.
> > > > 
> > > > Memory usage must be bounded.  The number of requests is less important
> > > > than the amount of memory consumed by them.
> > > > 
> > > > Existing QMP clients that send multiple QMP commands without waiting for
> > > > replies need to rethink their strategy because OOB commands cannot be
> > > > processed if queued non-OOB commands consume too much memory.
> > > 
> > > Thanks for pointing out this.  Yes the memory usage problem is valid,
> > > as Markus pointed out as well in previous discussions (in "Flow
> > > Control" section of that long reply).  Hopefully this series basically
> > > can work from design prospective, then I'll add this flow control in
> > > next version.
> > > 
> > > Regarding to what we should do if the limit is reached: Markus
> > > provided a few options, but the one I prefer most is that we don't
> > > respond, but send an event showing that a command is dropped.
> > > However, I would like it not queued, but a direct reply (after all,
> > > it's an event, and we should not need to care much on ordering of it).
> > > Then we can get rid of the babysitting of those "to be failed"
> > > requests asap, meanwhile we don't lose anything IMHO.
> > > 
> > > I think I also missed at least a unit test for this new interface.
> > > Again, I'll add it after the whole idea is proved solid.  Thanks,
> > 
> > Another solution: the server reports available receive buffer space to
> > the client.  The server only guarantees immediate OOB processing when
> > the client stays within the receive buffer size.
> > 
> > Clients wishing to take advantage of OOB must query the receive buffer
> > size and make sure to leave enough room.
> 
> I don't think having to query it ahead of time is particularly nice,
> and of course it is inherantly racy.
> 
> I would just have QEMU emit an event when it pausing processing of the
> incoming commands due to a full queue.  If the event includes the ID
> of the last queued command, the client will know which (if any) of
> its outstanding commands are delayed. Another even can be sent when
> it restarts reading.

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without
waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work
  (Where a class of commands is currently 'normal' and 'oob')

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Daniel P. Berrange]

On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
> > > > There should be a limit in the number of requests the thread can
> > > > queue. Before the patch, the limit was enforced by system socket
> > > > buffering I think. Now, should oob commands still be processed even if
> > > > the queue is full? If so, the thread can't be suspended.
> > > 
> > > I agree.
> > > 
> > > Memory usage must be bounded.  The number of requests is less important
> > > than the amount of memory consumed by them.
> > > 
> > > Existing QMP clients that send multiple QMP commands without waiting for
> > > replies need to rethink their strategy because OOB commands cannot be
> > > processed if queued non-OOB commands consume too much memory.
> > 
> > Thanks for pointing out this.  Yes the memory usage problem is valid,
> > as Markus pointed out as well in previous discussions (in "Flow
> > Control" section of that long reply).  Hopefully this series basically
> > can work from design prospective, then I'll add this flow control in
> > next version.
> > 
> > Regarding to what we should do if the limit is reached: Markus
> > provided a few options, but the one I prefer most is that we don't
> > respond, but send an event showing that a command is dropped.
> > However, I would like it not queued, but a direct reply (after all,
> > it's an event, and we should not need to care much on ordering of it).
> > Then we can get rid of the babysitting of those "to be failed"
> > requests asap, meanwhile we don't lose anything IMHO.
> > 
> > I think I also missed at least a unit test for this new interface.
> > Again, I'll add it after the whole idea is proved solid.  Thanks,
> 
> Another solution: the server reports available receive buffer space to
> the client.  The server only guarantees immediate OOB processing when
> the client stays within the receive buffer size.
> 
> Clients wishing to take advantage of OOB must query the receive buffer
> size and make sure to leave enough room.

I don't think having to query it ahead of time is particularly nice,
and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the
incoming commands due to a full queue.  If the event includes the ID
of the last queued command, the client will know which (if any) of
its outstanding commands are delayed. Another even can be sent when
it restarts reading.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Marc-André Lureau]

Hi

On Thu, Sep 14, 2017 at 9:46 PM, Peter Xu  wrote:
> On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert wrote:
>> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
>> > Hi
>> >
>> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  wrote:
>> > > This series was born from this one:
>> > >
>> > >   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
>> > >
>> > > The design comes from Markus, and also the whole-bunch-of discussions
>> > > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
>> > > Stefan, etc. on discussing the topic (...again!), providing shiny
>> > > ideas and suggestions.  Finally we got such a solution that seems to
>> > > satisfy everyone.
>> > >
>> > > I re-started the versioning since this series is totally different
>> > > from previous one.  Now it's version 1.
>> > >
>> > > In case new reviewers come along the way without reading previous
>> > > discussions, I will try to do a summary on what this is all about.
>> > >
>> > > What is OOB execution?
>> > > ==
>> > >
>> > > It's the shortcut of Out-Of-Band execution, its name is given by
>> > > Markus.  It's a way to quickly execute a QMP request.  Say, originally
>> > > QMP is going throw these steps:
>> > >
>> > >   JSON Parser --> QMP Dispatcher --> Respond
>> > >   /|\(2)(3) |
>> > >(1) |   \|/ (4)
>> > >+-  main thread  +
>> > >
>> > > The requests are executed by the so-called QMP-dispatcher after the
>> > > JSON is parsed.  If OOB is on, we run the command directly in the
>> > > parser and quickly returns.
>> >
>> > All commands should have the "id" field mandatory in this case, else
>> > the client will not distinguish the replies coming from the last/oob
>> > and the previous commands.
>> >
>> > This should probably be enforced upfront by client capability checks,
>> > more below.
>
> Hmm yes since the oob commands are actually running in async way,
> request ID should be needed here.  However I'm not sure whether
> enabling the whole "request ID" thing is too big for this "try to be
> small" oob change... And IMHO it suites better to be part of the whole
> async work (no matter which implementation we'll use).
>
> How about this: we make "id" mandatory for "run-oob" requests only.
> For oob commands, they will always have ID then no ordering issue, and
> we can do it async; for the rest of non-oob commands, we still allow
> them to go without ID, and since they are not oob, they'll always be
> done in order as well.  Would this work?

This mixed-mode is imho more complicated to deal with than having the
protocol enforced one way or the other, but that should work.

>
>> >
>> > > Yeah I know in current code the parser calls dispatcher directly
>> > > (please see handle_qmp_command()).  However it's not true again after
>> > > this series (parser will has its own IO thread, and dispatcher will
>> > > still be run in main thread).  So this OOB does brings something
>> > > different.
>> > >
>> > > There are more details on why OOB and the difference/relationship
>> > > between OOB, async QMP, block/general jobs, etc.. but IMHO that's
>> > > slightly out of topic (and believe me, it's not easy for me to
>> > > summarize that).  For more information, please refers to [1].
>> > >
>> > > Summary ends here.
>> > >
>> > > Some Implementation Details
>> > > ===
>> > >
>> > > Again, I mentioned that the old QMP workflow is this:
>> > >
>> > >   JSON Parser --> QMP Dispatcher --> Respond
>> > >   /|\(2)(3) |
>> > >(1) |   \|/ (4)
>> > >+-  main thread  +
>> > >
>> > > What this series does is, firstly:
>> > >
>> > >   JSON Parser QMP Dispatcher --> Respond
>> > >   /|\ |   /|\   (4) |
>> > >|  | (2)| (3)|  (5)
>> > >(1) |  +->  |   \|/
>> > >+-  main thread  <---+
>> > >
>> > > And further:
>> > >
>> > >queue/kick
>> > >  JSON Parser ==> QMP Dispatcher --> Respond
>> > >  /|\ | (3)   /|\(4)|
>> > >   (1) |  | (2)||  (5)
>> > >   | \|/   |   \|/
>> > > IO thread main thread  <---+
>> >
>> > Is the queue per monitor or per client?
>
> The queue is currently global. I think yes maybe at least we can do it
> per monitor, but I am not sure whether that is urgent or can be
> postponed.  After all now QMPRequest (please refer to patch 11) is
> defined as (mon, id, req) tuple, so at least "id" namespace is
> per-monitor.
>
>> > And is the dispatching going
>> > to be processed even if the client is disconnected, and are new
>> > clients going to receive the replies from 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Stefan Hajnoczi]

On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu wrote:
> On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
> > > There should be a limit in the number of requests the thread can
> > > queue. Before the patch, the limit was enforced by system socket
> > > buffering I think. Now, should oob commands still be processed even if
> > > the queue is full? If so, the thread can't be suspended.
> > 
> > I agree.
> > 
> > Memory usage must be bounded.  The number of requests is less important
> > than the amount of memory consumed by them.
> > 
> > Existing QMP clients that send multiple QMP commands without waiting for
> > replies need to rethink their strategy because OOB commands cannot be
> > processed if queued non-OOB commands consume too much memory.
> 
> Thanks for pointing out this.  Yes the memory usage problem is valid,
> as Markus pointed out as well in previous discussions (in "Flow
> Control" section of that long reply).  Hopefully this series basically
> can work from design prospective, then I'll add this flow control in
> next version.
> 
> Regarding to what we should do if the limit is reached: Markus
> provided a few options, but the one I prefer most is that we don't
> respond, but send an event showing that a command is dropped.
> However, I would like it not queued, but a direct reply (after all,
> it's an event, and we should not need to care much on ordering of it).
> Then we can get rid of the babysitting of those "to be failed"
> requests asap, meanwhile we don't lose anything IMHO.
> 
> I think I also missed at least a unit test for this new interface.
> Again, I'll add it after the whole idea is proved solid.  Thanks,

Another solution: the server reports available receive buffer space to
the client.  The server only guarantees immediate OOB processing when
the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer
size and make sure to leave enough room.

The advantage of this approach is that the semantics are backwards
compatible (existing clients may continue to queue as many commands as
they wish) and it requires no new behavior in the client (no new QMP
event code path).

Stefan

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Thu, Sep 14, 2017 at 07:53:15PM +0100, Dr. David Alan Gilbert wrote:
> * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > Hi
> > 
> > On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  wrote:
> > > This series was born from this one:
> > >
> > >   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> > >
> > > The design comes from Markus, and also the whole-bunch-of discussions
> > > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
> > > Stefan, etc. on discussing the topic (...again!), providing shiny
> > > ideas and suggestions.  Finally we got such a solution that seems to
> > > satisfy everyone.
> > >
> > > I re-started the versioning since this series is totally different
> > > from previous one.  Now it's version 1.
> > >
> > > In case new reviewers come along the way without reading previous
> > > discussions, I will try to do a summary on what this is all about.
> > >
> > > What is OOB execution?
> > > ==
> > >
> > > It's the shortcut of Out-Of-Band execution, its name is given by
> > > Markus.  It's a way to quickly execute a QMP request.  Say, originally
> > > QMP is going throw these steps:
> > >
> > >   JSON Parser --> QMP Dispatcher --> Respond
> > >   /|\(2)(3) |
> > >(1) |   \|/ (4)
> > >+-  main thread  +
> > >
> > > The requests are executed by the so-called QMP-dispatcher after the
> > > JSON is parsed.  If OOB is on, we run the command directly in the
> > > parser and quickly returns.
> > 
> > All commands should have the "id" field mandatory in this case, else
> > the client will not distinguish the replies coming from the last/oob
> > and the previous commands.
> > 
> > This should probably be enforced upfront by client capability checks,
> > more below.

Hmm yes since the oob commands are actually running in async way,
request ID should be needed here.  However I'm not sure whether
enabling the whole "request ID" thing is too big for this "try to be
small" oob change... And IMHO it suites better to be part of the whole
async work (no matter which implementation we'll use).

How about this: we make "id" mandatory for "run-oob" requests only.
For oob commands, they will always have ID then no ordering issue, and
we can do it async; for the rest of non-oob commands, we still allow
them to go without ID, and since they are not oob, they'll always be
done in order as well.  Would this work?

> > 
> > > Yeah I know in current code the parser calls dispatcher directly
> > > (please see handle_qmp_command()).  However it's not true again after
> > > this series (parser will has its own IO thread, and dispatcher will
> > > still be run in main thread).  So this OOB does brings something
> > > different.
> > >
> > > There are more details on why OOB and the difference/relationship
> > > between OOB, async QMP, block/general jobs, etc.. but IMHO that's
> > > slightly out of topic (and believe me, it's not easy for me to
> > > summarize that).  For more information, please refers to [1].
> > >
> > > Summary ends here.
> > >
> > > Some Implementation Details
> > > ===
> > >
> > > Again, I mentioned that the old QMP workflow is this:
> > >
> > >   JSON Parser --> QMP Dispatcher --> Respond
> > >   /|\(2)(3) |
> > >(1) |   \|/ (4)
> > >+-  main thread  +
> > >
> > > What this series does is, firstly:
> > >
> > >   JSON Parser QMP Dispatcher --> Respond
> > >   /|\ |   /|\   (4) |
> > >|  | (2)| (3)|  (5)
> > >(1) |  +->  |   \|/
> > >+-  main thread  <---+
> > >
> > > And further:
> > >
> > >queue/kick
> > >  JSON Parser ==> QMP Dispatcher --> Respond
> > >  /|\ | (3)   /|\(4)|
> > >   (1) |  | (2)||  (5)
> > >   | \|/   |   \|/
> > > IO thread main thread  <---+
> > 
> > Is the queue per monitor or per client?

The queue is currently global. I think yes maybe at least we can do it
per monitor, but I am not sure whether that is urgent or can be
postponed.  After all now QMPRequest (please refer to patch 11) is
defined as (mon, id, req) tuple, so at least "id" namespace is
per-monitor.

> > And is the dispatching going
> > to be processed even if the client is disconnected, and are new
> > clients going to receive the replies from previous clients
> > commands?

[1]

(will discuss together below)

> > I
> > believe there should be a per-client context, so there won't be "id"
> > request conflicts.

I'd say I am not familiar with this "client" idea, since after all
IMHO one monitor is currently designed to mostly work with a single
client. Say, unix sockets, 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Thu, Sep 14, 2017 at 07:56:04PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > This series was born from this one:
> > 
> >   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> 
> Are patches 1..6 separable and mergable without the rest ?

Yes I think so.

(I was always trying to put pre-requisite patches like these ones at
 the front of any of my series rather than separating them into more
 series, since I thought it is convenient for me to manage them (or
 add new ones when respin), and also easier for reviewers (so people
 don't need to try to find the dependencies).  And since I put them at
 the head, we can easily merge them without rebasing issue when they
 are good while the rest may still need further work.  Hopefully this
 is the right thing to do.)

-- 
Peter Xu

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
> On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
> > There should be a limit in the number of requests the thread can
> > queue. Before the patch, the limit was enforced by system socket
> > buffering I think. Now, should oob commands still be processed even if
> > the queue is full? If so, the thread can't be suspended.
> 
> I agree.
> 
> Memory usage must be bounded.  The number of requests is less important
> than the amount of memory consumed by them.
> 
> Existing QMP clients that send multiple QMP commands without waiting for
> replies need to rethink their strategy because OOB commands cannot be
> processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this.  Yes the memory usage problem is valid,
as Markus pointed out as well in previous discussions (in "Flow
Control" section of that long reply).  Hopefully this series basically
can work from design prospective, then I'll add this flow control in
next version.

Regarding to what we should do if the limit is reached: Markus
provided a few options, but the one I prefer most is that we don't
respond, but send an event showing that a command is dropped.
However, I would like it not queued, but a direct reply (after all,
it's an event, and we should not need to care much on ordering of it).
Then we can get rid of the babysitting of those "to be failed"
requests asap, meanwhile we don't lose anything IMHO.

I think I also missed at least a unit test for this new interface.
Again, I'll add it after the whole idea is proved solid.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> This series was born from this one:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

Are patches 1..6 separable and mergable without the rest ?

Dave

> The design comes from Markus, and also the whole-bunch-of discussions
> in previous thread.  My heartful thanks to Markus, Daniel, Dave,
> Stefan, etc. on discussing the topic (...again!), providing shiny
> ideas and suggestions.  Finally we got such a solution that seems to
> satisfy everyone.
> 
> I re-started the versioning since this series is totally different
> from previous one.  Now it's version 1.
> 
> In case new reviewers come along the way without reading previous
> discussions, I will try to do a summary on what this is all about.
> 
> What is OOB execution?
> ==
> 
> It's the shortcut of Out-Of-Band execution, its name is given by
> Markus.  It's a way to quickly execute a QMP request.  Say, originally
> QMP is going throw these steps:
> 
>   JSON Parser --> QMP Dispatcher --> Respond
>   /|\(2)(3) |
>(1) |   \|/ (4)
>+-  main thread  +
> 
> The requests are executed by the so-called QMP-dispatcher after the
> JSON is parsed.  If OOB is on, we run the command directly in the
> parser and quickly returns.
> 
> Yeah I know in current code the parser calls dispatcher directly
> (please see handle_qmp_command()).  However it's not true again after
> this series (parser will has its own IO thread, and dispatcher will
> still be run in main thread).  So this OOB does brings something
> different.
> 
> There are more details on why OOB and the difference/relationship
> between OOB, async QMP, block/general jobs, etc.. but IMHO that's
> slightly out of topic (and believe me, it's not easy for me to
> summarize that).  For more information, please refers to [1].
> 
> Summary ends here.
> 
> Some Implementation Details
> ===
> 
> Again, I mentioned that the old QMP workflow is this:
> 
>   JSON Parser --> QMP Dispatcher --> Respond
>   /|\(2)(3) |
>(1) |   \|/ (4)
>+-  main thread  +
> 
> What this series does is, firstly:
> 
>   JSON Parser QMP Dispatcher --> Respond
>   /|\ |   /|\   (4) |
>|  | (2)| (3)|  (5)
>(1) |  +->  |   \|/
>+-  main thread  <---+
> 
> And further:
> 
>queue/kick
>  JSON Parser ==> QMP Dispatcher --> Respond
>  /|\ | (3)   /|\(4)|
>   (1) |  | (2)||  (5)
>   | \|/   |   \|/
> IO thread main thread  <---+
> 
> Then it introduced the "allow-oob" parameter in QAPI schema to define
> commands, and "run-oob" flag to let oob-allowed command to run in the
> parser.
> 
> The last patch enables this for "migrate-incoming" command.
> 
> Please review.  Thanks.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> 
> Peter Xu (15):
>   char-io: fix possible race on IOWatchPoll
>   qobject: allow NULL for qstring_get_str()
>   qobject: introduce qobject_to_str()
>   monitor: move skip_flush into monitor_data_init
>   qjson: add "opaque" field to JSONMessageParser
>   monitor: move the cur_mon hack deeper for QMP
>   monitor: unify global init
>   monitor: create IO thread
>   monitor: allow to use IO thread for parsing
>   monitor: introduce monitor_qmp_respond()
>   monitor: separate QMP parser and dispatcher
>   monitor: enable IO thread for (qmp & !mux) typed
>   qapi: introduce new cmd option "allow-oob"
>   qmp: support out-of-band (oob) execution
>   qmp: let migrate-incoming allow out-of-band
> 
>  chardev/char-io.c|  15 ++-
>  docs/devel/qapi-code-gen.txt |  51 ++-
>  include/monitor/monitor.h|   2 +-
>  include/qapi/qmp/dispatch.h  |   2 +
>  include/qapi/qmp/json-streamer.h |   8 +-
>  include/qapi/qmp/qstring.h   |   1 +
>  monitor.c| 283 
> +++
>  qapi/introspect.json |   6 +-
>  qapi/migration.json  |   3 +-
>  qapi/qmp-dispatch.c  |  34 +
>  qga/main.c   |   5 +-
>  qobject/json-streamer.c  |   7 +-
>  qobject/qjson.c  |   5 +-
>  qobject/qstring.c|  13 +-
>  scripts/qapi-commands.py |  19 ++-
>  scripts/qapi-introspect.py   |  10 +-
>  scripts/qapi.py  |  15 ++-
>  scripts/qapi2texi.py |   2 +-
>  tests/libqtest.c |   5 +-
>  tests/qapi-schema/test-qapi.py   |   2 +-
>  trace-events |   2 +
>  vl.c |   3 +-
>  22 files changed, 398 insertions(+), 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
> 
> On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  wrote:
> > This series was born from this one:
> >
> >   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
> >
> > The design comes from Markus, and also the whole-bunch-of discussions
> > in previous thread.  My heartful thanks to Markus, Daniel, Dave,
> > Stefan, etc. on discussing the topic (...again!), providing shiny
> > ideas and suggestions.  Finally we got such a solution that seems to
> > satisfy everyone.
> >
> > I re-started the versioning since this series is totally different
> > from previous one.  Now it's version 1.
> >
> > In case new reviewers come along the way without reading previous
> > discussions, I will try to do a summary on what this is all about.
> >
> > What is OOB execution?
> > ==
> >
> > It's the shortcut of Out-Of-Band execution, its name is given by
> > Markus.  It's a way to quickly execute a QMP request.  Say, originally
> > QMP is going throw these steps:
> >
> >   JSON Parser --> QMP Dispatcher --> Respond
> >   /|\(2)(3) |
> >(1) |   \|/ (4)
> >+-  main thread  +
> >
> > The requests are executed by the so-called QMP-dispatcher after the
> > JSON is parsed.  If OOB is on, we run the command directly in the
> > parser and quickly returns.
> 
> All commands should have the "id" field mandatory in this case, else
> the client will not distinguish the replies coming from the last/oob
> and the previous commands.
> 
> This should probably be enforced upfront by client capability checks,
> more below.
> 
> > Yeah I know in current code the parser calls dispatcher directly
> > (please see handle_qmp_command()).  However it's not true again after
> > this series (parser will has its own IO thread, and dispatcher will
> > still be run in main thread).  So this OOB does brings something
> > different.
> >
> > There are more details on why OOB and the difference/relationship
> > between OOB, async QMP, block/general jobs, etc.. but IMHO that's
> > slightly out of topic (and believe me, it's not easy for me to
> > summarize that).  For more information, please refers to [1].
> >
> > Summary ends here.
> >
> > Some Implementation Details
> > ===
> >
> > Again, I mentioned that the old QMP workflow is this:
> >
> >   JSON Parser --> QMP Dispatcher --> Respond
> >   /|\(2)(3) |
> >(1) |   \|/ (4)
> >+-  main thread  +
> >
> > What this series does is, firstly:
> >
> >   JSON Parser QMP Dispatcher --> Respond
> >   /|\ |   /|\   (4) |
> >|  | (2)| (3)|  (5)
> >(1) |  +->  |   \|/
> >+-  main thread  <---+
> >
> > And further:
> >
> >queue/kick
> >  JSON Parser ==> QMP Dispatcher --> Respond
> >  /|\ | (3)   /|\(4)|
> >   (1) |  | (2)||  (5)
> >   | \|/   |   \|/
> > IO thread main thread  <---+
> 
> Is the queue per monitor or per client? And is the dispatching going
> to be processed even if the client is disconnected, and are new
> clients going to receive the replies from previous clients commands? I
> believe there should be a per-client context, so there won't be "id"
> request conflicts.
> 
> >
> > Then it introduced the "allow-oob" parameter in QAPI schema to define
> > commands, and "run-oob" flag to let oob-allowed command to run in the
> > parser.
> 
> From a protocol point of view, I find that "run-oob" distinction per
> command a bit pointless. It helps with legacy client that wouldn't
> expect out-of-order replies if qemu were to run oob commands oob by
> default though. Clients shouldn't care about how/where a command is
> being queued or not. If they send a command, they want it processed as
> quickly as possible. However, it can be interesting to know if the
> implementation of the command will be able to deliver oob, so that
> data in the introspection could be useful.
> 
> I would rather propose a client/server capability in qmp_capabilities,
> call it "oob":
> 
> This capability indicates oob commands support.

The problem is indicating which commands support oob as opposed to
indicating whether oob is present at all.  Future versions will
probably make more commands oob-able and a client will want to know
whether it can rely on a particular command being non-blocking.

> An oob command is a regular client message request with the "id"
> member mandatory, but the reply may be delivered
> out of order by the server if the client supports
> it too.
> 
> If both the server and the client have the "oob" capability, the
> server can handle 

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Stefan Hajnoczi]

On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
> There should be a limit in the number of requests the thread can
> queue. Before the patch, the limit was enforced by system socket
> buffering I think. Now, should oob commands still be processed even if
> the queue is full? If so, the thread can't be suspended.

I agree.

Memory usage must be bounded.  The number of requests is less important
than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for
replies need to rethink their strategy because OOB commands cannot be
processed if queued non-OOB commands consume too much memory.

Stefan

Re: [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Marc-André Lureau]

Hi

On Thu, Sep 14, 2017 at 9:50 AM, Peter Xu  wrote:
> This series was born from this one:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
>
> The design comes from Markus, and also the whole-bunch-of discussions
> in previous thread.  My heartful thanks to Markus, Daniel, Dave,
> Stefan, etc. on discussing the topic (...again!), providing shiny
> ideas and suggestions.  Finally we got such a solution that seems to
> satisfy everyone.
>
> I re-started the versioning since this series is totally different
> from previous one.  Now it's version 1.
>
> In case new reviewers come along the way without reading previous
> discussions, I will try to do a summary on what this is all about.
>
> What is OOB execution?
> ==
>
> It's the shortcut of Out-Of-Band execution, its name is given by
> Markus.  It's a way to quickly execute a QMP request.  Say, originally
> QMP is going throw these steps:
>
>   JSON Parser --> QMP Dispatcher --> Respond
>   /|\(2)(3) |
>(1) |   \|/ (4)
>+-  main thread  +
>
> The requests are executed by the so-called QMP-dispatcher after the
> JSON is parsed.  If OOB is on, we run the command directly in the
> parser and quickly returns.

All commands should have the "id" field mandatory in this case, else
the client will not distinguish the replies coming from the last/oob
and the previous commands.

This should probably be enforced upfront by client capability checks,
more below.

> Yeah I know in current code the parser calls dispatcher directly
> (please see handle_qmp_command()).  However it's not true again after
> this series (parser will has its own IO thread, and dispatcher will
> still be run in main thread).  So this OOB does brings something
> different.
>
> There are more details on why OOB and the difference/relationship
> between OOB, async QMP, block/general jobs, etc.. but IMHO that's
> slightly out of topic (and believe me, it's not easy for me to
> summarize that).  For more information, please refers to [1].
>
> Summary ends here.
>
> Some Implementation Details
> ===
>
> Again, I mentioned that the old QMP workflow is this:
>
>   JSON Parser --> QMP Dispatcher --> Respond
>   /|\(2)(3) |
>(1) |   \|/ (4)
>+-  main thread  +
>
> What this series does is, firstly:
>
>   JSON Parser QMP Dispatcher --> Respond
>   /|\ |   /|\   (4) |
>|  | (2)| (3)|  (5)
>(1) |  +->  |   \|/
>+-  main thread  <---+
>
> And further:
>
>queue/kick
>  JSON Parser ==> QMP Dispatcher --> Respond
>  /|\ | (3)   /|\(4)|
>   (1) |  | (2)||  (5)
>   | \|/   |   \|/
> IO thread main thread  <---+

Is the queue per monitor or per client? And is the dispatching going
to be processed even if the client is disconnected, and are new
clients going to receive the replies from previous clients commands? I
believe there should be a per-client context, so there won't be "id"
request conflicts.

>
> Then it introduced the "allow-oob" parameter in QAPI schema to define
> commands, and "run-oob" flag to let oob-allowed command to run in the
> parser.

>From a protocol point of view, I find that "run-oob" distinction per
command a bit pointless. It helps with legacy client that wouldn't
expect out-of-order replies if qemu were to run oob commands oob by
default though. Clients shouldn't care about how/where a command is
being queued or not. If they send a command, they want it processed as
quickly as possible. However, it can be interesting to know if the
implementation of the command will be able to deliver oob, so that
data in the introspection could be useful.

I would rather propose a client/server capability in qmp_capabilities,
call it "oob":

This capability indicates oob commands support.

An oob command is a regular client message request with the "id"
member mandatory, but the reply may be delivered
out of order by the server if the client supports
it too.

If both the server and the client have the "oob" capability, the
server can handle new client requests while previous requests are being
processed.

If the client doesn't have the "oob" capability, it may still call
an oob command, and make multiple outstanding calls. In this case,
the commands are processed in order, so the replies will also be in
order. The "id" member isn't mandatory in this case.

The client should match the replies with the "id" member associated
with the requests.

When a client is disconnected, the pending commands are not
necessarily cancelled. But the future clients will not get replies from

[Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support

[Peter Xu]

This series was born from this one:

  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

The design comes from Markus, and also the whole-bunch-of discussions
in previous thread.  My heartful thanks to Markus, Daniel, Dave,
Stefan, etc. on discussing the topic (...again!), providing shiny
ideas and suggestions.  Finally we got such a solution that seems to
satisfy everyone.

I re-started the versioning since this series is totally different
from previous one.  Now it's version 1.

In case new reviewers come along the way without reading previous
discussions, I will try to do a summary on what this is all about.

What is OOB execution?
==

It's the shortcut of Out-Of-Band execution, its name is given by
Markus.  It's a way to quickly execute a QMP request.  Say, originally
QMP is going throw these steps:

  JSON Parser --> QMP Dispatcher --> Respond
  /|\(2)(3) |
   (1) |   \|/ (4)
   +-  main thread  +

The requests are executed by the so-called QMP-dispatcher after the
JSON is parsed.  If OOB is on, we run the command directly in the
parser and quickly returns.

Yeah I know in current code the parser calls dispatcher directly
(please see handle_qmp_command()).  However it's not true again after
this series (parser will has its own IO thread, and dispatcher will
still be run in main thread).  So this OOB does brings something
different.

There are more details on why OOB and the difference/relationship
between OOB, async QMP, block/general jobs, etc.. but IMHO that's
slightly out of topic (and believe me, it's not easy for me to
summarize that).  For more information, please refers to [1].

Summary ends here.

Some Implementation Details
===

Again, I mentioned that the old QMP workflow is this:

  JSON Parser --> QMP Dispatcher --> Respond
  /|\(2)(3) |
   (1) |   \|/ (4)
   +-  main thread  +

What this series does is, firstly:

  JSON Parser QMP Dispatcher --> Respond
  /|\ |   /|\   (4) |
   |  | (2)| (3)|  (5)
   (1) |  +->  |   \|/
   +-  main thread  <---+

And further:

   queue/kick
 JSON Parser ==> QMP Dispatcher --> Respond
 /|\ | (3)   /|\(4)|
  (1) |  | (2)||  (5)
  | \|/   |   \|/
IO thread main thread  <---+

Then it introduced the "allow-oob" parameter in QAPI schema to define
commands, and "run-oob" flag to let oob-allowed command to run in the
parser.

The last patch enables this for "migrate-incoming" command.

Please review.  Thanks.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

Peter Xu (15):
  char-io: fix possible race on IOWatchPoll
  qobject: allow NULL for qstring_get_str()
  qobject: introduce qobject_to_str()
  monitor: move skip_flush into monitor_data_init
  qjson: add "opaque" field to JSONMessageParser
  monitor: move the cur_mon hack deeper for QMP
  monitor: unify global init
  monitor: create IO thread
  monitor: allow to use IO thread for parsing
  monitor: introduce monitor_qmp_respond()
  monitor: separate QMP parser and dispatcher
  monitor: enable IO thread for (qmp & !mux) typed
  qapi: introduce new cmd option "allow-oob"
  qmp: support out-of-band (oob) execution
  qmp: let migrate-incoming allow out-of-band

 chardev/char-io.c|  15 ++-
 docs/devel/qapi-code-gen.txt |  51 ++-
 include/monitor/monitor.h|   2 +-
 include/qapi/qmp/dispatch.h  |   2 +
 include/qapi/qmp/json-streamer.h |   8 +-
 include/qapi/qmp/qstring.h   |   1 +
 monitor.c| 283 +++
 qapi/introspect.json |   6 +-
 qapi/migration.json  |   3 +-
 qapi/qmp-dispatch.c  |  34 +
 qga/main.c   |   5 +-
 qobject/json-streamer.c  |   7 +-
 qobject/qjson.c  |   5 +-
 qobject/qstring.c|  13 +-
 scripts/qapi-commands.py |  19 ++-
 scripts/qapi-introspect.py   |  10 +-
 scripts/qapi.py  |  15 ++-
 scripts/qapi2texi.py |   2 +-
 tests/libqtest.c |   5 +-
 tests/qapi-schema/test-qapi.py   |   2 +-
 trace-events |   2 +
 vl.c |   3 +-
 22 files changed, 398 insertions(+), 95 deletions(-)

-- 
2.7.4