Re: [Qemu-devel] [PATCH 4/9] monitor: move need_resume flag into monitor struct

[Peter Xu <peterx@redhat.com>]

On Thu, Jul 05, 2018 at 01:32:46PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jul 5, 2018 at 1:09 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > Peter Xu <peterx@redhat.com> writes:
> >
> >> On Thu, Jul 05, 2018 at 10:51:33AM +0200, Markus Armbruster wrote:
> >>> Peter Xu <peterx@redhat.com> writes:
> >>>
> >>> > It was put into the request object to show whether we'll need to resume
> >>> > the monitor after dispatching the command.  Now we move that into the
> >>> > monitor struct so that it might be even used in other places in the
> >>> > future, e.g., out-of-band message flow controls.
> >>> >
> >>> > One thing to mention is that there is no lock needed before when
> >>> > accessing the flag since the request object will always be owned by a
> >>> > single thread.  After we move it into monitor struct we need to protect
> >>> > that flag since it might be accessed by multiple threads now.  Renaming
> >>> > the qmp_queue_lock into qmp_lock to protect the flag as well.
> >>> >
> >>> > No functional change.
> >>> >
> >>> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >>>
> >>> Marc-André's "[PATCH v3 04/38] monitor: no need to save need_resume" and
> >>> "[PATCH v3 05/38] monitor: further simplify previous patch" also mess
> >>> with need_resume.  Marc-André, could you have a look at this patch and
> >>> the next one?
> >>
> >> Sorry I should have looked at those before hand.  I think I must be
> >> waiting for another post to split the patches into two (after
> >> Marc-Andre poked me with that thread) but then I forgot about that.
> >>
> >> So now I suspect we'd better keep that flag since in the next patch
> >> the suspend operation can happen conditionally now.
> >
> > Could you two together figure out how to combine your work?  Would take
> > me off this critical path...
> >
> 
> With Peter patches, the monitor is also suspended when the queue of
> command is too long (when oob-enabled). Thus the variable now covers
> one of two different cases.
> 
> My patches only covered the first case simplification (legacy /
> !oob-enabled suspend).
> 
> The second case could probably use a similar simplification, looking
> at the queue length, but at this point I think I'd prefer to keep the
> variable for clarity and sanity state checking.

Thanks for confirming that.

Meanwhile, I'm not 100% sure whether the trick can be played again
here (e.g., resume when queue length is N-1 for either of the queue).
If without the flag, there should be a very tricky condition (when
both queues are with length N-1) that we might resume the monitor
twice while we should only do that once.

Regards,

-- 
Peter Xu