[PATCH v2] nodedev: transient mdev update on nodeDeviceCreateXML
Update the optional mdev attributes by running an mdevctl update on a new created nodedev object representing an mdev. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158 Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_udev.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4c37ec3189..fce4212728 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1757,12 +1757,20 @@ nodeStateCleanup(void) static int udevHandleOneDevice(struct udev_device *device) { + virNodeDevCapType dev_cap_type; const char *action = udev_device_get_action(device); VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device)); - if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(device); + if (STREQ(action, "add") || STREQ(action, "change")) { + int ret = udevAddOneDevice(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"); + return ret; + } if (STREQ(action, "remove")) return udevRemoveOneDevice(device); -- 2.41.0
On 6/30/23 6:34 AM, Boris Fiuczynski wrote:
Update the optional mdev attributes by running an mdevctl update on a new created nodedev object representing an mdev.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158 Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_udev.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4c37ec3189..fce4212728 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1757,12 +1757,20 @@ nodeStateCleanup(void) static int udevHandleOneDevice(struct udev_device *device) { + virNodeDevCapType dev_cap_type; const char *action = udev_device_get_action(device);
VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));
- if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(device); + if (STREQ(action, "add") || STREQ(action, "change")) { + int ret = udevAddOneDevice(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"); + return ret; + }
if (STREQ(action, "remove")) return udevRemoveOneDevice(device);
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
move mdevctl updates to the mdevctl thread. These two patches apply on top of Boris's patch: "[PATCH v2] nodedev: transient mdev update on nodeDeviceCreateXML" Jonathon Jongsma (2): nodedev: refactor mdevctl thread functions nodedev: update mdevs from the mdevctl thread src/node_device/node_device_udev.c | 50 +++++++++++++++++++----------- 1 file changed, 32 insertions(+), 18 deletions(-) -- 2.41.0
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@redhat.com> --- 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
On 7/6/23 9:44 PM, Jonathon Jongsma wrote:
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@redhat.com> --- 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) }
+ The above additional line should be removed.
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) } <snip>
Beside the nit above Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> -- 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
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@redhat.com> --- 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:44 PM, Jonathon Jongsma wrote:
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@redhat.com> --- src/node_device/node_device_udev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
LGTM and works for vfio-ap mdevs (therefore for all other types as well). Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> -- 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
On 7/6/23 9:40 PM, Jonathon Jongsma wrote:
On 6/30/23 6:34 AM, Boris Fiuczynski wrote:
Update the optional mdev attributes by running an mdevctl update on a new created nodedev object representing an mdev.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158 Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_udev.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4c37ec3189..fce4212728 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1757,12 +1757,20 @@ nodeStateCleanup(void) static int udevHandleOneDevice(struct udev_device *device) { + virNodeDevCapType dev_cap_type; const char *action = udev_device_get_action(device); VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device)); - if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(device); + if (STREQ(action, "add") || STREQ(action, "change")) { + int ret = udevAddOneDevice(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"); + return ret; + } if (STREQ(action, "remove")) return udevRemoveOneDevice(device);
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
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
Polite ping On 7/7/23 11:17 AM, Boris Fiuczynski wrote:
On 7/6/23 9:40 PM, Jonathon Jongsma wrote:
On 6/30/23 6:34 AM, Boris Fiuczynski wrote:
Update the optional mdev attributes by running an mdevctl update on a new created nodedev object representing an mdev.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158 Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_udev.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4c37ec3189..fce4212728 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1757,12 +1757,20 @@ nodeStateCleanup(void) static int udevHandleOneDevice(struct udev_device *device) { + virNodeDevCapType dev_cap_type; const char *action = udev_device_get_action(device); VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device)); - if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(device); + if (STREQ(action, "add") || STREQ(action, "change")) { + int ret = udevAddOneDevice(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"); + return ret; + } if (STREQ(action, "remove")) return udevRemoveOneDevice(device);
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
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
On 7/17/23 10:12 AM, Boris Fiuczynski wrote:
Polite ping
On 7/7/23 11:17 AM, Boris Fiuczynski wrote:
On 7/6/23 9:40 PM, Jonathon Jongsma wrote:
On 6/30/23 6:34 AM, Boris Fiuczynski wrote:
Update the optional mdev attributes by running an mdevctl update on a new created nodedev object representing an mdev.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158 Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> ---
Just found that it was push to master last Thursday. Thanks. -- 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
participants (2)
-
Boris Fiuczynski -
Jonathon Jongsma