Re: [Qemu-devel] monitor: enable OOB by default

[Peter Xu]

On Thu, Jun 28, 2018 at 03:20:41PM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > On Wed, Jun 27, 2018 at 03:13:57PM +0200, Markus Armbruster wrote:
> >> Monitor behavior changes even when the client rejects capability "oob".
> >> 
> >> Traditionally, the monitor reads, executes and responds to one command
> >> after the other.  If the client sends in-band commands faster than the
> >> server can execute them, the kernel will eventually refuse to buffer
> >> more, and sending blocks or fails with EAGAIN.
> >> 
> >> To make OOB possible, we need to read and queue commands as we receive
> >> them.  If the client sends in-band commands faster than the server can
> >> execute them, the server will eventually drop commands to limit the
> >> queue length.  The sever sends event COMMAND_DROPPED then.
> >> 
> >> However, we get the new behavior even when the client rejects capability
> >> "oob".  We get the traditional behavior only when the server doesn't
> >> offer "oob".
> >> 
> >> Is this what we want?
> >
> > Not really.  But I thought we kept that old behavior, haven't we?
> >
> > In handle_qmp_command() we have this:
> >
> >     /*
> >      * If OOB is not enabled on the current monitor, we'll emulate the
> >      * old behavior that we won't process the current monitor any more
> >      * until it has responded.  This helps make sure that as long as
> >      * OOB is not enabled, the server will never drop any command.
> >      */
> >     if (!qmp_oob_enabled(mon)) {
> >         monitor_suspend(mon);
> >         req_obj->need_resume = true;
> >     } else {
> >         /* Drop the request if queue is full. */
> >         if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> >             qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> >             qapi_event_send_command_dropped(id,
> >                                             COMMAND_DROP_REASON_QUEUE_FULL,
> >                                             &error_abort);
> >             qmp_request_free(req_obj);
> >             return;
> >         }
> >     }
> >
> > Here assuming that we are not enabling OOB, then since we will suspend
> > the monitor when we receive one command, then monitor_can_read() later
> > will check with a result that "we should not read the chardev", then
> > it'll leave all the data in the input buffer.  Then AFAIU the QMP
> > client that didn't enable OOB will have similar behavior as before.
> >
> > Also, we will never receive a COMMAND_DROPPED event in that legacy
> > mode as well since it's in the "else" block.   Am I right?
> 
> Hmm.  Did I get confused?  Let me look again.
> 
> Some places check qmp_oob_enabled(), which is true when the server
> offered capability "oob" and the client accepted.  In terms of my reply
> to Daniel, it distinguishes the two run time cases "OOB off" and "OOB
> on".

Exactly.

> 
> Other places check ->use_io_thr, which is true when
> 
>     (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr)
> 
> in monitor_init().
> 
> One of these places is get_qmp_greeting().  It makes the server offer
> "oob" when ->use_io_thr.  Makes sense.
> 
> In terms of my reply to Daniel, ->use_io_thr distinguishes between "OOB
> not offered" and ("OOB offered, but rejected by client" or "OOB offered
> and accepted").

Exactly.

To be more clear, let's name the three cases (as you mentioned):

(A) OOB not offered
(B) OOB offered, but rejected by client
(C) OOB offered, and accepted

Then:

(1) qmp_oob_enabled() will only be return true if (C).
(2) ->use_io_thr will only be true if (B) or (C).

> 
> Uses could to violate 'may use "server offered OOB" only for
> configuration purposes', so let's check them:
> 
> * monitor_json_emitter()
> 
>   If mon->use_io_thr, push to response queue.  Else emit directly.
> 
>   I'm afraid run time behavior differs for "OOB not offered" (emit
>   directly) and "OOB offered by rejected" (queue).

Yeah it's different, but logically it's the same.  IMHO it's only a
fast path for main thread.  In other word, I think it can still work
correctly if we remove that else clause.  In that sense, it's not
really a "true" behavior change.

> 
> * qmp_caps_check()
> 
>   If !mon->use_io_thr, reject client's acceptance of "oob".  Implicitly
>   recomputing the set of offered capabilities here is less than elegant,
>   but it's not wrong.
> 
> * monitor_resume()
> 
>   If mon->use_io_thr(), kick the monitor I/O thread.
> 
>   Again, different behavior for the two variations of "OOB off".

This is another example like above - IMHO we can just kick it no
matter what, but we just don't need to do that for main thread (since
we are in main thread so we are sure it is not asleep).  It's just
another trivial enhancement on top of the main logic.

> 
> * get_qmp_greeting()
> 
>   Covered above, looks good to me.
> 
> * monitor_qmp_setup_handlers_bh()
> 
>   If mon->use_io_thr(), pass the monitor I/O thread's AIOContext to
>   qemu_chr_fe_set_handlers(), else pass NULL.
> 
>   Again, different behavior for the two variations of "OOB off".

This is different indeed, but IMHO it's not really "behavior
difference", it's just the context (thread to run the code) that is
different.  The major code path for case (A) and (B) (which are the
two "off" cases) should be the same (when I say "major", I excluded
those trivial enhancements that I mentioned).

> 
> * monitor_init()
> 
>   If mon->use_io_thr, set up bottom half
>   monitor_qmp_setup_handlers_bh(), else call qemu_chr_fe_set_handlers()
>   directly.

This is indeed a bit tricky (to avoid a potential race that Stefan has
pointed out), however after things are setup it'll be exactly the same
code path for both OFF cases.

And actually when I read the code I noticed that we can actually
simplify the code with this:

diff --git a/monitor.c b/monitor.c
index 0730a27172..5d6bff8d51 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4635,18 +4635,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     Monitor *mon = opaque;
     GMainContext *context;

-    if (mon->use_io_thr) {
-        /*
-         * When use_io_thr is set, we use the global shared dedicated
-         * IO thread for this monitor to handle input/output.
-         */
-        context = monitor_get_io_context();
-        /* We should have inited globals before reaching here. */
-        assert(context);
-    } else {
-        /* The default main loop, which is the main thread */
-        context = NULL;
-    }
+    assert(mon->use_io_thr);
+    /*
+     * When use_io_thr is set, we use the global shared dedicated
+     * IO thread for this monitor to handle input/output.
+     */
+    context = monitor_get_io_context();
+    /* We should have inited globals before reaching here. */
+    assert(context);

     qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
                              monitor_qmp_event, NULL, mon, context, true);

Since monitor_qmp_setup_handlers_bh() will only be used for IOThread
case.  I can post a patch for that.

> 
>   Again, different behavior for the two variations of "OOB off".
> 
> Either I am confused now, or the two variations of "OOB off" execute
> different code at run time.  Which is it?

As mentioned, they should be running the same code at run time.
Though with some trivial differences, but if you see above most of
them should only be small enhancements which can actually be removed.

> 
> If it's different code, are the differences observable for the client?

AFAIU since the code path should mostly be the same for the two OOF
cases, IMHO there should have no difference observable from the
client, otherwise it's buggy and I'd be willing to fix it.

> 
> Observable or not, I suspect the differences should not exist.

We can remove those "trivial enhancements" if we want to make sure the
code paths are exactly the same, but I'm not sure whether that's
really what we need (or we just need to make sure it's unobservable).

Thanks!

-- 
Peter Xu