Re: [libvirt] [PATCH] qemu: allow blkstat/blkinfo calls during migration

[Federico Simoncelli <fsimonce redhat com>]

----- Original Message -----
> From: "Eric Blake" <eblake redhat com>
> To: "Federico Simoncelli" <fsimonce redhat com>
> Cc: libvir-list redhat com
> Sent: Friday, May 13, 2011 1:38:45 AM
> Subject: Re: [PATCH] qemu: allow blkstat/blkinfo calls during migration
> On 05/11/2011 07:26 AM, Federico Simoncelli wrote:
> >
> > + if ((priv->jobActive == QEMU_JOB_MIGRATION_OUT)
> > + || (priv->jobActive == QEMU_JOB_SAVE)) {
> > + virDomainObjRef(vm);
> > + while (priv->jobSignals & QEMU_JOB_SIGNAL_BLKSTAT)
> > + ignore_value(virCondWait(&priv->signalCond, &vm->lock));
> 
> Hmm, should we mark priv->jobSignals as volatile in the header, to
> ensure the compiler won't optimize this into an infinite loop?

Not sure.
http://software.intel.com/en-us/blogs/2007/11/30/volatile-almost-useless-for-multi-threaded-programming/

> > + if (priv->jobSignals & QEMU_JOB_SIGNAL_BLKSTAT) {
> > + qemuDomainObjEnterMonitorWithDriver(driver, vm);
> > + rc = qemuMonitorGetBlockStatsInfo(priv->mon,
> 
> I'm still wondering if we need to hold the signalLock condition during
> the duration where we drop driver lock to call out to the monitor. I
> can't convince myself that we need to, but I also can't convince
> myself
> that your code is safe without it (I guess it goes to show that I
> haven't done much programming on condition variables in any prior job
> -
> they're cool tools, but hard to wrap your head around when first
> learning them).

There is no signalLock, on the other hand the signalCond is used to wake
up a thread (using virCondSignal/virCondBroadcast) that is currently
sleeping on virCondWait (and not holding vm->lock).

-- 
Federico