On 06/29/2015 11:17 AM, Pavel Hrdina wrote:
> There are multiple consumers for the domain condition and we should
> always wake them all.
>
> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 7 -------
> src/conf/domain_conf.h | 1 -
> src/libvirt_private.syms | 1 -
> src/qemu/qemu_process.c | 4 ++--
> 4 files changed, 2 insertions(+), 11 deletions(-)
>
Hmmm... This is perhaps quite a difference as you're now unblocking all
threads for HandleBlockJob and HandleSpiceMigrated.
Using virCondBroadcast means all threads waiting on the condition will
be unblocked as opposed to unblocking 'at least one of the threads' when
using virCondSignal - for some reason that just doesn't feel right.
I can certainly understand unblocking everything waiting for
HandleTrayChange since it's a single physical block and perhaps
HandleSpiceMigrate since that seems to be a single SPICE server
transferring things (such as perhaps ports). Whereas, it just feels like
there could be multiple causes for BlockJob wanting synchronized access.
I guess I'd like to read others input on this.
The other patches seem mostly reasonable - it's just this one that's
causing me to think longer and question things.
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 988818c..9efe175 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2661,13 +2661,6 @@ virDomainObjEndAPI(virDomainObjPtr *vm)
>
>
> void
> -virDomainObjSignal(virDomainObjPtr vm)
> -{
> - virCondSignal(&vm->cond);
> -}
> -
> -
> -void
> virDomainObjBroadcast(virDomainObjPtr vm)
> {
> virCondBroadcast(&vm->cond);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index aeba5a5..59655c1 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2442,7 +2442,6 @@ void virDomainObjEndAPI(virDomainObjPtr *vm);
> bool virDomainObjTaint(virDomainObjPtr obj,
> virDomainTaintFlags taint);
>
> -void virDomainObjSignal(virDomainObjPtr vm);
> void virDomainObjBroadcast(virDomainObjPtr vm);
> int virDomainObjWait(virDomainObjPtr vm);
> int virDomainObjWaitUntil(virDomainObjPtr vm,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1566d11..720afdf 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -412,7 +412,6 @@ virDomainObjParseNode;
> virDomainObjSetDefTransient;
> virDomainObjSetMetadata;
> virDomainObjSetState;
> -virDomainObjSignal;
> virDomainObjTaint;
> virDomainObjUpdateModificationImpact;
> virDomainObjWait;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ba84182..a688feb 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1004,7 +1004,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> /* We have a SYNC API waiting for this event, dispatch it back */
> diskPriv->blockJobType = type;
> diskPriv->blockJobStatus = status;
> - virDomainObjSignal(vm);
> + virDomainObjBroadcast(vm);
> } else {
> /* there is no waiting SYNC API, dispatch the update to a thread */
> if (VIR_ALLOC(processEvent) < 0)
> @@ -1500,7 +1500,7 @@ qemuProcessHandleSpiceMigrated(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> }
>
> priv->job.spiceMigrated = true;
> - virDomainObjSignal(vm);
> + virDomainObjBroadcast(vm);
>
> cleanup:
> virObjectUnlock(vm);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list