On 6/30/23 6:34 AM, Boris Fiuczynski wrote: So, this should work and roughly matches what we've done for other similar cases. But this is running from within the udev event handler thread, and theoretically there could already be an mdevctl thread running concurrently and updating the internal device list. The device list update functions are thread-safe so I don't think it'll corrupt anything, but it seems better to do all mdevctl updates from mdevctl update thread rather than calling it directly here. The same actually applies to a couple other existing calls to nodeDeviceUpdateMediatedDevices(). I'll send a couple patches that apply on top of yours and refactor things a bit. Let me know what you think. Jonathon

Factor out a new scheduleMdevctlUpdate() function so that we can re-use it from other places. Now that other events can make it necessary to re-query mdevctl for mdev updates, this function will be useful for coalescing multiple updates in quick succession into a single mdevctl query. Also rename a couple functions. The names weren't very descriptive of their behavior. For example, the old scheduleMdevctlHandler() function didn't actually schedule anything, it just started a thread. So rename it to free up the 'schedule' name for the above refactored function. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_udev.c | 37 ++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index fce4212728..9ba550dbc1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2075,7 +2075,7 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) static void -mdevctlHandlerThread(void *opaque G_GNUC_UNUSED) +mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) { udevEventData *priv = driver->privateData; VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock); @@ -2085,8 +2085,9 @@ mdevctlHandlerThread(void *opaque G_GNUC_UNUSED) } + static void -scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque) +launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) { udevEventData *priv = opaque; virThread thread; @@ -2096,7 +2097,7 @@ scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque) priv->mdevctlTimeout = -1; } - if (virThreadCreateFull(&thread, false, mdevctlHandlerThread, + if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, "mdevctl-thread", false, NULL) < 0) { virReportSystemError(errno, "%s", _("failed to create mdevctl thread")); @@ -2192,6 +2193,26 @@ mdevctlEnableMonitor(udevEventData *priv) } +/* Schedules an mdevctl update for 100ms in the future, canceling any existing + * timeout that may have been set. In this way, multiple update requests in + * quick succession can be collapsed into a single update. if @force is true, + * an update thread will be spawned immediately. */ +static void +scheduleMdevctlUpdate(udevEventData *data, + bool force) +{ + if (!force) { + if (data->mdevctlTimeout > 0) + virEventRemoveTimeout(data->mdevctlTimeout); + data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread, + data, NULL); + return; + } + + launchMdevctlUpdateThread(-1, data); +} + + static void mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, GFile *file, @@ -2222,15 +2243,7 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, * configuration change, try to coalesce these changes by waiting for the * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the * signal if that event never comes */ - if (event_type != G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) { - if (priv->mdevctlTimeout > 0) - virEventRemoveTimeout(priv->mdevctlTimeout); - priv->mdevctlTimeout = virEventAddTimeout(100, scheduleMdevctlHandler, - priv, NULL); - return; - } - - scheduleMdevctlHandler(-1, priv); + scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); } -- 2.41.0

Rather than directly executing mdevctl from the udev event thread when we determine that we need to re-query, schedule the mdevctl thread to run. This also helps to coalesce multiple back-to-back updates into a single one when there are multiple updates in a row or at startup when a host has a very large number of mdevs. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; --- src/node_device/node_device_udev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9ba550dbc1..da63d326a1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1443,6 +1443,9 @@ udevGetDeviceDetails(struct udev_device *device, } +static void scheduleMdevctlUpdate(udevEventData *data, bool force); + + static int udevRemoveOneDeviceSysPath(const char *path) { @@ -1475,8 +1478,7 @@ udevRemoveOneDeviceSysPath(const char *path) virNodeDeviceObjEndAPI(&obj); /* cannot check for mdev_types since they have already been removed */ - if (nodeDeviceUpdateMediatedDevices() < 0) - VIR_WARN("mdevctl failed to update mediated devices"); + scheduleMdevctlUpdate(driver->privateData, false); virObjectEventStateQueue(driver->nodeDeviceEventState, event); return 0; @@ -1604,8 +1606,8 @@ udevAddOneDevice(struct udev_device *device) has_mdev_types = virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_MDEV_TYPES); virNodeDeviceObjEndAPI(&obj); - if (has_mdev_types && nodeDeviceUpdateMediatedDevices() < 0) - VIR_WARN("mdevctl failed to update mediated devices"); + if (has_mdev_types) + scheduleMdevctlUpdate(driver->privateData, false); ret = 0; @@ -1767,8 +1769,7 @@ udevHandleOneDevice(struct udev_device *device) if (ret == 0 && udevGetDeviceType(device, &dev_cap_type) == 0 && dev_cap_type == VIR_NODE_DEV_CAP_MDEV) - if (nodeDeviceUpdateMediatedDevices() < 0) - VIR_WARN("mdevctl failed to update mediated devices"); + scheduleMdevctlUpdate(driver->privateData, false); return ret; } -- 2.41.0

On 7/6/23 9:40 PM, Jonathon Jongsma wrote: LGTM and testing was successful. See my direct replies on the patches. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294