Re: [Xen-devel] [PATCH 4/9] libxl_internal: Create new lock for devices hotplug via QMP

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH 4/9] libxl_internal: Create new lock for devices 
hotplug via QMP"):
> The current lock `domain_userdata_lock' can't be used when modification
> to a guest is done by sending command to QEMU, this is a slow process
> and requires to call CTX_UNLOCK, which is not possible while holding
> the `domain_userdata_lock'.
...
> +struct libxl__domain_qmp_lock {
> +    libxl__generic_lock lock;
> +};
> +
> +libxl__domain_qmp_lock *libxl__lock_domain_qmp(libxl__gc *gc,
> +                                               libxl_domid domid)
> +{
> +    libxl__domain_qmp_lock *lock = NULL;
> +    int rc;
> +
> +    lock = libxl__zalloc(NOGC, sizeof(*lock));
> +    rc = libxl__lock_generic(gc, domid, &lock->lock,
> +                             "libxl-device-changes-lock");
> +    if (rc) {
> +        libxl__unlock_domain_qmp(lock);
> +        return NULL;
> +    }
> +
> +    return lock;

This is almost identical to the libxl__lock_domain_userdata which
appeared in the previous patch.  That suggests that the factorisation
here is at the wrong layer.

> +void libxl__unlock_domain_qmp(libxl__domain_qmp_lock *lock)
> +{
> +    libxl__unlock_generic(&lock->lock);
> +    free(lock);
> +}

This is completely identical to libxl__unlock_domain_userdata.  The
only reason we have to two of these is so that the two locks are
distinguished by the type of the lock struct.  That means that you
have to call
   libxl__unlock_domain_qmp(foo->qmp_lock)
but
   libxl__unlock_domain_userdate(foo->userdata_lock)
or some such, and the compiler will spot if you write
   libxl__unlock_domain_userdata(foo->qmp_lock)
or something.  But is it really useful to have to write the `qmp' vs
`userdata' thing twice ?

> + * It is to be acquired by an ao event callback.

I think there is a worse problem here, though.  This lock is probably
*inside* some lock from the caller.  So usual libxl rules apply and
you may not block to acquaire it.

Ie libxl__lock_domain_qmp must be one of these things that takes a
little ev state struct and makes a callback.

At the syscall level, acquiring an fcntl lock cannot be selected on.
The options are to poll, or to spawn a thread, or to fork.

Currently libxl does not call pthread_create, although maybe it could
do.  To safely create a thread you have to do a dance with
sigprocmask, to avoid signal delivery onto your thread.

Maybe it would be better to try once with LOCK_NB, and to fork if this
is not successful.  But it would be simpler to always fork...

> + * Or in case QEMU is the primary config, this pattern can be use:
> + *   lock qmp (qmp_lock)
> + *      lock json config (json_lock)
> + *          read json config
> + *          update in-memory json config with new entry, replacing
> + *             any stale entry
> + *      unlock json config
> + *      apply new config to primary config
> + *      lock json config (json_lock)
> + *          read json config
> + *          update in-memory json config with new entry, replacing
> + *             any stale entry
> + *          write in-memory json config to disk
> + *      unlock json config
> + *   unlock qmp

This doesn't show the ctx locks but I think that is fine.  So I think
this description is correct.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel