From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: [PATCH v4 for 4.5 2/8] libxl: synchronise configuration when we hotplug a device Date: Tue, 16 Sep 2014 11:01:12 +0100 Message-ID: <1410861678-31428-3-git-send-email-wei.liu2@citrix.com> References: <1410861678-31428-1-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1410861678-31428-1-git-send-email-wei.liu2@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xen.org Cc: Wei Liu , ian.jackson@eu.citrix.com, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org We update JSON version first, then write to xenstore, so that we maintain the following invariant: any device which is present in xenstore has a corresponding entry in JSON. The workflow is as followed: lock json config read json config update in-memory json config with new entry, replacing any stale entry for loop open xs transaction check device existence, abort if it exists write in-memory json config to disk commit xs transaction end for loop unlock json config Please see comment in libxl_internal.h for correctness proof. As those routines are called both during domain creation and device hotplug, we add a flag to indicate whether we need to update JSON config. This flag is only set to true when we hotplug a device. We cannot update JSON config during domain creation as JSON config is committed to disk only when domain creation finishes. Signed-off-by: Wei Liu --- change in v4: 1. correct typos and reword comment 2. libxl__carefd -> libxl__domain_userdata_lock --- tools/libxl/libxl.c | 96 +++++++++++++++++++++++++++++++ tools/libxl/libxl_internal.h | 130 ++++++++++++++++++++++++++++++++++++++++++ tools/libxl/libxl_pci.c | 24 ++++++++ 3 files changed, 250 insertions(+) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 3b2d104..e5d4891 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -1894,6 +1894,13 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid, libxl__device *device; int rc; xs_transaction_t t = XBT_NULL; + libxl_domain_config d_config; + libxl_device_vtpm vtpm_saved; + libxl__domain_userdata_lock *lock = NULL; + + libxl_domain_config_init(&d_config); + libxl_device_vtpm_init(&vtpm_saved); + libxl_device_vtpm_copy(CTX, &vtpm_saved, vtpm); rc = libxl__device_vtpm_setdefault(gc, vtpm); if (rc) goto out; @@ -1908,6 +1915,8 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid, } } + libxl__update_config_vtpm(gc, &vtpm_saved, vtpm); + GCNEW(device); rc = libxl__device_from_vtpm(gc, domid, vtpm, device); if ( rc != 0 ) goto out; @@ -1933,6 +1942,19 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid, flexarray_append(front, "handle"); flexarray_append(front, GCSPRINTF("%d", vtpm->devid)); + if (aodev->update_json) { + lock = libxl__lock_domain_userdata(gc, domid); + if (!lock) { + rc = ERROR_LOCK_FAIL; + goto out; + } + + rc = libxl__get_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + + DEVICE_ADD(vtpm, vtpms, domid, &vtpm_saved, COMPARE_DEVID, &d_config); + } + for (;;) { rc = libxl__xs_transaction_start(gc, &t); if (rc) goto out; @@ -1946,6 +1968,11 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid, goto out; } + if (aodev->update_json) { + rc = libxl__set_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + } + libxl__device_generic_add(gc, t, device, libxl__xs_kvs_of_flexarray(gc, back, back->count), @@ -1965,6 +1992,9 @@ void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid, rc = 0; out: libxl__xs_transaction_abort(gc, &t); + if (lock) libxl__unlock_domain_userdata(lock); + libxl_device_vtpm_dispose(&vtpm_saved); + libxl_domain_config_dispose(&d_config); aodev->rc = rc; if(rc) aodev->callback(egc, aodev); return; @@ -2197,6 +2227,13 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid, int rc; libxl_ctx *ctx = gc->owner; xs_transaction_t t = XBT_NULL; + libxl_domain_config d_config; + libxl_device_disk disk_saved; + libxl__domain_userdata_lock *lock = NULL; + + libxl_domain_config_init(&d_config); + libxl_device_disk_init(&disk_saved); + libxl_device_disk_copy(ctx, &disk_saved, disk); libxl_domain_type type = libxl__domain_type(gc, domid); if (type == LIBXL_DOMAIN_TYPE_INVALID) { @@ -2204,6 +2241,26 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid, goto out; } + /* + * get_vdev != NULL -> local attach + * get_vdev == NULL -> block attach + * + * We don't care about local attach state because it's only + * intermediate state. + */ + if (!get_vdev && aodev->update_json) { + lock = libxl__lock_domain_userdata(gc, domid); + if (!lock) { + rc = ERROR_LOCK_FAIL; + goto out; + } + + rc = libxl__get_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + + DEVICE_ADD(disk, disks, domid, &disk_saved, COMPARE_DISK, &d_config); + } + for (;;) { rc = libxl__xs_transaction_start(gc, &t); if (rc) goto out; @@ -2356,6 +2413,11 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid, } } + if (!get_vdev && aodev->update_json) { + rc = libxl__set_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + } + libxl__device_generic_add(gc, t, device, libxl__xs_kvs_of_flexarray(gc, back, back->count), libxl__xs_kvs_of_flexarray(gc, front, front->count), @@ -2374,6 +2436,9 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid, out: libxl__xs_transaction_abort(gc, &t); + if (lock) libxl__unlock_domain_userdata(lock); + libxl_device_disk_dispose(&disk_saved); + libxl_domain_config_dispose(&d_config); aodev->rc = rc; if (rc) aodev->callback(egc, aodev); return; @@ -3030,6 +3095,13 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, libxl__device *device; int rc; xs_transaction_t t = XBT_NULL; + libxl_domain_config d_config; + libxl_device_nic nic_saved; + libxl__domain_userdata_lock *lock = NULL; + + libxl_domain_config_init(&d_config); + libxl_device_nic_init(&nic_saved); + libxl_device_nic_copy(CTX, &nic_saved, nic); rc = libxl__device_nic_setdefault(gc, nic, domid); if (rc) goto out; @@ -3044,6 +3116,8 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, } } + libxl__update_config_nic(gc, &nic_saved, nic); + GCNEW(device); rc = libxl__device_from_nic(gc, domid, nic, device); if ( rc != 0 ) goto out; @@ -3101,6 +3175,19 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, flexarray_append(front, libxl__sprintf(gc, LIBXL_MAC_FMT, LIBXL_MAC_BYTES(nic->mac))); + if (aodev->update_json) { + lock = libxl__lock_domain_userdata(gc, domid); + if (!lock) { + rc = ERROR_LOCK_FAIL; + goto out; + } + + rc = libxl__get_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + + DEVICE_ADD(nic, nics, domid, &nic_saved, COMPARE_DEVID, &d_config); + } + for (;;) { rc = libxl__xs_transaction_start(gc, &t); if (rc) goto out; @@ -3114,6 +3201,11 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, goto out; } + if (aodev->update_json) { + rc = libxl__set_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + } + libxl__device_generic_add(gc, t, device, libxl__xs_kvs_of_flexarray(gc, back, back->count), @@ -3133,6 +3225,9 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, rc = 0; out: libxl__xs_transaction_abort(gc, &t); + if (lock) libxl__unlock_domain_userdata(lock); + libxl_device_nic_dispose(&nic_saved); + libxl_domain_config_dispose(&d_config); aodev->rc = rc; if (rc) aodev->callback(egc, aodev); return; @@ -3686,6 +3781,7 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1) GCNEW(aodev); \ libxl__prepare_ao_device(ao, aodev); \ aodev->callback = device_addrm_aocomplete; \ + aodev->update_json = true; \ libxl__device_##type##_add(egc, domid, type, aodev); \ \ return AO_INPROGRESS; \ diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index aa18e74..f61673c 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -2148,6 +2148,8 @@ struct libxl__ao_device { int num_exec; /* for calling hotplug scripts */ libxl__async_exec_state aes; + /* If we need to update JSON config */ + bool update_json; }; /* @@ -2273,6 +2275,77 @@ struct libxl__multidev { * * Once finished, aodev->callback will be executed. */ +/* + * As of Xen 4.5 we maintain various infomation, including hotplug + * device information, in JSON files, so that we can use this JSON + * file as a template to reconstruct domain configuration. + * + * In essense there are now two views of device state, one is xenstore, + * the other is JSON file. We use xenstore as primary reference. + * + * Here we maintain one invariant: every device in xenstore must have + * an entry in JSON file. + * + * All device hotplug routines should comply to following pattern: + * lock json config (json_lock) + * read json config + * update in-memory json config with new entry, replacing + * any stale entry + * for loop -- xs transaction + * open xs transaction + * check device existence, abort if it exists + * write in-memory json config to disk + * commit xs transaction + * end for loop + * unlock json config + * + * Device removal routines are not touched. + * + * Here is the proof that we always maintain that invariant and we + * don't leak files during interaction of hotplug thread and other + * threads / processes. + * + * # Safe against parallel add + * + * When another thread / process tries to add same device, it's + * blocked by json_lock. The loser of two threads will bail at + * existence check, so that we don't overwrite anything. + * + * # Safe against domain destruction + * + * If the thread / process trying to destroy domain loses the race, it's + * blocked by json_lock. If the hotplug thread is loser, it bails at + * acquiring lock because lock acquisition function checks existence of + * the domain. + * + * # Safe against parallel removal + * + * When another thread / process tries to remove a device, it's _NOT_ + * blocked by json_lock, but xenstore transaction can help maintain + * invariant. The removal threads either a) sees that device in + * xenstore, b) doesn't see that device in xenstore. + * + * In a), it sees that device in xenstore. At that point hotplug is + * already finished (both JSON and xenstore changes committed). So that + * device can be safely removed. JSON entry is left untouched and + * becomes stale, but this is a valid state -- next time when a + * device with same identifier gets added, the stale entry gets + * overwritten. + * + * In b), it doesn't see that device in xenstore, but it will commence + * anyway. Eventually a forcibly removal is initiated, which will forcely + * remove xenstore entry. + * + * If hotplug threads creates xenstore entry (therefore JSON entry as + * well) before force removal, that xenstore entry is removed. We're + * left with JSON stale entry but not xenstore entry, which is a valid + * state. + * + * If hotplug thread has not created xenstore entry when the removal + * is committed, we're obviously safe. Hotplug thread will add in + * xenstore entry afterwards. We have both JSON and xenstore entry, + * it's a valid state. + */ _hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid, libxl_device_disk *disk, libxl__ao_device *aodev); @@ -3280,6 +3353,63 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc, libxl_uuid_copy(CTX, &dst->uuid, &src->uuid); } +/* Macros used to compare device identifier. Returns true if the two + * devices have same identifier. */ +#define COMPARE_DEVID(a, b) ((a)->devid == (b)->devid) +#define COMPARE_DISK(a, b) (!strcmp((a)->vdev, (b)->vdev)) +#define COMPARE_PCI(a, b) ((a)->func == (b)->func && \ + (a)->bus == (b)->bus && \ + (a)->dev == (b)->dev) + +/* DEVICE_ADD + * + * Add a device in libxl_domain_config structure + * + * It takes 6 parameters: + * type: the type of the device, say nic, vtpm, disk, pci etc + * ptr: pointer to the start of the array, the array must be + * of type libxl_device_#type + * domid: domain id of target domain + * dev: the device that is to be added / removed / updated + * compare: the COMPARE_* macro used to compare @dev's identifier to + * those in the array pointed to by @ptr + * d_config: pointer to template domain config + * + * For most device types (nic, vtpm), the array pointer @ptr can be + * derived from @type, pci device being the exception, hence we need + * to have @ptr. + * + * If there is already a device with the same identifier in d_config, + * that entry is updated. + */ +#define DEVICE_ADD(type, ptr, domid, dev, compare, d_config) \ + ({ \ + int DA_x; \ + libxl_device_##type *DA_p = NULL; \ + \ + /* Check for existing device */ \ + for (DA_x = 0; DA_x < (d_config)->num_##ptr; DA_x++) { \ + if (compare(&(d_config)->ptr[DA_x], (dev))) { \ + DA_p = &(d_config)->ptr[DA_x]; \ + break; \ + } \ + } \ + \ + if (!DA_p) { \ + (d_config)->ptr = \ + libxl__realloc(NOGC, (d_config)->ptr, \ + ((d_config)->num_##ptr + 1) * \ + sizeof(libxl_device_##type)); \ + DA_p = &(d_config)->ptr[(d_config)->num_##ptr]; \ + (d_config)->num_##ptr++; \ + } else { \ + libxl_device_##type##_dispose(DA_p); \ + } \ + \ + libxl_device_##type##_init(DA_p); \ + libxl_device_##type##_copy(CTX, DA_p, (dev)); \ + }) + #endif /* diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index c22767e..9f40100 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -126,6 +126,13 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d xs_transaction_t t; libxl__device *device; int rc; + libxl_domain_config d_config; + libxl_device_pci pcidev_saved; + libxl__domain_userdata_lock *lock = NULL; + + libxl_domain_config_init(&d_config); + libxl_device_pci_init(&pcidev_saved); + libxl_device_pci_copy(CTX, &pcidev_saved, pcidev); be_path = libxl__sprintf(gc, "%s/backend/pci/%d/0", libxl__xs_get_dompath(gc, 0), domid); num_devs = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/num_devs", be_path)); @@ -153,6 +160,17 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d GCNEW(device); libxl__device_from_pcidev(gc, domid, pcidev, device); + lock = libxl__lock_domain_userdata(gc, domid); + if (!lock) { + rc = ERROR_LOCK_FAIL; + goto out; + } + + rc = libxl__get_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + + DEVICE_ADD(pci, pcidevs, domid, &pcidev_saved, COMPARE_PCI, &d_config); + for (;;) { rc = libxl__xs_transaction_start(gc, &t); if (rc) goto out; @@ -165,6 +183,9 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d goto out; } + rc = libxl__set_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back, back->count)); @@ -175,6 +196,9 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d out: libxl__xs_transaction_abort(gc, &t); + if (lock) libxl__unlock_domain_userdata(lock); + libxl_device_pci_dispose(&pcidev_saved); + libxl_domain_config_dispose(&d_config); return rc; } -- 1.7.10.4