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 <arm...@redhat.com> wrote: > > Peter Xu <pet...@redhat.com> writes: > > > >> On Thu, Jul 05, 2018 at 10:51:33AM +0200, Markus Armbruster wrote: > >>> Peter Xu <pet...@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 <pet...@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
