Re: [libvirt PATCH v6 30/30] nodedev: avoid delay when defining a new mdev

When calling virNodeDeviceDefineXML() to define a new mediated device, we call virMdevctlDefine() and then wait for the new device to appear in the driver's device list before returning. This caused long delays due to the behavior of nodeDeviceFindNewMediatedDevice(). This function checks to see if the device is in the list and then waits for 5s before checking again. Because mdevctl is relatively slow to query the list of defined devices[0], the newly-defined device was generally not in the device list when we first checked. This results in libvirt almost always taking at least 5s to complete this API call for mediated devices, which is unacceptable. In order to avoid this long delay, we resort to a workaround. If the call to virMdevctlDefine() was successful, we can assume that this new device will exist the next time we query mdevctl for new devices. So we simply add this provisional device definition directly to the nodedev driver's device list and return from the function. At some point in the future, the mdevctl handler will run and the "official" device will be processed, which will update the provisional device if any new details need to be added. The reason that this is not necessary for virNodeDeviceCreateXML() is because detecting newly-created (not defined) mdevs happens through udev instead of mdevctl. And nodeDeviceFindNewMediatedDevice() always calls 'udevadm settle' before checking to see whether the device is in the list. This allows us to wait just long enough for all udev events to be processed, so the device is almost always in the list the first time we check and so we almost never end up hitting the 5s sleep. [0] on my machine, 'mdevctl list --defined' took around 0.8s to complete for only 3 defined mdevs. --- src/node_device/node_device_driver.c | 126 +++++++++++++++------------ 1 file changed, 68 insertions(+), 58 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 35db24817a..a1b79d93f7 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1215,16 +1215,70 @@ nodeDeviceDestroy(virNodeDevicePtr device) return ret; } + +/* takes ownership of @def and potentially frees it. @def should not be used + * after returning from this function */ +static int +nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def) +{ + virNodeDeviceObj *obj; + virObjectEvent *event; + bool defined = false; + g_autoptr(virNodeDeviceDef) owned = def; + g_autofree char *name = g_strdup(owned->name); + + owned->driver = g_strdup("vfio_mdev"); + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, owned->name))) { + virNodeDeviceDef *d = g_steal_pointer(&owned); + if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, d))) { + virNodeDeviceDefFree(d); + return -1; + } + } else { + bool changed; + virNodeDeviceDef *olddef = virNodeDeviceObjGetDef(obj); + + defined = virNodeDeviceObjIsPersistent(obj); + /* Active devices contain some additional information (e.g. sysfs + * path) that is not provided by mdevctl, so re-use the existing + * definition and copy over new mdev data */ + changed = nodeDeviceDefCopyFromMdevctl(olddef, owned); + + if (defined && !changed) { + /* if this device was already defined and the definition + * hasn't changed, there's nothing to do for this device */ + virNodeDeviceObjEndAPI(&obj); + return 0; + } + } + + /* all devices returned by virMdevctlListDefined() are persistent */ + virNodeDeviceObjSetPersistent(obj, true); + + if (!defined) + event = virNodeDeviceEventLifecycleNew(name, + VIR_NODE_DEVICE_EVENT_DEFINED, + 0); + else + event = virNodeDeviceEventUpdateNew(name); + + virNodeDeviceObjEndAPI(&obj); + virObjectEventStateQueue(driver->nodeDeviceEventState, event); + + return 0;

+} + virNodeDevice* -nodeDeviceDefineXML(virConnect *conn, +nodeDeviceDefineXML(virConnectPtr conn,

const char *xmlDesc, unsigned int flags) { g_autoptr(virNodeDeviceDef) def = NULL; - virNodeDevice *device = NULL; const char *virt_type = NULL; g_autofree char *uuid = NULL; g_autofree char *errmsg = NULL; + g_autofree char *name = NULL; virCheckFlags(0, NULL); @@ -1264,9 +1318,19 @@ nodeDeviceDefineXML(virConnect *conn, } mdevGenerateDeviceName(def); - device = nodeDeviceFindNewMediatedDevice(conn, def->caps->data.mdev.uuid); + name = g_strdup(def->name); + + /* Normally we would call nodeDeviceFindNewMediatedDevice() here to wait + * for the new device to appear. But mdevctl can take a while to query + * devices, and if nodeDeviceFindNewMediatedDevice() doesn't find the new + * device immediately it will wait at 5s before checking again. Since we + * have already received the uuid from virMdevctlDefine(), we can simply + * add the provisional device to the list and return it immediately and + * avoid this long delay. */ + if (nodeDeviceUpdateMediatedDevice(g_steal_pointer(&def)) < 0) + return NULL;