[PATCH v1 00/20] node_dev_udev: use workerpool and improve nodedev events
When an udev event occurs for a mediated device (mdev) the mdev config data requires an update via mdevctl as the udev event does not contain all config data. This update needs to occur immediately and to be finished before the libvirt nodedev event is issued to keep the API usage reliable. Changelog: RFCv1->v1: + removed some of my own s-o-b's that were accidentally inserted in the RFC + added r-b's from Boris and Jonathon + worked in comments from Boris and Jonathon, but I did not inline "nodeDeviceDefResetMdevActiveConfig" as I'm not sure whether this improves the readability + reworked patch "[RFC PATCH v1 11/15] node_device_udev: Use `stateShutdownPrepare` and `stateShutdownWait`" + reworked patch "node_device_udev: Use a worker pool for processing events and emitting nodedev event" + added patches: - node_device_udev: Move responsibility to release `(init|udev)Thread` to `udevEventDataDispose` - node_device_udev: Fix leak of mdevctlLock, udevThreadCond, and mdevCtlMonitor - node_device_udev: nodeStateShutdownPrepare: Disconnect the signals explicitly - node_device_udev: Pass the driver state as parameter in prepartion for the next commit - node_device_udev: Add support for `g_autoptr` to `udevEventData - node_device_udev: Pass the `udevEventData` via parameter and use refcounting Boris Fiuczynski (3): nodedev: fix mdev add udev event data handling nodedev: immediate update of active config on udev add nodedev: reset active config data on udev remove event Marc Hartmayer (17): node_device_udev: Set @def to NULL node_device_udev: Remove the timeout if the data is disposed node_device_udev: Test for mdevctlTimeout != -1 node_device_udev: Don't take `mdevctlLock` for `mdevctl list` and add comments about locking node_device_udev: Take lock if `driver->privateData` is modified node_device_udev: Add prefix `udev` for udev related data node_device_udev: Inline `udevRemoveOneDevice` node_device_udev: Move responsibility to release `(init|udev)Thread` to `udevEventDataDispose` node_device_udev: Fix leak of mdevctlLock, udevThreadCond, and mdevCtlMonitors node_device_udev: Introduce and use `stateShutdownPrepare` and `stateShutdownWait` node_device_udev: nodeStateShutdownPrepare: Disconnect the signals explicitly node_device_udev: Pass the driver state as parameter in preparation for the next commit node_device_udev: Use a worker pool for processing events and emitting nodedev event node_device_udev: Call `nodeDeviceUpdateMediatedDevices` directly node_device_udev: Make the code easier to read node_device_udev: Add support for `g_autoptr` to `udevEventData` node_device_udev: Pass the `udevEventData` via parameter and use refcounting src/node_device/node_device_driver.h | 5 +- src/util/virmdev.h | 4 + src/conf/node_device_conf.c | 10 +- src/node_device/node_device_driver.c | 19 +- src/node_device/node_device_udev.c | 510 ++++++++++++++++++--------- src/test/test_driver.c | 3 +- src/util/virmdev.c | 20 ++ src/libvirt_private.syms | 2 + 8 files changed, 384 insertions(+), 189 deletions(-) base-commit: c38720b337f74337ec94c0fe2e97a7c2c57188ae -- 2.34.1
From: Boris Fiuczynski <fiuczy@linux.ibm.com> Two situations will trigger an udev add event: 1) the mdev is created when started (transient) or 2) the mdev was defined and is started In case 1 there is no node object existing and no config data is copied. In case 2 copying the active config data of an existing node object will only copy invalid data. Instead copying the defined config data will store valid data into the newly added node object. Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f1e402f8f7f6..4730a5b986ca 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device) objdef = virNodeDeviceObjGetDef(obj); if (is_mdev) - nodeDeviceDefCopyFromMdevctl(def, objdef, false); + nodeDeviceDefCopyFromMdevctl(def, objdef, true); persistent = virNodeDeviceObjIsPersistent(obj); autostart = virNodeDeviceObjIsAutostart(obj); -- 2.34.1
@def is owned by @obj after adding it the node device object list. As soon as the @obj lock has been released, another thread could free @obj and therefore @def. If now someone accesses @def this would lead to a heap-use-after-free and therefore most likely to a segmentation fault, therefore set @def to NULL after the ownership has moved. While at it, add comments to other code places why @def is set to NULL. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 4 ++++ src/test/test_driver.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4730a5b986ca..6613528d0e37 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1588,6 +1588,8 @@ udevAddOneDevice(struct udev_device *device) * and the current definition will take its place. */ if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + /* @def is now owned by @obj */ + def = NULL; virNodeDeviceObjSetPersistent(obj, persistent); virNodeDeviceObjSetAutostart(obj, autostart); objdef = virNodeDeviceObjGetDef(obj); @@ -1983,6 +1985,8 @@ udevSetupSystemDev(void) if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; + /* @def is now owned by @obj */ + def = NULL; virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetAutostart(obj, true); virNodeDeviceObjSetPersistent(obj, true); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b656..81b1ba4294bd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7656,8 +7656,9 @@ testNodeDeviceMockCreateVport(testDriver *driver, if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) goto cleanup; - virNodeDeviceObjSetSkipUpdateCaps(obj, true); + /* @def is now owned by @obj */ def = NULL; + virNodeDeviceObjSetSkipUpdateCaps(obj, true); objdef = virNodeDeviceObjGetDef(obj); event = virNodeDeviceEventLifecycleNew(objdef->name, -- 2.34.1
From: Boris Fiuczynski <fiuczy@linux.ibm.com> When an udev add event occurs the mdev active config data requires an update via mdevctl as the udev does not contain all config data. This update needs to occur immediately and to be finished before the libvirt CREATE event is issued to keep the API usage reliable. After this change, scheduleMdevctlUpdate call is already called in `udevAddOneDevice` and can therefore be removed in `udevHandleOneDevice`. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 6613528d0e37..44393c2718cb 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1535,6 +1535,7 @@ udevSetParent(struct udev_device *device, static int udevAddOneDevice(struct udev_device *device) { + g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; virNodeDeviceObj *obj = NULL; virNodeDeviceDef *objdef; @@ -1549,6 +1550,9 @@ udevAddOneDevice(struct udev_device *device) def = g_new0(virNodeDeviceDef, 1); def->sysfs_path = g_strdup(udev_device_get_syspath(device)); + /* Create a copy of sysfs_path so it can be safely accessed, even without + * holding the @obj lock during the VIR_WARN(...) call at the end. */ + sysfs_path = g_strdup(def->sysfs_path); udevGetStringProperty(device, "DRIVER", &def->driver); @@ -1608,6 +1612,14 @@ udevAddOneDevice(struct udev_device *device) if (has_mdev_types) scheduleMdevctlUpdate(driver->privateData, false); + /* The added mdev needs an immediate active config update before the event + * is issued so that full device information is available at the time that + * the 'created' event is emitted. */ + if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) { + VIR_WARN("Update of mediated device %s failed", + NULLSTR_EMPTY(sysfs_path)); + } + ret = 0; cleanup: @@ -1758,19 +1770,12 @@ 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")) { - int ret = udevAddOneDevice(device); - if (ret == 0 && - udevGetDeviceType(device, &dev_cap_type) == 0 && - dev_cap_type == VIR_NODE_DEV_CAP_MDEV) - scheduleMdevctlUpdate(driver->privateData, false); - return ret; - } + if (STREQ(action, "add") || STREQ(action, "change")) + return udevAddOneDevice(device); if (STREQ(action, "remove")) return udevRemoveOneDevice(device); -- 2.34.1
From: Boris Fiuczynski <fiuczy@linux.ibm.com> When a mdev device is destroyed or stopped the udev remove event handling needs to reset the active config data of the node object representing a persisted mdev. Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_driver.h | 3 +++ src/util/virmdev.h | 4 ++++ src/conf/node_device_conf.c | 10 ++-------- src/node_device/node_device_driver.c | 13 +++++++++++++ src/node_device/node_device_udev.c | 1 + src/util/virmdev.c | 20 ++++++++++++++++++++ src/libvirt_private.syms | 2 ++ 7 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index b3bc4b2e96ed..f195cfef9d49 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -197,3 +197,6 @@ int nodeDeviceUpdate(virNodeDevice *dev, const char *xmlDesc, unsigned int flags); + +void +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def); diff --git a/src/util/virmdev.h b/src/util/virmdev.h index 853041ac0619..e8e69040e5d4 100644 --- a/src/util/virmdev.h +++ b/src/util/virmdev.h @@ -54,6 +54,10 @@ struct _virMediatedDeviceConfig { size_t nattributes; }; +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config); +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceConfig, virMediatedDeviceConfigFree); + typedef struct _virMediatedDeviceType virMediatedDeviceType; struct _virMediatedDeviceType { char *id; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 5cfbd6a7eb72..897c67d6af91 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2592,15 +2592,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps) g_free(data->sg.path); break; case VIR_NODE_DEV_CAP_MDEV: - g_free(data->mdev.defined_config.type); - g_free(data->mdev.active_config.type); g_free(data->mdev.uuid); - for (i = 0; i < data->mdev.defined_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]); - g_free(data->mdev.defined_config.attributes); - for (i = 0; i < data->mdev.active_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]); - g_free(data->mdev.active_config.attributes); + virMediatedDeviceConfigClear(&data->mdev.defined_config); + virMediatedDeviceConfigClear(&data->mdev.active_config); g_free(data->mdev.parent_addr); break; case VIR_NODE_DEV_CAP_CSS_DEV: diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d99b48138ebf..f623339dc973 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -2018,6 +2018,19 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, } +/* A mediated device definition contains data from mdevctl about the active + * device. When the device is deactivated the active configuration data needs + * to be removed. */ +void +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def) +{ + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV) + return; + + virMediatedDeviceConfigClear(&def->caps->data.mdev.active_config); +} + + int nodeDeviceSetAutostart(virNodeDevice *device, int autostart) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 44393c2718cb..7eef802d5975 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1467,6 +1467,7 @@ udevRemoveOneDeviceSysPath(const char *path) if (virNodeDeviceObjIsPersistent(obj)) { VIR_FREE(def->sysfs_path); virNodeDeviceObjSetActive(obj, false); + nodeDeviceDefResetMdevActiveConfig(def); } else { VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, path); diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 992f3eb1b74c..6ecdbdf0ab77 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -516,6 +516,26 @@ void virMediatedDeviceAttrFree(virMediatedDeviceAttr *attr) g_free(attr); } +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config) +{ + if (!config) + return; + + virMediatedDeviceConfigClear(config); + g_free(config); +} + +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config) +{ + size_t i = 0; + + g_clear_pointer(&config->type, g_free); + for (i = 0; i < config->nattributes; i++) + virMediatedDeviceAttrFree(config->attributes[i]); + config->nattributes = 0; + g_clear_pointer(&config->attributes, g_free); +} + #define MDEV_BUS_DIR "/sys/class/mdev_bus" diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 839fe4f545f1..b3c0c576aa60 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2780,6 +2780,8 @@ virMacMapWriteFile; # util/virmdev.h virMediatedDeviceAttrFree; virMediatedDeviceAttrNew; +virMediatedDeviceConfigClear; +virMediatedDeviceConfigFree; virMediatedDeviceFree; virMediatedDeviceGetIOMMUGroupDev; virMediatedDeviceGetIOMMUGroupNum; -- 2.34.1
Remove the timeout when the udevEventData is disposed, analogous to priv->watch. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 7eef802d5975..4ccef628d84c 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,6 +88,9 @@ udevEventDataDispose(void *obj) if (priv->watch != -1) virEventRemoveHandle(priv->watch); + if (priv->mdevctlTimeout > 0) + virEventRemoveTimeout(priv->mdevctlTimeout); + if (!priv->udev_monitor) return; -- 2.34.1
It is done a little differently everywhere in libvirt, but most common is to test for != -1. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4ccef628d84c..049501c62870 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,7 +88,7 @@ udevEventDataDispose(void *obj) if (priv->watch != -1) virEventRemoveHandle(priv->watch); - if (priv->mdevctlTimeout > 0) + if (priv->mdevctlTimeout != -1) virEventRemoveTimeout(priv->mdevctlTimeout); if (!priv->udev_monitor) @@ -139,6 +139,7 @@ udevEventDataNew(void) return NULL; } + ret->mdevctlTimeout = -1; ret->watch = -1; return ret; } @@ -2087,7 +2088,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) udevEventData *priv = opaque; virThread thread; - if (priv->mdevctlTimeout > 0) { + if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); priv->mdevctlTimeout = -1; } @@ -2197,7 +2198,7 @@ scheduleMdevctlUpdate(udevEventData *data, bool force) { if (!force) { - if (data->mdevctlTimeout > 0) + if (data->mdevctlTimeout != -1) virEventRemoveTimeout(data->mdevctlTimeout); data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread, data, NULL); -- 2.34.1
Commit a99d876a0f58 ("node_device: Use automatic mutex management") replaced the locking mechanism and accidentally removed the comment with the reason why the lock is taken. The reason was to "ensure only a single thread can query mdevctl at a time", but this reason is no longer valid or maybe it never was. Therefore, let's remove this lock and add a comment to `mdevCtl` what it protects. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 049501c62870..757febffa2f8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -72,8 +72,9 @@ struct _udevEventData { /* init thread */ virThread *initThread; - GList *mdevctlMonitors; + /* Protects @mdevctlMonitors */ virMutex mdevctlLock; + GList *mdevctlMonitors; int mdevctlTimeout; }; @@ -2074,9 +2075,6 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) static void mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) { - udevEventData *priv = driver->privateData; - VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock); - if (nodeDeviceUpdateMediatedDevices() < 0) VIR_WARN("mdevctl failed to update mediated devices"); } -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
Commit a99d876a0f58 ("node_device: Use automatic mutex management") replaced the locking mechanism and accidentally removed the comment with the reason why the lock is taken. The reason was to "ensure only a single thread can query mdevctl at a time", but this reason is no longer valid or maybe it never was. Therefore, let's remove this lock and add a comment to `mdevCtl` what it protects.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 049501c62870..757febffa2f8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -72,8 +72,9 @@ struct _udevEventData { /* init thread */ virThread *initThread;
- GList *mdevctlMonitors; + /* Protects @mdevctlMonitors */ virMutex mdevctlLock; + GList *mdevctlMonitors; int mdevctlTimeout; };
@@ -2074,9 +2075,6 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) static void mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) { - udevEventData *priv = driver->privateData; - VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock); - if (nodeDeviceUpdateMediatedDevices() < 0) VIR_WARN("mdevctl failed to update mediated devices"); }
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
Commit a99d876a0f58 ("node_device: Use automatic mutex management") replaced the locking mechanism and accidentally removed the comment with the reason why the lock is taken. The reason was to "ensure only a single thread can query mdevctl at a time", but this reason is no longer valid or maybe it never was. Therefore, let's remove this lock and add a comment to `mdevCtl` what it protects.
Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com>
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Since @driver->privateData is modified take the lock. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 757febffa2f8..281e852c7ff1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1481,7 +1481,9 @@ udevRemoveOneDeviceSysPath(const char *path) virNodeDeviceObjEndAPI(&obj); /* cannot check for mdev_types since they have already been removed */ - scheduleMdevctlUpdate(driver->privateData, false); + VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { + scheduleMdevctlUpdate(driver->privateData, false); + } virObjectEventStateQueue(driver->nodeDeviceEventState, event); return 0; @@ -1615,8 +1617,11 @@ udevAddOneDevice(struct udev_device *device) has_mdev_types = virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_MDEV_TYPES); virNodeDeviceObjEndAPI(&obj); - if (has_mdev_types) - scheduleMdevctlUpdate(driver->privateData, false); + if (has_mdev_types) { + VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { + scheduleMdevctlUpdate(driver->privateData, false); + } + } /* The added mdev needs an immediate active config update before the event * is issued so that full device information is available at the time that @@ -2237,7 +2242,9 @@ 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 */ - scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + } } -- 2.34.1
The new names make it easier to understand the purpose of the data. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 48 +++++++++++++++--------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 281e852c7ff1..616ceca0b5c0 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -63,11 +63,11 @@ struct _udevEventData { struct udev_monitor *udev_monitor; int watch; - /* Thread data */ - virThread *th; - virCond threadCond; - bool threadQuit; - bool dataReady; + /* Udev thread data */ + virThread *udevThread; + virCond udevThreadCond; + bool udevThreadQuit; + bool udevDataReady; /* init thread */ virThread *initThread; @@ -104,7 +104,7 @@ udevEventDataDispose(void *obj) } virMutexDestroy(&priv->mdevctlLock); - virCondDestroy(&priv->threadCond); + virCondDestroy(&priv->udevThreadCond); } @@ -130,7 +130,7 @@ udevEventDataNew(void) if (!(ret = virObjectLockableNew(udevEventDataClass))) return NULL; - if (virCondInit(&ret->threadCond) < 0) { + if (virCondInit(&ret->udevThreadCond) < 0) { virObjectUnref(ret); return NULL; } @@ -1747,16 +1747,16 @@ nodeStateCleanup(void) priv = driver->privateData; if (priv) { VIR_WITH_OBJECT_LOCK_GUARD(priv) { - priv->threadQuit = true; - virCondSignal(&priv->threadCond); + priv->udevThreadQuit = true; + virCondSignal(&priv->udevThreadCond); } if (priv->initThread) { virThreadJoin(priv->initThread); g_clear_pointer(&priv->initThread, g_free); } - if (priv->th) { - virThreadJoin(priv->th); - g_clear_pointer(&priv->th, g_free); + if (priv->udevThread) { + virThreadJoin(priv->udevThread); + g_clear_pointer(&priv->udevThread, g_free); } } @@ -1865,15 +1865,15 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED) /* continue rather than break from the loop on non-fatal errors */ while (1) { VIR_WITH_OBJECT_LOCK_GUARD(priv) { - while (!priv->dataReady && !priv->threadQuit) { - if (virCondWait(&priv->threadCond, &priv->parent.lock)) { + while (!priv->udevDataReady && !priv->udevThreadQuit) { + if (virCondWait(&priv->udevThreadCond, &priv->parent.lock)) { virReportSystemError(errno, "%s", _("handler failed to wait on condition")); return; } } - if (priv->threadQuit) + if (priv->udevThreadQuit) return; errno = 0; @@ -1904,7 +1904,7 @@ udevEventHandleThread(void *opaque G_GNUC_UNUSED) * after the udev_monitor_receive_device wouldn't help much * due to event mgmt and scheduler timing. */ VIR_WITH_OBJECT_LOCK_GUARD(priv) { - priv->dataReady = false; + priv->udevDataReady = false; } continue; @@ -1931,11 +1931,11 @@ udevEventHandleCallback(int watch G_GNUC_UNUSED, VIR_LOCK_GUARD lock = virObjectLockGuard(priv); if (!udevEventMonitorSanityCheck(priv, fd)) - priv->threadQuit = true; + priv->udevThreadQuit = true; else - priv->dataReady = true; + priv->udevDataReady = true; - virCondSignal(&priv->threadCond); + virCondSignal(&priv->udevThreadCond); } @@ -2045,8 +2045,8 @@ nodeStateInitializeEnumerate(void *opaque) VIR_WITH_OBJECT_LOCK_GUARD(priv) { ignore_value(virEventRemoveHandle(priv->watch)); priv->watch = -1; - priv->threadQuit = true; - virCondSignal(&priv->threadCond); + priv->udevThreadQuit = true; + virCondSignal(&priv->udevThreadCond); } goto cleanup; @@ -2340,12 +2340,12 @@ nodeStateInitialize(bool privileged, udev_monitor_set_receive_buffer_size(priv->udev_monitor, 128 * 1024 * 1024); - priv->th = g_new0(virThread, 1); - if (virThreadCreateFull(priv->th, true, udevEventHandleThread, + priv->udevThread = g_new0(virThread, 1); + if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread, "udev-event", false, NULL) < 0) { virReportSystemError(errno, "%s", _("failed to create udev handler thread")); - g_clear_pointer(&priv->th, g_free); + g_clear_pointer(&priv->udevThread, g_free); goto unlock; } -- 2.34.1
Inline `udevRemoveOneDevice` as it's used only once. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 616ceca0b5c0..1c8832ebd990 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1489,16 +1489,6 @@ udevRemoveOneDeviceSysPath(const char *path) return 0; } - -static int -udevRemoveOneDevice(struct udev_device *device) -{ - const char *path = udev_device_get_syspath(device); - - return udevRemoveOneDeviceSysPath(path); -} - - static int udevSetParent(struct udev_device *device, virNodeDeviceDef *def) @@ -1788,8 +1778,11 @@ udevHandleOneDevice(struct udev_device *device) if (STREQ(action, "add") || STREQ(action, "change")) return udevAddOneDevice(device); - if (STREQ(action, "remove")) - return udevRemoveOneDevice(device); + if (STREQ(action, "remove")) { + const char *path = udev_device_get_syspath(device); + + return udevRemoveOneDeviceSysPath(path); + } if (STREQ(action, "move")) { const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); -- 2.34.1
Everything is released in `udevEventDataDispose` except for the threads, change this as this makes the code easier to read as it can be simplified a little. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1c8832ebd990..7f233652b461 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -86,12 +86,16 @@ udevEventDataDispose(void *obj) struct udev *udev = NULL; udevEventData *priv = obj; + g_clear_pointer(&priv->initThread, g_free); + if (priv->watch != -1) virEventRemoveHandle(priv->watch); if (priv->mdevctlTimeout != -1) virEventRemoveTimeout(priv->mdevctlTimeout); + g_clear_pointer(&priv->udevThread, g_free); + if (!priv->udev_monitor) return; @@ -1740,14 +1744,10 @@ nodeStateCleanup(void) priv->udevThreadQuit = true; virCondSignal(&priv->udevThreadCond); } - if (priv->initThread) { + if (priv->initThread) virThreadJoin(priv->initThread); - g_clear_pointer(&priv->initThread, g_free); - } - if (priv->udevThread) { + if (priv->udevThread) virThreadJoin(priv->udevThread); - g_clear_pointer(&priv->udevThread, g_free); - } } virObjectUnref(priv); @@ -2338,7 +2338,6 @@ nodeStateInitialize(bool privileged, "udev-event", false, NULL) < 0) { virReportSystemError(errno, "%s", _("failed to create udev handler thread")); - g_clear_pointer(&priv->udevThread, g_free); goto unlock; } @@ -2370,7 +2369,6 @@ nodeStateInitialize(bool privileged, "nodedev-init", false, udev) < 0) { virReportSystemError(errno, "%s", _("failed to create udev enumerate thread")); - g_clear_pointer(&priv->initThread, g_free); goto cleanup; } -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
Everything is released in `udevEventDataDispose` except for the threads, change this as this makes the code easier to read as it can be simplified a little.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1c8832ebd990..7f233652b461 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -86,12 +86,16 @@ udevEventDataDispose(void *obj) struct udev *udev = NULL; udevEventData *priv = obj;
+ g_clear_pointer(&priv->initThread, g_free); + if (priv->watch != -1) virEventRemoveHandle(priv->watch);
if (priv->mdevctlTimeout != -1) virEventRemoveTimeout(priv->mdevctlTimeout);
+ g_clear_pointer(&priv->udevThread, g_free); + if (!priv->udev_monitor) return;
@@ -1740,14 +1744,10 @@ nodeStateCleanup(void) priv->udevThreadQuit = true; virCondSignal(&priv->udevThreadCond); } - if (priv->initThread) { + if (priv->initThread) virThreadJoin(priv->initThread); - g_clear_pointer(&priv->initThread, g_free); - } - if (priv->udevThread) { + if (priv->udevThread) virThreadJoin(priv->udevThread); - g_clear_pointer(&priv->udevThread, g_free); - } }
virObjectUnref(priv); @@ -2338,7 +2338,6 @@ nodeStateInitialize(bool privileged, "udev-event", false, NULL) < 0) { virReportSystemError(errno, "%s", _("failed to create udev handler thread")); - g_clear_pointer(&priv->udevThread, g_free); goto unlock; }
@@ -2370,7 +2369,6 @@ nodeStateInitialize(bool privileged, "nodedev-init", false, udev) < 0) { virReportSystemError(errno, "%s", _("failed to create udev enumerate thread")); - g_clear_pointer(&priv->initThread, g_free); goto cleanup; }
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
Everything is released in `udevEventDataDispose` except for the threads, change this as this makes the code easier to read as it can be simplified a little.
Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Even if `priv->udev_monitor` was never initialized, the mdevctlLock, udevThread were. Therefore let's match the order of releasing the resources the order of allocating the resources in `nodeStateInitialize`. In addition, use `g_steal_pointer` in `g_list_free_full`. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 7f233652b461..1638a7196709 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,6 +88,10 @@ udevEventDataDispose(void *obj) g_clear_pointer(&priv->initThread, g_free); + VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors), g_object_unref); + } + if (priv->watch != -1) virEventRemoveHandle(priv->watch); @@ -96,16 +100,12 @@ udevEventDataDispose(void *obj) g_clear_pointer(&priv->udevThread, g_free); - if (!priv->udev_monitor) - return; - - udev = udev_monitor_get_udev(priv->udev_monitor); - udev_monitor_unref(priv->udev_monitor); - udev_unref(udev); - - VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { - g_list_free_full(priv->mdevctlMonitors, g_object_unref); + if (priv->udev_monitor) { + udev = udev_monitor_get_udev(priv->udev_monitor); + udev_monitor_unref(priv->udev_monitor); + udev_unref(udev); } + virMutexDestroy(&priv->mdevctlLock); virCondDestroy(&priv->udevThreadCond); -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
Even if `priv->udev_monitor` was never initialized, the mdevctlLock, udevThread were. Therefore let's match the order of releasing the resources the order of allocating the resources in `nodeStateInitialize`.
In addition, use `g_steal_pointer` in `g_list_free_full`.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 7f233652b461..1638a7196709 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -88,6 +88,10 @@ udevEventDataDispose(void *obj)
g_clear_pointer(&priv->initThread, g_free);
+ VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors), g_object_unref); + } + if (priv->watch != -1) virEventRemoveHandle(priv->watch);
@@ -96,16 +100,12 @@ udevEventDataDispose(void *obj)
g_clear_pointer(&priv->udevThread, g_free);
- if (!priv->udev_monitor) - return; - - udev = udev_monitor_get_udev(priv->udev_monitor); - udev_monitor_unref(priv->udev_monitor); - udev_unref(udev); - - VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { - g_list_free_full(priv->mdevctlMonitors, g_object_unref); + if (priv->udev_monitor) { + udev = udev_monitor_get_udev(priv->udev_monitor); + udev_monitor_unref(priv->udev_monitor); + udev_unref(udev); } + virMutexDestroy(&priv->mdevctlLock);
virCondDestroy(&priv->udevThreadCond);
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
Even if `priv->udev_monitor` was never initialized, the mdevctlLock, udevThread were. Therefore let's match the order of releasing the resources the order of allocating the resources in `nodeStateInitialize`.
In addition, use `g_steal_pointer` in `g_list_free_full`.
Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Introduce and use the driver functions for the node state shutdown preparation and wait. As they're also called in the error/cleanup path of `nodeStateInitialize`, they must be written in a way, that they can safely be executed even if not everything is initialized. In the next commit, these functions will be extended. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 84 ++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 21 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1638a7196709..a3006433e842 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -92,12 +92,6 @@ udevEventDataDispose(void *obj) g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors), g_object_unref); } - if (priv->watch != -1) - virEventRemoveHandle(priv->watch); - - if (priv->mdevctlTimeout != -1) - virEventRemoveTimeout(priv->mdevctlTimeout); - g_clear_pointer(&priv->udevThread, g_free); if (priv->udev_monitor) { @@ -1733,24 +1727,10 @@ udevPCITranslateDeinit(void) static int nodeStateCleanup(void) { - udevEventData *priv = NULL; - if (!driver) return -1; - priv = driver->privateData; - if (priv) { - VIR_WITH_OBJECT_LOCK_GUARD(priv) { - priv->udevThreadQuit = true; - virCondSignal(&priv->udevThreadCond); - } - if (priv->initThread) - virThreadJoin(priv->initThread); - if (priv->udevThread) - virThreadJoin(priv->udevThread); - } - - virObjectUnref(priv); + virObjectUnref(driver->privateData); virObjectUnref(driver->nodeDeviceEventState); virNodeDeviceObjListFree(driver->devs); @@ -2241,6 +2221,64 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, } +/* Note: It must be safe to call this function even if the driver was not + * successfully initialized. This must be considered when changing this + * function. */ +static int +nodeStateShutdownPrepare(void) +{ + udevEventData *priv = NULL; + + if (!driver) + return 0; + + priv = driver->privateData; + if (!priv) + return 0; + + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + if (priv->mdevctlTimeout != -1) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + if (priv->watch) { + virEventRemoveHandle(priv->watch); + priv->watch = -1; + } + + priv->udevThreadQuit = true; + virCondSignal(&priv->udevThreadCond); + } + return 0; +} + + +/* Note: It must be safe to call this function even if the driver was not + * successfully initialized. This must be considered when changing this + * function. */ +static int +nodeStateShutdownWait(void) +{ + udevEventData *priv = NULL; + + if (!driver) + return 0; + + priv = driver->privateData; + if (!priv) + return 0; + + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + if (priv->initThread) + virThreadJoin(priv->initThread); + if (priv->udevThread) + virThreadJoin(priv->udevThread); + } + return 0; +} + + static int nodeStateInitialize(bool privileged, const char *root, @@ -2375,6 +2413,8 @@ nodeStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_COMPLETE; cleanup: + nodeStateShutdownPrepare(); + nodeStateShutdownWait(); nodeStateCleanup(); return VIR_DRV_STATE_INIT_ERROR; @@ -2440,6 +2480,8 @@ static virStateDriver udevStateDriver = { .stateInitialize = nodeStateInitialize, /* 0.7.3 */ .stateCleanup = nodeStateCleanup, /* 0.7.3 */ .stateReload = nodeStateReload, /* 0.7.3 */ + .stateShutdownPrepare = nodeStateShutdownPrepare, /* 10.3.0 */ + .stateShutdownWait = nodeStateShutdownWait, /* 10.3.0 */ }; -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
Introduce and use the driver functions for the node state shutdown preparation and wait. As they're also called in the error/cleanup path of `nodeStateInitialize`, they must be written in a way, that they can safely be executed even if not everything is initialized.
In the next commit, these functions will be extended.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 84 ++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 21 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1638a7196709..a3006433e842 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -92,12 +92,6 @@ udevEventDataDispose(void *obj) g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors), g_object_unref); }
- if (priv->watch != -1) - virEventRemoveHandle(priv->watch); - - if (priv->mdevctlTimeout != -1) - virEventRemoveTimeout(priv->mdevctlTimeout); - g_clear_pointer(&priv->udevThread, g_free);
if (priv->udev_monitor) { @@ -1733,24 +1727,10 @@ udevPCITranslateDeinit(void) static int nodeStateCleanup(void) { - udevEventData *priv = NULL; - if (!driver) return -1;
- priv = driver->privateData; - if (priv) { - VIR_WITH_OBJECT_LOCK_GUARD(priv) { - priv->udevThreadQuit = true; - virCondSignal(&priv->udevThreadCond); - } - if (priv->initThread) - virThreadJoin(priv->initThread); - if (priv->udevThread) - virThreadJoin(priv->udevThread); - } - - virObjectUnref(priv); + virObjectUnref(driver->privateData); virObjectUnref(driver->nodeDeviceEventState);
virNodeDeviceObjListFree(driver->devs); @@ -2241,6 +2221,64 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, }
+/* Note: It must be safe to call this function even if the driver was not + * successfully initialized. This must be considered when changing this + * function. */ +static int +nodeStateShutdownPrepare(void) +{ + udevEventData *priv = NULL; + + if (!driver) + return 0; + + priv = driver->privateData; + if (!priv) + return 0; + + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + if (priv->mdevctlTimeout != -1) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + if (priv->watch) { + virEventRemoveHandle(priv->watch); + priv->watch = -1; + } + + priv->udevThreadQuit = true; + virCondSignal(&priv->udevThreadCond); + } + return 0; +} + + +/* Note: It must be safe to call this function even if the driver was not + * successfully initialized. This must be considered when changing this + * function. */ +static int +nodeStateShutdownWait(void) +{ + udevEventData *priv = NULL; + + if (!driver) + return 0; + + priv = driver->privateData; + if (!priv) + return 0; + + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + if (priv->initThread) + virThreadJoin(priv->initThread); + if (priv->udevThread) + virThreadJoin(priv->udevThread); + } + return 0; +} + + static int nodeStateInitialize(bool privileged, const char *root, @@ -2375,6 +2413,8 @@ nodeStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_COMPLETE;
cleanup: + nodeStateShutdownPrepare(); + nodeStateShutdownWait(); nodeStateCleanup(); return VIR_DRV_STATE_INIT_ERROR;
@@ -2440,6 +2480,8 @@ static virStateDriver udevStateDriver = { .stateInitialize = nodeStateInitialize, /* 0.7.3 */ .stateCleanup = nodeStateCleanup, /* 0.7.3 */ .stateReload = nodeStateReload, /* 0.7.3 */ + .stateShutdownPrepare = nodeStateShutdownPrepare, /* 10.3.0 */ + .stateShutdownWait = nodeStateShutdownWait, /* 10.3.0 */ };
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
Introduce and use the driver functions for the node state shutdown preparation and wait. As they're also called in the error/cleanup path of `nodeStateInitialize`, they must be written in a way, that they can safely be executed even if not everything is initialized.
In the next commit, these functions will be extended.
Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 84 ++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 21 deletions(-)
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
The documentation of gobject signals reads: "If you are connecting handlers to signals and using a GObject instance as your signal handler user data, you should remember to pair calls to g_signal_connect() with calls to g_signal_handler_disconnect() or g_signal_handlers_disconnect_by_func(). While signal handlers are automatically disconnected when the object emitting the signal is finalised..." [1] This means that the signal handlers are automatically disconnected as soon as the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`. But this also means that it's possible that new work is tried to be scheduled for the workerpool by the `mdevctlEventHandleCallback` (main thread context) even if the workerpool has already been stopped by `nodeStateShutdownWait`. To fully understand this, it's important to know that the main loop of the main thread is still running for some time even after `nodeStateShutdownPrepare` has been called. Let's avoid this situation by explicitly disconnect the signals during `nodeStateShutdownPrepare`, which is called in the main thread, so that no new work is attempted to be scheduled for the worker pool. [1] https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-handle... Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a3006433e842..38740033a66e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2236,6 +2236,12 @@ nodeStateShutdownPrepare(void) if (!priv) return 0; + VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + GList *tmp; + for (tmp = priv->mdevctlMonitors; tmp; tmp = tmp->next) + g_signal_handlers_disconnect_by_data(tmp->data, priv); + } + VIR_WITH_OBJECT_LOCK_GUARD(priv) { if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
The documentation of gobject signals reads:
"If you are connecting handlers to signals and using a GObject instance as your signal handler user data, you should remember to pair calls to g_signal_connect() with calls to g_signal_handler_disconnect() or g_signal_handlers_disconnect_by_func(). While signal handlers are automatically disconnected when the object emitting the signal is finalised..." [1]
This means that the signal handlers are automatically disconnected as soon as the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`. But this also means that it's possible that new work is tried to be scheduled for the workerpool by the `mdevctlEventHandleCallback` (main thread context) even if the workerpool has already been stopped by `nodeStateShutdownWait`. To fully understand this, it's important to know that the main loop of the main thread is still running for some time even after `nodeStateShutdownPrepare` has been called. Let's avoid this situation by explicitly disconnect the signals during `nodeStateShutdownPrepare`, which is called in the main thread, so that no new work is attempted to be scheduled for the worker pool.
[1] https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-handle...
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a3006433e842..38740033a66e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2236,6 +2236,12 @@ nodeStateShutdownPrepare(void) if (!priv) return 0;
+ VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + GList *tmp; + for (tmp = priv->mdevctlMonitors; tmp; tmp = tmp->next) + g_signal_handlers_disconnect_by_data(tmp->data, priv); + } + VIR_WITH_OBJECT_LOCK_GUARD(priv) { if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout);
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
The documentation of gobject signals reads:
"If you are connecting handlers to signals and using a GObject instance as your signal handler user data, you should remember to pair calls to g_signal_connect() with calls to g_signal_handler_disconnect() or g_signal_handlers_disconnect_by_func(). While signal handlers are automatically disconnected when the object emitting the signal is finalised..." [1]
This means that the signal handlers are automatically disconnected as soon as the `priv->mdevCtlMonitors` are finalised/released by `udevEventDataDispose`. But this also means that it's possible that new work is tried to be scheduled for the workerpool by the `mdevctlEventHandleCallback` (main thread context) even if the workerpool has already been stopped by `nodeStateShutdownWait`. To fully understand this, it's important to know that the main loop of the main thread is still running for some time even after `nodeStateShutdownPrepare` has been called. Let's avoid this situation by explicitly disconnect the signals during `nodeStateShutdownPrepare`, which is called in the main thread, so that no new work is attempted to be scheduled for the worker pool.
[1]https://docs.gtk.org/gobject/signals.html#memory-management-of-signal-handle...
Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 6 ++++++ 1 file changed, 6 insertions(+)
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
It's better practice for all functions called by the threads to pass the driver via parameter and not global variables. Easier to test and cleaner. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_driver.h | 2 +- src/node_device/node_device_driver.c | 6 +-- src/node_device/node_device_udev.c | 72 ++++++++++++++-------------- 3 files changed, 41 insertions(+), 39 deletions(-) diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index f195cfef9d49..2781ad136d68 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -147,7 +147,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring, bool defined); int -nodeDeviceUpdateMediatedDevices(void); +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *driver); void nodeDeviceGenerateName(virNodeDeviceDef *def, diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index f623339dc973..59c5f9b417a4 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1887,7 +1887,7 @@ removeMissingPersistentMdev(virNodeDeviceObj *obj, int -nodeDeviceUpdateMediatedDevices(void) +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *node_driver) { g_autofree virNodeDeviceDef **defs = NULL; g_autofree virNodeDeviceDef **act_defs = NULL; @@ -1911,7 +1911,7 @@ nodeDeviceUpdateMediatedDevices(void) /* Any mdevs that were previously defined but were not returned in the * latest mdevctl query should be removed from the device list */ data.defs = defs; - virNodeDeviceObjListForEachRemove(driver->devs, + virNodeDeviceObjListForEachRemove(node_driver->devs, removeMissingPersistentMdev, &data); for (i = 0; i < data.ndefs; i++) @@ -2374,7 +2374,7 @@ nodeDeviceUpdate(virNodeDevice *device, cleanup: virNodeDeviceObjEndAPI(&obj); if (updated) - nodeDeviceUpdateMediatedDevices(); + nodeDeviceUpdateMediatedDevices(driver); return ret; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 38740033a66e..e4b1532dc385 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -361,7 +361,7 @@ udevTranslatePCIIds(unsigned int vendor, static int -udevProcessPCI(struct udev_device *device, +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def) { virNodeDevCapPCIDev *pci_dev = &def->caps->data.pci_dev; @@ -372,8 +372,8 @@ udevProcessPCI(struct udev_device *device, char *p; bool privileged = false; - VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { - privileged = driver->privileged; + VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) { + privileged = driver_state->privileged; } pci_dev->klass = -1; @@ -1391,12 +1391,12 @@ udevGetDeviceType(struct udev_device *device, static int -udevGetDeviceDetails(struct udev_device *device, +udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def) { switch (def->caps->data.type) { case VIR_NODE_DEV_CAP_PCI_DEV: - return udevProcessPCI(device, def); + return udevProcessPCI(driver_state, device, def); case VIR_NODE_DEV_CAP_USB_DEV: return udevProcessUSBDevice(device, def); case VIR_NODE_DEV_CAP_USB_INTERFACE: @@ -1447,13 +1447,13 @@ static void scheduleMdevctlUpdate(udevEventData *data, bool force); static int -udevRemoveOneDeviceSysPath(const char *path) +udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state, const char *path) { virNodeDeviceObj *obj = NULL; virNodeDeviceDef *def; virObjectEvent *event = NULL; - if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) { + if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, path))) { VIR_DEBUG("Failed to find device to remove that has udev path '%s'", path); return -1; @@ -1474,21 +1474,21 @@ udevRemoveOneDeviceSysPath(const char *path) } else { VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, path); - virNodeDeviceObjListRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver_state->devs, obj); } virNodeDeviceObjEndAPI(&obj); /* cannot check for mdev_types since they have already been removed */ VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver->privateData, false); + scheduleMdevctlUpdate(driver_state->privateData, false); } - virObjectEventStateQueue(driver->nodeDeviceEventState, event); + virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); return 0; } static int -udevSetParent(struct udev_device *device, +udevSetParent(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def) { struct udev_device *parent_device = NULL; @@ -1511,7 +1511,7 @@ udevSetParent(struct udev_device *device, return -1; } - if ((obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, + if ((obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); def->parent = g_strdup(objdef->name); @@ -1529,7 +1529,7 @@ udevSetParent(struct udev_device *device, } static int -udevAddOneDevice(struct udev_device *device) +udevAddOneDevice(virNodeDeviceDriverState *driver_state, struct udev_device *device) { g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; @@ -1560,15 +1560,15 @@ udevAddOneDevice(struct udev_device *device) if (udevGetDeviceNodes(device, def) != 0) goto cleanup; - if (udevGetDeviceDetails(device, def) != 0) + if (udevGetDeviceDetails(driver_state, device, def) != 0) goto cleanup; - if (udevSetParent(device, def) != 0) + if (udevSetParent(driver_state, device, def) != 0) goto cleanup; is_mdev = def->caps->data.type == VIR_NODE_DEV_CAP_MDEV; - if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + if ((obj = virNodeDeviceObjListFindByName(driver_state->devs, def->name))) { objdef = virNodeDeviceObjGetDef(obj); if (is_mdev) @@ -1586,7 +1586,7 @@ udevAddOneDevice(struct udev_device *device) /* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver_state->devs, def))) goto cleanup; /* @def is now owned by @obj */ def = NULL; @@ -1606,15 +1606,15 @@ udevAddOneDevice(struct udev_device *device) virNodeDeviceObjEndAPI(&obj); if (has_mdev_types) { - VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver->privateData, false); + VIR_WITH_OBJECT_LOCK_GUARD(driver_state->privateData) { + scheduleMdevctlUpdate(driver_state->privateData, false); } } /* The added mdev needs an immediate active config update before the event * is issued so that full device information is available at the time that * the 'created' event is emitted. */ - if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) { + if (is_mdev && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { VIR_WARN("Update of mediated device %s failed", NULLSTR_EMPTY(sysfs_path)); } @@ -1622,7 +1622,7 @@ udevAddOneDevice(struct udev_device *device) ret = 0; cleanup: - virObjectEventStateQueue(driver->nodeDeviceEventState, event); + virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); if (ret != 0) { VIR_DEBUG("Discarding device %d %p %s", ret, def, @@ -1635,7 +1635,7 @@ udevAddOneDevice(struct udev_device *device) static int -udevProcessDeviceListEntry(struct udev *udev, +udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, struct udev *udev, struct udev_list_entry *list_entry) { struct udev_device *device; @@ -1647,7 +1647,7 @@ udevProcessDeviceListEntry(struct udev *udev, device = udev_device_new_from_syspath(udev, name); if (device != NULL) { - if (udevAddOneDevice(device) != 0) { + if (udevAddOneDevice(driver_state, device) != 0) { VIR_DEBUG("Failed to create node device for udev device '%s'", name); } @@ -1685,7 +1685,7 @@ udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate) static int -udevEnumerateDevices(struct udev *udev) +udevEnumerateDevices(virNodeDeviceDriverState *driver_state, struct udev *udev) { struct udev_enumerate *udev_enumerate = NULL; struct udev_list_entry *list_entry = NULL; @@ -1701,7 +1701,7 @@ udevEnumerateDevices(struct udev *udev) udev_list_entry_foreach(list_entry, udev_enumerate_get_list_entry(udev_enumerate)) { - udevProcessDeviceListEntry(udev, list_entry); + udevProcessDeviceListEntry(driver_state, udev, list_entry); } ret = 0; @@ -1756,12 +1756,12 @@ udevHandleOneDevice(struct udev_device *device) VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device)); if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(device); + return udevAddOneDevice(driver, device); if (STREQ(action, "remove")) { const char *path = udev_device_get_syspath(device); - return udevRemoveOneDeviceSysPath(path); + return udevRemoveOneDeviceSysPath(driver, path); } if (STREQ(action, "move")) { @@ -1770,10 +1770,10 @@ udevHandleOneDevice(struct udev_device *device) if (devpath_old) { g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); - udevRemoveOneDeviceSysPath(devpath_old_fixed); + udevRemoveOneDeviceSysPath(driver, devpath_old_fixed); } - return udevAddOneDevice(device); + return udevAddOneDevice(driver, device); } return 0; @@ -1995,15 +1995,15 @@ udevSetupSystemDev(void) static void nodeStateInitializeEnumerate(void *opaque) { - struct udev *udev = opaque; udevEventData *priv = driver->privateData; + struct udev *udev = opaque; /* Populate with known devices */ - if (udevEnumerateDevices(udev) != 0) + if (udevEnumerateDevices(driver, udev) != 0) goto error; /* Load persistent mdevs (which might not be activated yet) and additional * information about active mediated devices from mdevctl */ - if (nodeDeviceUpdateMediatedDevices() != 0) + if (nodeDeviceUpdateMediatedDevices(driver) != 0) goto error; cleanup: @@ -2051,9 +2051,11 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) static void -mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) +mdevctlUpdateThreadFunc(void *opaque) { - if (nodeDeviceUpdateMediatedDevices() < 0) + virNodeDeviceDriverState *driver_state = opaque; + + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) VIR_WARN("mdevctl failed to update mediated devices"); } @@ -2070,7 +2072,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) } if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, - "mdevctl-thread", false, NULL) < 0) { + "mdevctl-thread", false, driver) < 0) { virReportSystemError(errno, "%s", _("failed to create mdevctl thread")); } -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
It's better practice for all functions called by the threads to pass the driver via parameter and not global variables. Easier to test and cleaner.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_driver.h | 2 +- src/node_device/node_device_driver.c | 6 +-- src/node_device/node_device_udev.c | 72 ++++++++++++++-------------- 3 files changed, 41 insertions(+), 39 deletions(-)
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index f195cfef9d49..2781ad136d68 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -147,7 +147,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring, bool defined);
int -nodeDeviceUpdateMediatedDevices(void); +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *driver);
void nodeDeviceGenerateName(virNodeDeviceDef *def, diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index f623339dc973..59c5f9b417a4 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1887,7 +1887,7 @@ removeMissingPersistentMdev(virNodeDeviceObj *obj,
int -nodeDeviceUpdateMediatedDevices(void) +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *node_driver) { g_autofree virNodeDeviceDef **defs = NULL; g_autofree virNodeDeviceDef **act_defs = NULL; @@ -1911,7 +1911,7 @@ nodeDeviceUpdateMediatedDevices(void) /* Any mdevs that were previously defined but were not returned in the * latest mdevctl query should be removed from the device list */ data.defs = defs; - virNodeDeviceObjListForEachRemove(driver->devs, + virNodeDeviceObjListForEachRemove(node_driver->devs, removeMissingPersistentMdev, &data);
for (i = 0; i < data.ndefs; i++) @@ -2374,7 +2374,7 @@ nodeDeviceUpdate(virNodeDevice *device, cleanup: virNodeDeviceObjEndAPI(&obj); if (updated) - nodeDeviceUpdateMediatedDevices(); + nodeDeviceUpdateMediatedDevices(driver);
return ret; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 38740033a66e..e4b1532dc385 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -361,7 +361,7 @@ udevTranslatePCIIds(unsigned int vendor,
static int -udevProcessPCI(struct udev_device *device, +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def)
While there are exceptions, the general coding style is to have a single argument per line for function definitions.
{ virNodeDevCapPCIDev *pci_dev = &def->caps->data.pci_dev; @@ -372,8 +372,8 @@ udevProcessPCI(struct udev_device *device, char *p; bool privileged = false;
- VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { - privileged = driver->privileged; + VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) { + privileged = driver_state->privileged; }
pci_dev->klass = -1; @@ -1391,12 +1391,12 @@ udevGetDeviceType(struct udev_device *device,
static int -udevGetDeviceDetails(struct udev_device *device, +udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def)
same
{ switch (def->caps->data.type) { case VIR_NODE_DEV_CAP_PCI_DEV: - return udevProcessPCI(device, def); + return udevProcessPCI(driver_state, device, def); case VIR_NODE_DEV_CAP_USB_DEV: return udevProcessUSBDevice(device, def); case VIR_NODE_DEV_CAP_USB_INTERFACE: @@ -1447,13 +1447,13 @@ static void scheduleMdevctlUpdate(udevEventData *data, bool force);
static int -udevRemoveOneDeviceSysPath(const char *path) +udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state, const char *path)
same
{ virNodeDeviceObj *obj = NULL; virNodeDeviceDef *def; virObjectEvent *event = NULL;
- if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) { + if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, path))) { VIR_DEBUG("Failed to find device to remove that has udev path '%s'", path); return -1; @@ -1474,21 +1474,21 @@ udevRemoveOneDeviceSysPath(const char *path) } else { VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, path); - virNodeDeviceObjListRemove(driver->devs, obj); + virNodeDeviceObjListRemove(driver_state->devs, obj); } virNodeDeviceObjEndAPI(&obj);
/* cannot check for mdev_types since they have already been removed */ VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver->privateData, false); + scheduleMdevctlUpdate(driver_state->privateData, false); }
- virObjectEventStateQueue(driver->nodeDeviceEventState, event); + virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); return 0; }
static int -udevSetParent(struct udev_device *device, +udevSetParent(virNodeDeviceDriverState *driver_state, struct udev_device *device,
same
virNodeDeviceDef *def) { struct udev_device *parent_device = NULL; @@ -1511,7 +1511,7 @@ udevSetParent(struct udev_device *device, return -1; }
- if ((obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, + if ((obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, parent_sysfs_path))) { objdef = virNodeDeviceObjGetDef(obj); def->parent = g_strdup(objdef->name); @@ -1529,7 +1529,7 @@ udevSetParent(struct udev_device *device, }
static int -udevAddOneDevice(struct udev_device *device) +udevAddOneDevice(virNodeDeviceDriverState *driver_state, struct udev_device *device)
same
{ g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; @@ -1560,15 +1560,15 @@ udevAddOneDevice(struct udev_device *device) if (udevGetDeviceNodes(device, def) != 0) goto cleanup;
- if (udevGetDeviceDetails(device, def) != 0) + if (udevGetDeviceDetails(driver_state, device, def) != 0) goto cleanup;
- if (udevSetParent(device, def) != 0) + if (udevSetParent(driver_state, device, def) != 0) goto cleanup;
is_mdev = def->caps->data.type == VIR_NODE_DEV_CAP_MDEV;
- if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) { + if ((obj = virNodeDeviceObjListFindByName(driver_state->devs, def->name))) { objdef = virNodeDeviceObjGetDef(obj);
if (is_mdev) @@ -1586,7 +1586,7 @@ udevAddOneDevice(struct udev_device *device)
/* If this is a device change, the old definition will be freed * and the current definition will take its place. */ - if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def))) + if (!(obj = virNodeDeviceObjListAssignDef(driver_state->devs, def))) goto cleanup; /* @def is now owned by @obj */ def = NULL; @@ -1606,15 +1606,15 @@ udevAddOneDevice(struct udev_device *device) virNodeDeviceObjEndAPI(&obj);
if (has_mdev_types) { - VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver->privateData, false); + VIR_WITH_OBJECT_LOCK_GUARD(driver_state->privateData) { + scheduleMdevctlUpdate(driver_state->privateData, false); } }
/* The added mdev needs an immediate active config update before the event * is issued so that full device information is available at the time that * the 'created' event is emitted. */ - if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) { + if (is_mdev && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { VIR_WARN("Update of mediated device %s failed", NULLSTR_EMPTY(sysfs_path)); } @@ -1622,7 +1622,7 @@ udevAddOneDevice(struct udev_device *device) ret = 0;
cleanup: - virObjectEventStateQueue(driver->nodeDeviceEventState, event); + virObjectEventStateQueue(driver_state->nodeDeviceEventState, event);
if (ret != 0) { VIR_DEBUG("Discarding device %d %p %s", ret, def, @@ -1635,7 +1635,7 @@ udevAddOneDevice(struct udev_device *device)
static int -udevProcessDeviceListEntry(struct udev *udev, +udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, struct udev *udev,
same
struct udev_list_entry *list_entry) { struct udev_device *device; @@ -1647,7 +1647,7 @@ udevProcessDeviceListEntry(struct udev *udev, device = udev_device_new_from_syspath(udev, name);
if (device != NULL) { - if (udevAddOneDevice(device) != 0) { + if (udevAddOneDevice(driver_state, device) != 0) { VIR_DEBUG("Failed to create node device for udev device '%s'", name); } @@ -1685,7 +1685,7 @@ udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate)
static int -udevEnumerateDevices(struct udev *udev) +udevEnumerateDevices(virNodeDeviceDriverState *driver_state, struct udev *udev)
same
{ struct udev_enumerate *udev_enumerate = NULL; struct udev_list_entry *list_entry = NULL; @@ -1701,7 +1701,7 @@ udevEnumerateDevices(struct udev *udev) udev_list_entry_foreach(list_entry, udev_enumerate_get_list_entry(udev_enumerate)) {
- udevProcessDeviceListEntry(udev, list_entry); + udevProcessDeviceListEntry(driver_state, udev, list_entry); }
ret = 0; @@ -1756,12 +1756,12 @@ udevHandleOneDevice(struct udev_device *device) VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));
if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(device); + return udevAddOneDevice(driver, device);
if (STREQ(action, "remove")) { const char *path = udev_device_get_syspath(device);
- return udevRemoveOneDeviceSysPath(path); + return udevRemoveOneDeviceSysPath(driver, path); }
if (STREQ(action, "move")) { @@ -1770,10 +1770,10 @@ udevHandleOneDevice(struct udev_device *device) if (devpath_old) { g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old);
- udevRemoveOneDeviceSysPath(devpath_old_fixed); + udevRemoveOneDeviceSysPath(driver, devpath_old_fixed); }
- return udevAddOneDevice(device); + return udevAddOneDevice(driver, device); }
return 0; @@ -1995,15 +1995,15 @@ udevSetupSystemDev(void) static void nodeStateInitializeEnumerate(void *opaque) { - struct udev *udev = opaque; udevEventData *priv = driver->privateData; + struct udev *udev = opaque;
unnecessary change?
/* Populate with known devices */ - if (udevEnumerateDevices(udev) != 0) + if (udevEnumerateDevices(driver, udev) != 0) goto error; /* Load persistent mdevs (which might not be activated yet) and additional * information about active mediated devices from mdevctl */ - if (nodeDeviceUpdateMediatedDevices() != 0) + if (nodeDeviceUpdateMediatedDevices(driver) != 0) goto error;
cleanup: @@ -2051,9 +2051,11 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
static void -mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) +mdevctlUpdateThreadFunc(void *opaque) { - if (nodeDeviceUpdateMediatedDevices() < 0) + virNodeDeviceDriverState *driver_state = opaque; + + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) VIR_WARN("mdevctl failed to update mediated devices"); }
@@ -2070,7 +2072,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) }
if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, - "mdevctl-thread", false, NULL) < 0) { + "mdevctl-thread", false, driver) < 0) { virReportSystemError(errno, "%s", _("failed to create mdevctl thread")); }
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
It's better practice for all functions called by the threads to pass the driver via parameter and not global variables. Easier to test and cleaner.
[…snip…]
static int -udevProcessPCI(struct udev_device *device, +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def)
While there are exceptions, the general coding style is to have a single argument per line for function definitions.
Okay. BTW, why is there no .clangformat configuration available for Libvirt? :/ […snip…]
return 0; @@ -1995,15 +1995,15 @@ udevSetupSystemDev(void) static void nodeStateInitializeEnumerate(void *opaque) { - struct udev *udev = opaque; udevEventData *priv = driver->privateData; + struct udev *udev = opaque;
unnecessary change?
Will remove.
/* Populate with known devices */ - if (udevEnumerateDevices(udev) != 0) + if (udevEnumerateDevices(driver, udev) != 0) goto error; /* Load persistent mdevs (which might not be activated yet) and additional * information about active mediated devices from mdevctl */ - if (nodeDeviceUpdateMediatedDevices() != 0) + if (nodeDeviceUpdateMediatedDevices(driver) != 0) goto error;
cleanup: @@ -2051,9 +2051,11 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
static void -mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) +mdevctlUpdateThreadFunc(void *opaque) { - if (nodeDeviceUpdateMediatedDevices() < 0) + virNodeDeviceDriverState *driver_state = opaque; + + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) VIR_WARN("mdevctl failed to update mediated devices"); }
@@ -2070,7 +2072,7 @@ launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) }
if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, - "mdevctl-thread", false, NULL) < 0) { + "mdevctl-thread", false, driver) < 0) { virReportSystemError(errno, "%s", _("failed to create mdevctl thread")); }
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Thanks.
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote:
On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
It's better practice for all functions called by the threads to pass the driver via parameter and not global variables. Easier to test and cleaner.
[…snip…]
static int -udevProcessPCI(struct udev_device *device, +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def)
While there are exceptions, the general coding style is to have a single argument per line for function definitions.
Okay. BTW, why is there no .clangformat configuration available for Libvirt? :/
There is no combination of clangformat settings that can match libvirt code style. If we were startnig again from scratch we would of course want to match a defined clangformat style, but unless we're willing to bulk reformat the codebase we are stuck with what we have. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote:
On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
It's better practice for all functions called by the threads to pass the driver via parameter and not global variables. Easier to test and cleaner.
[…snip…]
static int -udevProcessPCI(struct udev_device *device, +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def)
While there are exceptions, the general coding style is to have a single argument per line for function definitions.
Okay. BTW, why is there no .clangformat configuration available for Libvirt? :/
There is no combination of clangformat settings that can match libvirt code style. If we were startnig again from scratch we would of course want to match a defined clangformat style, but unless we're willing to bulk reformat the codebase we are stuck with what we have.
Hmm, is the downside of doing a full codebase reformat greater than always doing the code formatting manually? Probably it is, otherwise it would have already be done :) If we would have such a big commit we could list it in a `.git-blame-ignore-revs` file so it will ignored by git blames.
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, Apr 23, 2024 at 10:46:14AM +0200, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote:
On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
It's better practice for all functions called by the threads to pass the driver via parameter and not global variables. Easier to test and cleaner.
[…snip…]
static int -udevProcessPCI(struct udev_device *device, +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device, virNodeDeviceDef *def)
While there are exceptions, the general coding style is to have a single argument per line for function definitions.
Okay. BTW, why is there no .clangformat configuration available for Libvirt? :/
There is no combination of clangformat settings that can match libvirt code style. If we were startnig again from scratch we would of course want to match a defined clangformat style, but unless we're willing to bulk reformat the codebase we are stuck with what we have.
Hmm, is the downside of doing a full codebase reformat greater than always doing the code formatting manually? Probably it is, otherwise it would have already be done :)
If we would have such a big commit we could list it in a `.git-blame-ignore-revs` file so it will ignored by git blames.
Yes, that's great for git blame. The bigger problem though is that a bulk reformat will immediately kill the ability of distro maintainers to cleanly cherry-pick patches across the big reformat commit. Cherry-picking the reformat likely won't be clean either, so they'll be faced with many patches needing manual editting. The tricky question is whether it is none the less worthwhile doing it. The distro maintainer pain will be very real, but also somewhat timelimited, as the need to backport fixes to a given release as it ages. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Apr 23, 2024 at 10:06 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Apr 23, 2024 at 10:46:14AM +0200, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote:
On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
It's better practice for all functions called by the threads to
On 4/19/24 9:49 AM, Marc Hartmayer wrote: pass the driver
[…snip…]
Hmm, is the downside of doing a full codebase reformat greater than always doing the code formatting manually? Probably it is, otherwise it would have already be done :)
If we would have such a big commit we could list it in a `.git-blame-ignore-revs` file so it will ignored by git blames.
Yes, that's great for git blame. The bigger problem though is that a bulk reformat will immediately kill the ability of distro maintainers to cleanly cherry-pick patches across the big reformat commit. Cherry-picking the reformat likely won't be clean either, so they'll be faced with many patches needing manual editting.
Yes, but isn't that already the case most of the time? But since I do not backport libvirt fixes, I cannot answer this for sure :) Anyhow, we shouldn't misuse this mail thread for the side discussion :) Is it worth starting a separate thread for it or is the answer a clear NACK?
The tricky question is whether it is none the less worthwhile doing it. The distro maintainer pain will be very real, but also somewhat timelimited, as the need to backport fixes to a given release as it ages.
If the people doing the backporting are more or less libvirt developers, it might be a good trade for them in the long run.
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 4/23/24 3:41 AM, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 10:06 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Apr 23, 2024 at 10:46:14AM +0200, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Apr 23, 2024 at 10:03:35AM +0200, Marc Hartmayer wrote:
On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
On 4/19/24 9:49 AM, Marc Hartmayer wrote: > It's better practice for all functions called by the threads to pass the driver
[…snip…]
Hmm, is the downside of doing a full codebase reformat greater than always doing the code formatting manually? Probably it is, otherwise it would have already be done :)
If we would have such a big commit we could list it in a `.git-blame-ignore-revs` file so it will ignored by git blames.
Yes, that's great for git blame. The bigger problem though is that a bulk reformat will immediately kill the ability of distro maintainers to cleanly cherry-pick patches across the big reformat commit. Cherry-picking the reformat likely won't be clean either, so they'll be faced with many patches needing manual editting.
Yes, but isn't that already the case most of the time? But since I do not backport libvirt fixes, I cannot answer this for sure :)
As a downstream package maintainer, I always shake my fist at these types of reformat changes when they break clean cherry-picks. But I also understand progress, modernization, etc can't be hamstrung by downstream convenience :-). Regards, Jim
Anyhow, we shouldn't misuse this mail thread for the side discussion :) Is it worth starting a separate thread for it or is the answer a clear NACK?
The tricky question is whether it is none the less worthwhile doing it. The distro maintainer pain will be very real, but also somewhat timelimited, as the need to backport fixes to a given release as it ages.
If the people doing the backporting are more or less libvirt developers, it might be a good trade for them in the long run.
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
It's better practice for all functions called by the threads to pass the driver via parameter and not global variables. Easier to test and cleaner.
Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com> --- src/node_device/node_device_driver.h | 2 +- src/node_device/node_device_driver.c | 6 +-- src/node_device/node_device_udev.c | 72 ++++++++++++++-------------- 3 files changed, 41 insertions(+), 39 deletions(-)
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Use a worker pool for processing the events (e.g. udev, mdevctl config changes) and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and emitting the libvirt nodedev events. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 244 +++++++++++++++++++++-------- 1 file changed, 179 insertions(+), 65 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e4b1532dc385..67a8b5cd7132 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -43,6 +43,7 @@ #include "virnetdev.h" #include "virmdev.h" #include "virutil.h" +#include "virthreadpool.h" #include "configmake.h" @@ -69,13 +70,13 @@ struct _udevEventData { bool udevThreadQuit; bool udevDataReady; - /* init thread */ - virThread *initThread; - /* Protects @mdevctlMonitors */ virMutex mdevctlLock; GList *mdevctlMonitors; int mdevctlTimeout; + + /* Immutable pointer, self-locking APIs */ + virThreadPool *workerPool; }; static virClass *udevEventDataClass; @@ -86,8 +87,6 @@ udevEventDataDispose(void *obj) struct udev *udev = NULL; udevEventData *priv = obj; - g_clear_pointer(&priv->initThread, g_free); - VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors), g_object_unref); } @@ -100,6 +99,8 @@ udevEventDataDispose(void *obj) udev_unref(udev); } + g_clear_pointer(&priv->workerPool, virThreadPoolFree); + virMutexDestroy(&priv->mdevctlLock); virCondDestroy(&priv->udevThreadCond); @@ -143,6 +144,66 @@ udevEventDataNew(void) return ret; } +typedef enum { + NODE_DEVICE_EVENT_INIT = 0, + NODE_DEVICE_EVENT_UDEV_ADD, + NODE_DEVICE_EVENT_UDEV_REMOVE, + NODE_DEVICE_EVENT_UDEV_CHANGE, + NODE_DEVICE_EVENT_UDEV_MOVE, + NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED, + + NODE_DEVICE_EVENT_LAST +} nodeDeviceEventType; + +struct _nodeDeviceEvent { + nodeDeviceEventType eventType; + void *data; + virFreeCallback dataFreeFunc; +}; +typedef struct _nodeDeviceEvent nodeDeviceEvent; + +static void +nodeDeviceEventFree(nodeDeviceEvent *event) +{ + if (!event) + return; + + if (event->dataFreeFunc) + event->dataFreeFunc(event->data); + g_free(event); +} +G_DEFINE_AUTOPTR_CLEANUP_FUNC(nodeDeviceEvent, nodeDeviceEventFree); + + /** + * nodeDeviceEventSubmit: + * @eventType: the event to be processed + * @data: additional data for the event processor (the pointer is stolen and it + * will be properly freed using @dataFreeFunc) + * @dataFreeFunc: callback to free @data + * + * Submits @eventType to be processed by the asynchronous event handling + * thread. + */ +static int nodeDeviceEventSubmit(nodeDeviceEventType eventType, void *data, virFreeCallback dataFreeFunc) +{ + nodeDeviceEvent *event = g_new0(nodeDeviceEvent, 1); + udevEventData *priv = NULL; + + if (!driver) + return -1; + + priv = driver->privateData; + + event->eventType = eventType; + event->data = data; + event->dataFreeFunc = dataFreeFunc; + if (virThreadPoolSendJob(priv->workerPool, 0, event) < 0) { + nodeDeviceEventFree(event); + return -1; + } + return 0; +} + static bool udevHasDeviceProperty(struct udev_device *dev, @@ -1447,7 +1508,7 @@ static void scheduleMdevctlUpdate(udevEventData *data, bool force); static int -udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state, const char *path) +processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char *path) { virNodeDeviceObj *obj = NULL; virNodeDeviceDef *def; @@ -1529,7 +1590,7 @@ udevSetParent(virNodeDeviceDriverState *driver_state, struct udev_device *device } static int -udevAddOneDevice(virNodeDeviceDriverState *driver_state, struct udev_device *device) +processNodeDeviceAddAndChangeEvent(virNodeDeviceDriverState *driver_state, struct udev_device *device) { g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; @@ -1647,7 +1708,7 @@ udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, struct udev * device = udev_device_new_from_syspath(udev, name); if (device != NULL) { - if (udevAddOneDevice(driver_state, device) != 0) { + if (processNodeDeviceAddAndChangeEvent(driver_state, device) != 0) { VIR_DEBUG("Failed to create node device for udev device '%s'", name); } @@ -1755,26 +1816,23 @@ udevHandleOneDevice(struct udev_device *device) VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device)); - if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(driver, device); - - if (STREQ(action, "remove")) { - const char *path = udev_device_get_syspath(device); - - return udevRemoveOneDeviceSysPath(driver, path); - } - - if (STREQ(action, "move")) { - const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); - - if (devpath_old) { - g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); - - udevRemoveOneDeviceSysPath(driver, devpath_old_fixed); - } - - return udevAddOneDevice(driver, device); + /* Reference is either released via workerpool logic or at the end of this + * function. */ + device = udev_device_ref(device); + if (STREQ(action, "add")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_ADD, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "change")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_CHANGE, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "remove")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_REMOVE, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "move")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_MOVE, device, + (virFreeCallback)udev_device_unref); } + udev_device_unref(device); return 0; } @@ -1993,23 +2051,23 @@ udevSetupSystemDev(void) static void -nodeStateInitializeEnumerate(void *opaque) +processNodeStateInitializeEnumerate(virNodeDeviceDriverState *driver_state, void *opaque) { - udevEventData *priv = driver->privateData; + udevEventData *priv = driver_state->privateData; struct udev *udev = opaque; /* Populate with known devices */ - if (udevEnumerateDevices(driver, udev) != 0) + if (udevEnumerateDevices(driver_state, udev) != 0) goto error; /* Load persistent mdevs (which might not be activated yet) and additional * information about active mediated devices from mdevctl */ - if (nodeDeviceUpdateMediatedDevices(driver) != 0) + if (nodeDeviceUpdateMediatedDevices(driver_state) != 0) goto error; cleanup: - VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { - driver->initialized = true; - virCondBroadcast(&driver->initCond); + VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) { + driver_state->initialized = true; + virCondBroadcast(&driver_state->initCond); } return; @@ -2051,31 +2109,16 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED) static void -mdevctlUpdateThreadFunc(void *opaque) -{ - virNodeDeviceDriverState *driver_state = opaque; - - if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) - VIR_WARN("mdevctl failed to update mediated devices"); -} - - -static void -launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) +submitMdevctlUpdate(int timer G_GNUC_UNUSED, void *opaque) { udevEventData *priv = opaque; - virThread thread; if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); priv->mdevctlTimeout = -1; } - if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, - "mdevctl-thread", false, driver) < 0) { - virReportSystemError(errno, "%s", - _("failed to create mdevctl thread")); - } + nodeDeviceEventSubmit(NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED, NULL, NULL); } @@ -2170,7 +2213,7 @@ 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. */ + * the worker job is submitted immediately. */ static void scheduleMdevctlUpdate(udevEventData *data, bool force) @@ -2178,12 +2221,12 @@ scheduleMdevctlUpdate(udevEventData *data, if (!force) { if (data->mdevctlTimeout != -1) virEventRemoveTimeout(data->mdevctlTimeout); - data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread, + data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, data, NULL); return; } - launchMdevctlUpdateThread(-1, data); + submitMdevctlUpdate(-1, data); } @@ -2223,6 +2266,62 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, } +static void nodeDeviceEventHandler(void *data, void *opaque) +{ + virNodeDeviceDriverState *driver_state = opaque; + g_autoptr(nodeDeviceEvent) processEvent = data; + + switch (processEvent->eventType) { + case NODE_DEVICE_EVENT_INIT: + { + struct udev *udev = processEvent->data; + + processNodeStateInitializeEnumerate(driver_state, udev); + } + break; + case NODE_DEVICE_EVENT_UDEV_ADD: + case NODE_DEVICE_EVENT_UDEV_CHANGE: + { + struct udev_device *device = processEvent->data; + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_UDEV_REMOVE: + { + struct udev_device *device = processEvent->data; + const char *path = udev_device_get_syspath(device); + + processNodeDeviceRemoveEvent(driver_state, path); + } + break; + case NODE_DEVICE_EVENT_UDEV_MOVE: + { + struct udev_device *device = processEvent->data; + const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); + + if (devpath_old) { + g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); + + processNodeDeviceRemoveEvent(driver_state, devpath_old_fixed); + } + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED: + { + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) + VIR_WARN("mdevctl failed to update mediated devices"); + } + break; + case NODE_DEVICE_EVENT_LAST: + g_assert_not_reached(); + break; + } +} + + /* Note: It must be safe to call this function even if the driver was not * successfully initialized. This must be considered when changing this * function. */ @@ -2258,6 +2357,9 @@ nodeStateShutdownPrepare(void) priv->udevThreadQuit = true; virCondSignal(&priv->udevThreadCond); } + + if (priv->workerPool) + virThreadPoolStop(priv->workerPool); return 0; } @@ -2278,11 +2380,19 @@ nodeStateShutdownWait(void) return 0; VIR_WITH_OBJECT_LOCK_GUARD(priv) { - if (priv->initThread) - virThreadJoin(priv->initThread); - if (priv->udevThread) - virThreadJoin(priv->udevThread); + if (priv->mdevctlTimeout != -1) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + if (priv->watch) { + virEventRemoveHandle(priv->watch); + priv->watch = -1; + } } + + if (priv->workerPool) + virThreadPoolDrain(priv->workerPool); return 0; } @@ -2353,6 +2463,16 @@ nodeStateInitialize(bool privileged, driver->parserCallbacks.postParse = nodeDeviceDefPostParse; driver->parserCallbacks.validate = nodeDeviceDefValidate; + /* must be initialized before trying to reconnect to all the running mdevs + * since there might occur some mdevctl monitor events that will be + * dispatched to the worker pool */ + priv->workerPool = virThreadPoolNewFull(1, 1, 0, nodeDeviceEventHandler, + "nodev-device-event", + NULL, + driver); + if (!priv->workerPool) + goto unlock; + if (udevPCITranslateInit(privileged) < 0) goto unlock; @@ -2410,13 +2530,7 @@ nodeStateInitialize(bool privileged, if (udevSetupSystemDev() != 0) goto cleanup; - priv->initThread = g_new0(virThread, 1); - if (virThreadCreateFull(priv->initThread, true, nodeStateInitializeEnumerate, - "nodedev-init", false, udev) < 0) { - virReportSystemError(errno, "%s", - _("failed to create udev enumerate thread")); - goto cleanup; - } + nodeDeviceEventSubmit(NODE_DEVICE_EVENT_INIT, udev_ref(udev), (virFreeCallback)udev_unref); return VIR_DRV_STATE_INIT_COMPLETE; -- 2.34.1
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
Use a worker pool for processing the events (e.g. udev, mdevctl config changes) and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and emitting the libvirt nodedev events.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 244 +++++++++++++++++++++-------- 1 file changed, 179 insertions(+), 65 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e4b1532dc385..67a8b5cd7132 100644
[…snip…]
}
@@ -2278,11 +2380,19 @@ nodeStateShutdownWait(void) return 0;
VIR_WITH_OBJECT_LOCK_GUARD(priv) { - if (priv->initThread) - virThreadJoin(priv->initThread); - if (priv->udevThread) - virThreadJoin(priv->udevThread);
+ if (priv->mdevctlTimeout != -1) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + if (priv->watch) { + virEventRemoveHandle(priv->watch); + priv->watch = -1; + }
Too many rebases… the diff should read as follows: @@ -2278,11 +2380,12 @@ nodeStateShutdownWait(void) return 0; VIR_WITH_OBJECT_LOCK_GUARD(priv) { - if (priv->initThread) - virThreadJoin(priv->initThread); if (priv->udevThread) virThreadJoin(priv->udevThread); } + + if (priv->workerPool) + virThreadPoolDrain(priv->workerPool); return 0; } […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 4/19/24 11:04 AM, Marc Hartmayer wrote:
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
Use a worker pool for processing the events (e.g. udev, mdevctl config changes) and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and emitting the libvirt nodedev events.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 244 +++++++++++++++++++++-------- 1 file changed, 179 insertions(+), 65 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e4b1532dc385..67a8b5cd7132 100644
[…snip…]
}
@@ -2278,11 +2380,19 @@ nodeStateShutdownWait(void) return 0;
VIR_WITH_OBJECT_LOCK_GUARD(priv) { - if (priv->initThread) - virThreadJoin(priv->initThread); - if (priv->udevThread) - virThreadJoin(priv->udevThread);
+ if (priv->mdevctlTimeout != -1) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + if (priv->watch) { + virEventRemoveHandle(priv->watch); + priv->watch = -1; + }
Too many rebases… the diff should read as follows:
@@ -2278,11 +2380,12 @@ nodeStateShutdownWait(void) return 0;
VIR_WITH_OBJECT_LOCK_GUARD(priv) { - if (priv->initThread) - virThreadJoin(priv->initThread); if (priv->udevThread) virThreadJoin(priv->udevThread); } + + if (priv->workerPool) + virThreadPoolDrain(priv->workerPool); return 0; }
[…snip]
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
Use a worker pool for processing the events (e.g. udev, mdevctl config changes) and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and emitting the libvirt nodedev events.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> ---
[…snip…]
+ /* must be initialized before trying to reconnect to all the running mdevs + * since there might occur some mdevctl monitor events that will be + * dispatched to the worker pool */ + priv->workerPool = virThreadPoolNewFull(1, 1, 0, nodeDeviceEventHandler,
The more I think about the number of workers in this pool, the more I'm convinced that it's (currently) important to use only _one_ worker (1 udev thread <-> 1 worker), because otherwise we don't have any guarantees that we comply to the following: order(udev_events) == order(libvirt nodedev events) And I guess we would like to fulfill this guarantee. If you agree, then we should add a comment to the code and if needed we can implement something for the case #count > 1. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Apr 22, 2024 at 02:45:52PM +0200, Marc Hartmayer wrote:
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
Use a worker pool for processing the events (e.g. udev, mdevctl config changes) and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and emitting the libvirt nodedev events.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> ---
[…snip…]
+ /* must be initialized before trying to reconnect to all the running mdevs + * since there might occur some mdevctl monitor events that will be + * dispatched to the worker pool */ + priv->workerPool = virThreadPoolNewFull(1, 1, 0, nodeDeviceEventHandler,
The more I think about the number of workers in this pool, the more I'm convinced that it's (currently) important to use only _one_ worker (1 udev thread <-> 1 worker), because otherwise we don't have any guarantees that we comply to the following:
order(udev_events) == order(libvirt nodedev events)
And I guess we would like to fulfill this guarantee.
Yes, we need to preserve that ordering, otherwise the events become largely unusable. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
With the too-many-rebase-add-change and the additional comment explaining why currently only one worker must be used Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
Use a worker pool for processing the events (e.g. udev, mdevctl config changes) and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and emitting the libvirt nodedev events.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 244 +++++++++++++++++++++-------- 1 file changed, 179 insertions(+), 65 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e4b1532dc385..67a8b5cd7132 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -43,6 +43,7 @@ #include "virnetdev.h" #include "virmdev.h" #include "virutil.h" +#include "virthreadpool.h"
#include "configmake.h"
@@ -69,13 +70,13 @@ struct _udevEventData { bool udevThreadQuit; bool udevDataReady;
- /* init thread */ - virThread *initThread; - /* Protects @mdevctlMonitors */ virMutex mdevctlLock; GList *mdevctlMonitors; int mdevctlTimeout; + + /* Immutable pointer, self-locking APIs */ + virThreadPool *workerPool; };
static virClass *udevEventDataClass; @@ -86,8 +87,6 @@ udevEventDataDispose(void *obj) struct udev *udev = NULL; udevEventData *priv = obj;
- g_clear_pointer(&priv->initThread, g_free); - VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { g_list_free_full(g_steal_pointer(&priv->mdevctlMonitors), g_object_unref); } @@ -100,6 +99,8 @@ udevEventDataDispose(void *obj) udev_unref(udev); }
+ g_clear_pointer(&priv->workerPool, virThreadPoolFree); + virMutexDestroy(&priv->mdevctlLock);
virCondDestroy(&priv->udevThreadCond); @@ -143,6 +144,66 @@ udevEventDataNew(void) return ret; }
+typedef enum { + NODE_DEVICE_EVENT_INIT = 0, + NODE_DEVICE_EVENT_UDEV_ADD, + NODE_DEVICE_EVENT_UDEV_REMOVE, + NODE_DEVICE_EVENT_UDEV_CHANGE, + NODE_DEVICE_EVENT_UDEV_MOVE, + NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED, + + NODE_DEVICE_EVENT_LAST +} nodeDeviceEventType; + +struct _nodeDeviceEvent { + nodeDeviceEventType eventType; + void *data; + virFreeCallback dataFreeFunc; +}; +typedef struct _nodeDeviceEvent nodeDeviceEvent; + +static void +nodeDeviceEventFree(nodeDeviceEvent *event) +{ + if (!event) + return; + + if (event->dataFreeFunc) + event->dataFreeFunc(event->data); + g_free(event); +} +G_DEFINE_AUTOPTR_CLEANUP_FUNC(nodeDeviceEvent, nodeDeviceEventFree); + + /** + * nodeDeviceEventSubmit: + * @eventType: the event to be processed + * @data: additional data for the event processor (the pointer is stolen and it + * will be properly freed using @dataFreeFunc) + * @dataFreeFunc: callback to free @data + * + * Submits @eventType to be processed by the asynchronous event handling + * thread. + */ +static int nodeDeviceEventSubmit(nodeDeviceEventType eventType, void *data, virFreeCallback dataFreeFunc) +{ + nodeDeviceEvent *event = g_new0(nodeDeviceEvent, 1); + udevEventData *priv = NULL; + + if (!driver) + return -1; + + priv = driver->privateData; + + event->eventType = eventType; + event->data = data; + event->dataFreeFunc = dataFreeFunc; + if (virThreadPoolSendJob(priv->workerPool, 0, event) < 0) { + nodeDeviceEventFree(event); + return -1; + } + return 0; +} +
static bool udevHasDeviceProperty(struct udev_device *dev, @@ -1447,7 +1508,7 @@ static void scheduleMdevctlUpdate(udevEventData *data, bool force);
static int -udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state, const char *path) +processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char *path) { virNodeDeviceObj *obj = NULL; virNodeDeviceDef *def; @@ -1529,7 +1590,7 @@ udevSetParent(virNodeDeviceDriverState *driver_state, struct udev_device *device }
static int -udevAddOneDevice(virNodeDeviceDriverState *driver_state, struct udev_device *device) +processNodeDeviceAddAndChangeEvent(virNodeDeviceDriverState *driver_state, struct udev_device *device) { g_autofree char *sysfs_path = NULL; virNodeDeviceDef *def = NULL; @@ -1647,7 +1708,7 @@ udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, struct udev * device = udev_device_new_from_syspath(udev, name);
if (device != NULL) { - if (udevAddOneDevice(driver_state, device) != 0) { + if (processNodeDeviceAddAndChangeEvent(driver_state, device) != 0) { VIR_DEBUG("Failed to create node device for udev device '%s'", name); } @@ -1755,26 +1816,23 @@ udevHandleOneDevice(struct udev_device *device)
VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));
- if (STREQ(action, "add") || STREQ(action, "change")) - return udevAddOneDevice(driver, device); - - if (STREQ(action, "remove")) { - const char *path = udev_device_get_syspath(device); - - return udevRemoveOneDeviceSysPath(driver, path); - } - - if (STREQ(action, "move")) { - const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); - - if (devpath_old) { - g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); - - udevRemoveOneDeviceSysPath(driver, devpath_old_fixed); - } - - return udevAddOneDevice(driver, device); + /* Reference is either released via workerpool logic or at the end of this + * function. */ + device = udev_device_ref(device); + if (STREQ(action, "add")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_ADD, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "change")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_CHANGE, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "remove")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_REMOVE, device, + (virFreeCallback)udev_device_unref); + } else if (STREQ(action, "move")) { + return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UDEV_MOVE, device, + (virFreeCallback)udev_device_unref); } + udev_device_unref(device);
return 0; } @@ -1993,23 +2051,23 @@ udevSetupSystemDev(void)
static void -nodeStateInitializeEnumerate(void *opaque) +processNodeStateInitializeEnumerate(virNodeDeviceDriverState *driver_state, void *opaque) { - udevEventData *priv = driver->privateData; + udevEventData *priv = driver_state->privateData; struct udev *udev = opaque;
/* Populate with known devices */ - if (udevEnumerateDevices(driver, udev) != 0) + if (udevEnumerateDevices(driver_state, udev) != 0) goto error; /* Load persistent mdevs (which might not be activated yet) and additional * information about active mediated devices from mdevctl */ - if (nodeDeviceUpdateMediatedDevices(driver) != 0) + if (nodeDeviceUpdateMediatedDevices(driver_state) != 0) goto error;
cleanup: - VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { - driver->initialized = true; - virCondBroadcast(&driver->initCond); + VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) { + driver_state->initialized = true; + virCondBroadcast(&driver_state->initCond); }
return; @@ -2051,31 +2109,16 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
static void -mdevctlUpdateThreadFunc(void *opaque) -{ - virNodeDeviceDriverState *driver_state = opaque; - - if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) - VIR_WARN("mdevctl failed to update mediated devices"); -} - - -static void -launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque) +submitMdevctlUpdate(int timer G_GNUC_UNUSED, void *opaque) { udevEventData *priv = opaque; - virThread thread;
if (priv->mdevctlTimeout != -1) { virEventRemoveTimeout(priv->mdevctlTimeout); priv->mdevctlTimeout = -1; }
- if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc, - "mdevctl-thread", false, driver) < 0) { - virReportSystemError(errno, "%s", - _("failed to create mdevctl thread")); - } + nodeDeviceEventSubmit(NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED, NULL, NULL); }
@@ -2170,7 +2213,7 @@ 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. */ + * the worker job is submitted immediately. */ static void scheduleMdevctlUpdate(udevEventData *data, bool force) @@ -2178,12 +2221,12 @@ scheduleMdevctlUpdate(udevEventData *data, if (!force) { if (data->mdevctlTimeout != -1) virEventRemoveTimeout(data->mdevctlTimeout); - data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread, + data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, data, NULL); return; }
- launchMdevctlUpdateThread(-1, data); + submitMdevctlUpdate(-1, data); }
@@ -2223,6 +2266,62 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, }
+static void nodeDeviceEventHandler(void *data, void *opaque) +{ + virNodeDeviceDriverState *driver_state = opaque; + g_autoptr(nodeDeviceEvent) processEvent = data; + + switch (processEvent->eventType) { + case NODE_DEVICE_EVENT_INIT: + { + struct udev *udev = processEvent->data; + + processNodeStateInitializeEnumerate(driver_state, udev); + } + break; + case NODE_DEVICE_EVENT_UDEV_ADD: + case NODE_DEVICE_EVENT_UDEV_CHANGE: + { + struct udev_device *device = processEvent->data; + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_UDEV_REMOVE: + { + struct udev_device *device = processEvent->data; + const char *path = udev_device_get_syspath(device); + + processNodeDeviceRemoveEvent(driver_state, path); + } + break; + case NODE_DEVICE_EVENT_UDEV_MOVE: + { + struct udev_device *device = processEvent->data; + const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); + + if (devpath_old) { + g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); + + processNodeDeviceRemoveEvent(driver_state, devpath_old_fixed); + } + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED: + { + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) + VIR_WARN("mdevctl failed to update mediated devices"); + } + break; + case NODE_DEVICE_EVENT_LAST: + g_assert_not_reached(); + break; + } +} + + /* Note: It must be safe to call this function even if the driver was not * successfully initialized. This must be considered when changing this * function. */ @@ -2258,6 +2357,9 @@ nodeStateShutdownPrepare(void) priv->udevThreadQuit = true; virCondSignal(&priv->udevThreadCond); } + + if (priv->workerPool) + virThreadPoolStop(priv->workerPool); return 0; }
@@ -2278,11 +2380,19 @@ nodeStateShutdownWait(void) return 0;
VIR_WITH_OBJECT_LOCK_GUARD(priv) { - if (priv->initThread) - virThreadJoin(priv->initThread); - if (priv->udevThread) - virThreadJoin(priv->udevThread); + if (priv->mdevctlTimeout != -1) { + virEventRemoveTimeout(priv->mdevctlTimeout); + priv->mdevctlTimeout = -1; + } + + if (priv->watch) { + virEventRemoveHandle(priv->watch); + priv->watch = -1; + } } + + if (priv->workerPool) + virThreadPoolDrain(priv->workerPool); return 0; }
@@ -2353,6 +2463,16 @@ nodeStateInitialize(bool privileged, driver->parserCallbacks.postParse = nodeDeviceDefPostParse; driver->parserCallbacks.validate = nodeDeviceDefValidate;
+ /* must be initialized before trying to reconnect to all the running mdevs + * since there might occur some mdevctl monitor events that will be + * dispatched to the worker pool */
Add the important information that the current implementation supports the use of one worker only to ensure the order of udev and libvirt events remains the same.
+ priv->workerPool = virThreadPoolNewFull(1, 1, 0, nodeDeviceEventHandler, + "nodev-device-event", + NULL, + driver); + if (!priv->workerPool) + goto unlock; + if (udevPCITranslateInit(privileged) < 0) goto unlock;
@@ -2410,13 +2530,7 @@ nodeStateInitialize(bool privileged, if (udevSetupSystemDev() != 0) goto cleanup;
- priv->initThread = g_new0(virThread, 1); - if (virThreadCreateFull(priv->initThread, true, nodeStateInitializeEnumerate, - "nodedev-init", false, udev) < 0) { - virReportSystemError(errno, "%s", - _("failed to create udev enumerate thread")); - goto cleanup; - } + nodeDeviceEventSubmit(NODE_DEVICE_EVENT_INIT, udev_ref(udev), (virFreeCallback)udev_unref);
return VIR_DRV_STATE_INIT_COMPLETE;
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
Use a worker pool for processing the events (e.g. udev, mdevctl config changes) and the initialization instead of a separate initThread and a mdevctl-thread. This has the large advantage that we can leverage the job API and now this thread pool is responsible to do all the "costly-work" and emitting the libvirt nodedev events.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> ---
[…snip…]
+static void nodeDeviceEventHandler(void *data, void *opaque) +{ + virNodeDeviceDriverState *driver_state = opaque; + g_autoptr(nodeDeviceEvent) processEvent = data; + + switch (processEvent->eventType) { + case NODE_DEVICE_EVENT_INIT: + { + struct udev *udev = processEvent->data; + + processNodeStateInitializeEnumerate(driver_state, udev); + } + break; + case NODE_DEVICE_EVENT_UDEV_ADD: + case NODE_DEVICE_EVENT_UDEV_CHANGE: + { + struct udev_device *device = processEvent->data; + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_UDEV_REMOVE: + { + struct udev_device *device = processEvent->data; + const char *path = udev_device_get_syspath(device); + + processNodeDeviceRemoveEvent(driver_state, path); + } + break; + case NODE_DEVICE_EVENT_UDEV_MOVE: + { + struct udev_device *device = processEvent->data; + const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD"); + + if (devpath_old) { + g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old); + + processNodeDeviceRemoveEvent(driver_state, devpath_old_fixed); + } + + processNodeDeviceAddAndChangeEvent(driver_state, device); + } + break; + case NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED: + { + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) + VIR_WARN("mdevctl failed to update mediated devices"); + } + break; + case NODE_DEVICE_EVENT_LAST:
+ g_assert_not_reached();
The assert statement should be replaced with: virReportEnumRangeError(nodeDeviceEventType, processEvent->eventType);
+ break; + } +}
[…snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
When an udev event occurs the mdev active config data requires an update via mdevctl as the udev does not contain all config data. This update needs to occur immediate and to be finished before the libvirt nodedev event is issued to keep the API usage reliable. The only case where a direct `nodeDeviceUpdateMediatedDevices` is not wished is `mdevctlEventHandleCallback` - see commit 2c57b28191b9 ("nodedev: Refresh mdev devices when changes are detected") for details, but for this case there are no nodedev events created so the problem described above does not exist. `udevAddOneDevice` and `udevRemoveOneDeviceSysPath` are only called by the worker pool threads therefore it's possible to call the `nodeDeviceUpdateMediatedDevices` directly without blocking the udev thread. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 67a8b5cd7132..25571ebf708f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1504,9 +1504,6 @@ udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, struct udev_device } -static void scheduleMdevctlUpdate(udevEventData *data, bool force); - - static int processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char *path) { @@ -1540,9 +1537,8 @@ processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char virNodeDeviceObjEndAPI(&obj); /* cannot check for mdev_types since they have already been removed */ - VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver_state->privateData, false); - } + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) + VIR_WARN("mdevctl failed to update mediated devices"); virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); return 0; @@ -1666,16 +1662,10 @@ processNodeDeviceAddAndChangeEvent(virNodeDeviceDriverState *driver_state, struc has_mdev_types = virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_MDEV_TYPES); virNodeDeviceObjEndAPI(&obj); - if (has_mdev_types) { - VIR_WITH_OBJECT_LOCK_GUARD(driver_state->privateData) { - scheduleMdevctlUpdate(driver_state->privateData, false); - } - } - /* The added mdev needs an immediate active config update before the event * is issued so that full device information is available at the time that * the 'created' event is emitted. */ - if (is_mdev && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { + if ((has_mdev_types || is_mdev) && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { VIR_WARN("Update of mediated device %s failed", NULLSTR_EMPTY(sysfs_path)); } -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
When an udev event occurs the mdev active config data requires an update via mdevctl as the udev does not contain all config data. This update needs to occur immediate and to be finished before the libvirt nodedev event is issued to keep the API usage reliable.
This commit message is almost the same as patch 3 (except the 'immediate' was not changed to 'immediately' ;) This patch is just handling the 'remove' and 'adding a parent device that supports mdevs' use cases where patch 3 handled the 'add a new mdev device' case. Should we just squash patch 3 with this patch and handle all cases at the same time?
The only case where a direct `nodeDeviceUpdateMediatedDevices` is not wished is `mdevctlEventHandleCallback` - see commit 2c57b28191b9 ("nodedev: Refresh mdev devices when changes are detected") for details, but for this case there are no nodedev events created so the problem described above does not exist.
`udevAddOneDevice` and `udevRemoveOneDeviceSysPath` are only called by the worker pool threads therefore it's possible to call the `nodeDeviceUpdateMediatedDevices` directly without blocking the udev thread.
These function names no longer exist as of the previous patch, so we should probably update the commit log. I should also note that there is a comment in node_device_driver.c that refers to the udevAddOneDevice() function which is now also out of date.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 67a8b5cd7132..25571ebf708f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1504,9 +1504,6 @@ udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, struct udev_device }
-static void scheduleMdevctlUpdate(udevEventData *data, bool force); - - static int processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char *path) { @@ -1540,9 +1537,8 @@ processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char virNodeDeviceObjEndAPI(&obj);
/* cannot check for mdev_types since they have already been removed */ - VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver_state->privateData, false); - } + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) + VIR_WARN("mdevctl failed to update mediated devices");
virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); return 0; @@ -1666,16 +1662,10 @@ processNodeDeviceAddAndChangeEvent(virNodeDeviceDriverState *driver_state, struc has_mdev_types = virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_MDEV_TYPES); virNodeDeviceObjEndAPI(&obj);
- if (has_mdev_types) { - VIR_WITH_OBJECT_LOCK_GUARD(driver_state->privateData) { - scheduleMdevctlUpdate(driver_state->privateData, false); - } - } - /* The added mdev needs an immediate active config update before the event * is issued so that full device information is available at the time that * the 'created' event is emitted. */ - if (is_mdev && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { + if ((has_mdev_types || is_mdev) && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { VIR_WARN("Update of mediated device %s failed", NULLSTR_EMPTY(sysfs_path)); }
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
This could be quashed with patch 3 but I am also fine with this if you do not want to spend the effort. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
When an udev event occurs the mdev active config data requires an update via mdevctl as the udev does not contain all config data. This update needs to occur immediate and to be finished before the libvirt nodedev event is issued to keep the API usage reliable.
The only case where a direct `nodeDeviceUpdateMediatedDevices` is not wished is `mdevctlEventHandleCallback` - see commit 2c57b28191b9 ("nodedev: Refresh mdev devices when changes are detected") for details, but for this case there are no nodedev events created so the problem described above does not exist.
`udevAddOneDevice` and `udevRemoveOneDeviceSysPath` are only called by the worker pool threads therefore it's possible to call the `nodeDeviceUpdateMediatedDevices` directly without blocking the udev thread.
Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Apr 22, 2024 at 05:43 PM +0200, Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
This could be quashed with patch 3 but I am also fine with this if you do not want to spend the effort.
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Will squash it. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
There is only one case where force is true, therefore let's inline that case. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 25571ebf708f..0b255952a9f9 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2202,21 +2202,14 @@ 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, - * the worker job is submitted immediately. */ + * quick succession can be collapsed into a single update. */ static void -scheduleMdevctlUpdate(udevEventData *data, - bool force) +scheduleMdevctlUpdate(udevEventData *data) { - if (!force) { - if (data->mdevctlTimeout != -1) - virEventRemoveTimeout(data->mdevctlTimeout); - data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, - data, NULL); - return; - } - - submitMdevctlUpdate(-1, data); + if (data->mdevctlTimeout != -1) + virEventRemoveTimeout(data->mdevctlTimeout); + data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, + data, NULL); } @@ -2251,7 +2244,11 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the * signal if that event never comes */ VIR_WITH_OBJECT_LOCK_GUARD(priv) { - scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + if (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) { + submitMdevctlUpdate(-1, priv); + } else { + scheduleMdevctlUpdate(priv); + } } } -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
There is only one case where force is true, therefore let's inline that case.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 25571ebf708f..0b255952a9f9 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2202,21 +2202,14 @@ 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, - * the worker job is submitted immediately. */ + * quick succession can be collapsed into a single update. */ static void -scheduleMdevctlUpdate(udevEventData *data, - bool force) +scheduleMdevctlUpdate(udevEventData *data) { - if (!force) { - if (data->mdevctlTimeout != -1) - virEventRemoveTimeout(data->mdevctlTimeout); - data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, - data, NULL); - return; - } - - submitMdevctlUpdate(-1, data); + if (data->mdevctlTimeout != -1) + virEventRemoveTimeout(data->mdevctlTimeout); + data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, + data, NULL); }
@@ -2251,7 +2244,11 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the * signal if that event never comes */ VIR_WITH_OBJECT_LOCK_GUARD(priv) { - scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + if (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) { + submitMdevctlUpdate(-1, priv); + } else { + scheduleMdevctlUpdate(priv); + } } }
I don't mind either way Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
There is only one case where force is true, therefore let's inline that case.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 25571ebf708f..0b255952a9f9 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2202,21 +2202,14 @@ 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, - * the worker job is submitted immediately. */ + * quick succession can be collapsed into a single update. */ static void -scheduleMdevctlUpdate(udevEventData *data, - bool force) +scheduleMdevctlUpdate(udevEventData *data) { - if (!force) { - if (data->mdevctlTimeout != -1) - virEventRemoveTimeout(data->mdevctlTimeout); - data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, - data, NULL); - return; - } - - submitMdevctlUpdate(-1, data); + if (data->mdevctlTimeout != -1) + virEventRemoveTimeout(data->mdevctlTimeout); + data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, + data, NULL); }
@@ -2251,7 +2244,11 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the * signal if that event never comes */ VIR_WITH_OBJECT_LOCK_GUARD(priv) { - scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + if (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) { + submitMdevctlUpdate(-1, priv); + } else { + scheduleMdevctlUpdate(priv); + } } }
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Use this feature in `udevEventDataNew`. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 0b255952a9f9..cc8700fc4117 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -78,6 +78,7 @@ struct _udevEventData { /* Immutable pointer, self-locking APIs */ virThreadPool *workerPool; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(udevEventData, virObjectUnref); static virClass *udevEventDataClass; @@ -121,7 +122,7 @@ VIR_ONCE_GLOBAL_INIT(udevEventData); static udevEventData * udevEventDataNew(void) { - udevEventData *ret = NULL; + g_autoptr(udevEventData) ret = NULL; if (udevEventDataInitialize() < 0) return NULL; @@ -129,19 +130,15 @@ udevEventDataNew(void) if (!(ret = virObjectLockableNew(udevEventDataClass))) return NULL; - if (virCondInit(&ret->udevThreadCond) < 0) { - virObjectUnref(ret); + if (virCondInit(&ret->udevThreadCond) < 0) return NULL; - } - if (virMutexInit(&ret->mdevctlLock) < 0) { - virObjectUnref(ret); + if (virMutexInit(&ret->mdevctlLock) < 0) return NULL; - } ret->mdevctlTimeout = -1; ret->watch = -1; - return ret; + return g_steal_pointer(&ret); } typedef enum { -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
Use this feature in `udevEventDataNew`.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 0b255952a9f9..cc8700fc4117 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -78,6 +78,7 @@ struct _udevEventData { /* Immutable pointer, self-locking APIs */ virThreadPool *workerPool; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(udevEventData, virObjectUnref);
static virClass *udevEventDataClass;
@@ -121,7 +122,7 @@ VIR_ONCE_GLOBAL_INIT(udevEventData); static udevEventData * udevEventDataNew(void) { - udevEventData *ret = NULL; + g_autoptr(udevEventData) ret = NULL;
if (udevEventDataInitialize() < 0) return NULL; @@ -129,19 +130,15 @@ udevEventDataNew(void) if (!(ret = virObjectLockableNew(udevEventDataClass))) return NULL;
- if (virCondInit(&ret->udevThreadCond) < 0) { - virObjectUnref(ret); + if (virCondInit(&ret->udevThreadCond) < 0) return NULL; - }
- if (virMutexInit(&ret->mdevctlLock) < 0) { - virObjectUnref(ret); + if (virMutexInit(&ret->mdevctlLock) < 0) return NULL; - }
ret->mdevctlTimeout = -1; ret->watch = -1; - return ret; + return g_steal_pointer(&ret); }
typedef enum {
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
Use this feature in `udevEventDataNew`.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 0b255952a9f9..cc8700fc4117 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -78,6 +78,7 @@ struct _udevEventData { /* Immutable pointer, self-locking APIs */ virThreadPool *workerPool; }; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(udevEventData, virObjectUnref);
static virClass *udevEventDataClass;
@@ -121,7 +122,7 @@ VIR_ONCE_GLOBAL_INIT(udevEventData); static udevEventData * udevEventDataNew(void) { - udevEventData *ret = NULL; + g_autoptr(udevEventData) ret = NULL;
if (udevEventDataInitialize() < 0) return NULL; @@ -129,19 +130,15 @@ udevEventDataNew(void) if (!(ret = virObjectLockableNew(udevEventDataClass))) return NULL;
- if (virCondInit(&ret->udevThreadCond) < 0) { - virObjectUnref(ret); + if (virCondInit(&ret->udevThreadCond) < 0) return NULL; - }
- if (virMutexInit(&ret->mdevctlLock) < 0) { - virObjectUnref(ret); + if (virMutexInit(&ret->mdevctlLock) < 0) return NULL; - }
ret->mdevctlTimeout = -1; ret->watch = -1; - return ret; + return g_steal_pointer(&ret); }
typedef enum {
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Instead of accessing the global `driver` object pass the `udevEventData` as parameter to the thread handler and watch callback. This has the advantage that: 1. proper refcounting 2. easier to read and test Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index cc8700fc4117..07cbdfa3bdeb 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1856,7 +1856,7 @@ udevEventMonitorSanityCheck(udevEventData *priv, /** * udevEventHandleThread - * @opaque: unused + * @opaque: udevEventData * * Thread to handle the udevEventHandleCallback processing when udev * tells us there's a device change for us (add, modify, delete, etc). @@ -1875,9 +1875,9 @@ udevEventMonitorSanityCheck(udevEventData *priv, * would still come into play. */ static void -udevEventHandleThread(void *opaque G_GNUC_UNUSED) +udevEventHandleThread(void *opaque) { - udevEventData *priv = driver->privateData; + g_autoptr(udevEventData) priv = opaque; struct udev_device *device = NULL; /* continue rather than break from the loop on non-fatal errors */ @@ -1943,9 +1943,9 @@ static void udevEventHandleCallback(int watch G_GNUC_UNUSED, int fd, int events G_GNUC_UNUSED, - void *data G_GNUC_UNUSED) + void *data) { - udevEventData *priv = driver->privateData; + udevEventData *priv = data; VIR_LOCK_GUARD lock = virObjectLockGuard(priv); if (!udevEventMonitorSanityCheck(priv, fd)) @@ -2485,7 +2485,7 @@ nodeStateInitialize(bool privileged, priv->udevThread = g_new0(virThread, 1); if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread, - "udev-event", false, NULL) < 0) { + "udev-event", false, virObjectRef(priv)) < 0) { virReportSystemError(errno, "%s", _("failed to create udev handler thread")); goto unlock; @@ -2501,7 +2501,7 @@ nodeStateInitialize(bool privileged, * that appear while the enumeration is taking place. */ priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor), VIR_EVENT_HANDLE_READABLE, - udevEventHandleCallback, NULL, NULL); + udevEventHandleCallback, virObjectRef(priv), virObjectUnref); if (priv->watch == -1) goto unlock; -- 2.34.1
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
Instead of accessing the global `driver` object pass the `udevEventData` as parameter to the thread handler and watch callback. This has the advantage that: 1. proper refcounting 2. easier to read and test
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index cc8700fc4117..07cbdfa3bdeb 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1856,7 +1856,7 @@ udevEventMonitorSanityCheck(udevEventData *priv,
/** * udevEventHandleThread - * @opaque: unused + * @opaque: udevEventData * * Thread to handle the udevEventHandleCallback processing when udev * tells us there's a device change for us (add, modify, delete, etc). @@ -1875,9 +1875,9 @@ udevEventMonitorSanityCheck(udevEventData *priv, * would still come into play. */ static void -udevEventHandleThread(void *opaque G_GNUC_UNUSED) +udevEventHandleThread(void *opaque) { - udevEventData *priv = driver->privateData; + g_autoptr(udevEventData) priv = opaque; struct udev_device *device = NULL;
/* continue rather than break from the loop on non-fatal errors */ @@ -1943,9 +1943,9 @@ static void udevEventHandleCallback(int watch G_GNUC_UNUSED, int fd, int events G_GNUC_UNUSED, - void *data G_GNUC_UNUSED) + void *data) { - udevEventData *priv = driver->privateData; + udevEventData *priv = data; VIR_LOCK_GUARD lock = virObjectLockGuard(priv);
if (!udevEventMonitorSanityCheck(priv, fd)) @@ -2485,7 +2485,7 @@ nodeStateInitialize(bool privileged,
priv->udevThread = g_new0(virThread, 1); if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread, - "udev-event", false, NULL) < 0) { + "udev-event", false, virObjectRef(priv)) < 0) { virReportSystemError(errno, "%s", _("failed to create udev handler thread")); goto unlock; @@ -2501,7 +2501,7 @@ nodeStateInitialize(bool privileged, * that appear while the enumeration is taking place. */ priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor), VIR_EVENT_HANDLE_READABLE, - udevEventHandleCallback, NULL, NULL); + udevEventHandleCallback, virObjectRef(priv), virObjectUnref); if (priv->watch == -1) goto unlock;
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/19/24 16:49, Marc Hartmayer wrote:
Instead of accessing the global `driver` object pass the `udevEventData` as parameter to the thread handler and watch callback. This has the advantage that: 1. proper refcounting 2. easier to read and test
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index cc8700fc4117..07cbdfa3bdeb 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1856,7 +1856,7 @@ udevEventMonitorSanityCheck(udevEventData *priv,
/** * udevEventHandleThread - * @opaque: unused + * @opaque: udevEventData * * Thread to handle the udevEventHandleCallback processing when udev * tells us there's a device change for us (add, modify, delete, etc). @@ -1875,9 +1875,9 @@ udevEventMonitorSanityCheck(udevEventData *priv, * would still come into play. */ static void -udevEventHandleThread(void *opaque G_GNUC_UNUSED) +udevEventHandleThread(void *opaque) { - udevEventData *priv = driver->privateData; + g_autoptr(udevEventData) priv = opaque; struct udev_device *device = NULL;
/* continue rather than break from the loop on non-fatal errors */ @@ -1943,9 +1943,9 @@ static void udevEventHandleCallback(int watch G_GNUC_UNUSED, int fd, int events G_GNUC_UNUSED, - void *data G_GNUC_UNUSED) + void *data) { - udevEventData *priv = driver->privateData; + udevEventData *priv = data; VIR_LOCK_GUARD lock = virObjectLockGuard(priv);
if (!udevEventMonitorSanityCheck(priv, fd)) @@ -2485,7 +2485,7 @@ nodeStateInitialize(bool privileged,
priv->udevThread = g_new0(virThread, 1); if (virThreadCreateFull(priv->udevThread, true, udevEventHandleThread, - "udev-event", false, NULL) < 0) { + "udev-event", false, virObjectRef(priv)) < 0) { virReportSystemError(errno, "%s", _("failed to create udev handler thread")); goto unlock; @@ -2501,7 +2501,7 @@ nodeStateInitialize(bool privileged, * that appear while the enumeration is taking place. */ priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor), VIR_EVENT_HANDLE_READABLE, - udevEventHandleCallback, NULL, NULL); + udevEventHandleCallback, virObjectRef(priv), virObjectUnref); if (priv->watch == -1) goto unlock;
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (5)
-
Boris Fiuczynski -
Daniel P. Berrangé -
Jim Fehlig -
Jonathon Jongsma -
Marc Hartmayer