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

[Marc-André Lureau]

Hi

On Thu, Jul 5, 2018 at 1:09 PM, Markus Armbruster <address@hidden> wrote:
> Peter Xu <address@hidden> writes:
>
>> On Thu, Jul 05, 2018 at 10:51:33AM +0200, Markus Armbruster wrote:
>>> Peter Xu <address@hidden> 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 <address@hidden>
>>>
>>> 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.


-- 
Marc-André Lureau