On Wed, Jun 06, 2018 at 11:46:07 +0800, zhuweilun wrote:
>
>
> 在 2018/6/5 15:10, Peter Krempa 写道:
> > On Tue, Jun 05, 2018 at 10:14:39 +0800, Shannon Zhao wrote:
> >> From: Weilun Zhu <zhuweilun@xxxxxxxxxx>
> >>
> >> As qemuMonitorJSONIOProcess() will unlock the qemu monitor, there is
> >> some extreme situation, eg qemu send message to monitor twice in a short
> >> time, where the local viriable 'msg' of qemuMonitorIOProcess() could be
> >> a wild point:
> >>
> >> 1. qemuMonitorSend() assign mon->msg to parameter 'msg', which is alse a
> >> local variable of its caller qemuMonitorJSONCommandWithFd(), cause
> >> eventloop to send message to monitor, then wait condition.
> >> 2. qemu send message to monitor for the first time immediately.
> >> 3. qemuMonitorIOProcess() is called, then wake up the qemuMonitorSend()
> >> thread, but the qemuMonitorSend() thread stuck for a while, which means
> >
> > If you wake up a tread via signalling a condition it _must_ have the
> > mutex locked prior to executing ....
> >
> > Note that after waking up from virCondWait you have the mutex which was
> > passed to it locked.
> >
>
> Yes, it _must_ have the mutex locked prior to executing, but there is still
> a chance that the qemuMonitorIO() hold the mutex faster.
>
> I mean virCondWait wants to wake up after qemuMonitorIOProcess() broadcast,
> but it stuck for a while as cpu pressure or scheduleed out or some other
> reasons. And in such short time, before virCondWait try to hold the mutex,
> qemu has sent message again, qemuMonitorIO() has been called again,
> and hold the mutex successfully. So virCondWait will still wait the mutex
> even qemuMonitorIOProcess() has broadcast.
>
> >> the qemu monitor is still unlocked.
> >> 4. qemu send message to monitor for the second time, such as RTC_CHANGE
> >> event
> >> 5. qemuMonitorIOProcess() is called, the local viriable 'msg' is
> >> assigned to mon->msg.
> >> 6. qemuMonitorIOProcess() call qemuMonitorJSONIOProcess() to deal with
> >> the message
> >> 7. qemuMonitorJSONIOProcess() unlock the qemu monitor, qemuMonitorSend()
> >> thread get the lock and free the mon->msg, assign mon->msg to NULL.
> >
> > The monitor is unlocked in qemuMonitorIO() after finishing processing
> > from the event loop. There is no point where qemuMonitorIOProcess()
> > would not hold the mutex locked.
> >
>
> qemuMonitorJSONIOProcess() is called by qemuMonitorIOProcess() to deal with
> the message, qemuMonitorJSONIOProcess()->qemuMonitorJSONIOProcessLine()->
> qemuMonitorJSONIOProcessEvent->qemuMonitorEmitEvent()->QEMU_MONITOR_CALLBACK
>
> #define QEMU_MONITOR_CALLBACK(mon, ret, callback, ...) \
> do { \
> virObjectRef(mon); \
> virObjectUnlock(mon); \
> if ((mon)->cb && (mon)->cb->callback) \
> (ret) = (mon)->cb->callback(mon, __VA_ARGS__, \
> (mon)->callbackOpaque); \
> virObjectLock(mon); \
> virObjectUnref(mon); \
> } while (0)
Ah, okay I did not notice that these unlock the monitor.
That means that the proposed solution is not correct though.
I think a proper solution is to process the events in the same way
normal messages are processed, which is after the monitor is unlocked,
but that is a rather complex fix.
Other possibility is to re-acquire the 'msg' object after the call to
qemuMonitorJSONIOProcess returns. This requires adding a comment why is
it necessary
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
