Re: [libvirt PATCH v6 30/30] nodedev: avoid delay when defining a new mdev
On Fri, Mar 26, 2021 at 11:48:26AM -0500, Jonathon Jongsma wrote:
> 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();
> +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();
> +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();
> +virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> +
> +return 0;
In 29/30, you extracted ^this code into a separate function. If you'd moved it
to the right place as well, the diff in this patch would be smaller.
> +}
> +
> virNodeDevice*
> -nodeDeviceDefineXML(virConnect *conn,
> +nodeDeviceDefineXML(virConnectPtr conn,
I don't think you wanted ^this hunk
> 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,
[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();
+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();
+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();
+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
