[PATCH v2 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: v1->v2: + squashed old patches 3 and 17 (comments from Jonathon and Boris) + added r-b's from Jonathon and Boris + worked in comments from Jonathon to old patch 15 + added comment why only one worker can currently be used + added patch `node_device_udev: remove incorrect G_GNUC_UNUSED` 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 events 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: 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 node_device_udev: remove incorrect G_GNUC_UNUSED 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 | 28 +- src/node_device/node_device_udev.c | 516 ++++++++++++++++++--------- src/test/test_driver.c | 11 +- src/util/virmdev.c | 20 ++ src/libvirt_private.syms | 2 + 8 files changed, 398 insertions(+), 198 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, change or remove 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 nodedev 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`. 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 | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 6613528d0e37..e272d9f1ea2b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1440,9 +1440,6 @@ udevGetDeviceDetails(struct udev_device *device, } -static void scheduleMdevctlUpdate(udevEventData *data, bool force); - - static int udevRemoveOneDeviceSysPath(const char *path) { @@ -1475,7 +1472,8 @@ udevRemoveOneDeviceSysPath(const char *path) virNodeDeviceObjEndAPI(&obj); /* cannot check for mdev_types since they have already been removed */ - scheduleMdevctlUpdate(driver->privateData, false); + if (nodeDeviceUpdateMediatedDevices() < 0) + VIR_WARN("mdevctl failed to update mediated devices"); virObjectEventStateQueue(driver->nodeDeviceEventState, event); return 0; @@ -1535,6 +1533,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 +1548,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); @@ -1605,8 +1607,13 @@ 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); + /* 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 ((has_mdev_types || is_mdev) && (nodeDeviceUpdateMediatedDevices() < 0)) { + VIR_WARN("Update of mediated device %s failed", + NULLSTR_EMPTY(sysfs_path)); + } ret = 0; @@ -1758,19 +1765,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 e272d9f1ea2b..921f7806afbd 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1464,6 +1464,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 921f7806afbd..945cfea0851e 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 945cfea0851e..616f98885a60 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; } @@ -2082,7 +2083,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; } @@ -2192,7 +2193,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. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 616f98885a60..c2e6c593709b 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; }; @@ -2069,9 +2070,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
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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index c2e6c593709b..ee96a8a6c92b 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2232,7 +2232,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 ee96a8a6c92b..5be09aeb23d6 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; } @@ -1737,16 +1737,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); } } @@ -1855,15 +1855,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; @@ -1894,7 +1894,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; @@ -1921,11 +1921,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); } @@ -2035,8 +2035,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; @@ -2330,12 +2330,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 5be09aeb23d6..0ad83efc86a8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1485,16 +1485,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) @@ -1778,8 +1768,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. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 0ad83efc86a8..9aab4340fa2d 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; @@ -1730,14 +1734,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); @@ -2328,7 +2328,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; } @@ -2360,7 +2359,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
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`. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 9aab4340fa2d..906b16778eef 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
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. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 906b16778eef..b2c5d11acbdf 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) { @@ -1723,24 +1717,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); @@ -2231,6 +2211,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, @@ -2365,6 +2403,8 @@ nodeStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_COMPLETE; cleanup: + nodeStateShutdownPrepare(); + nodeStateShutdownWait(); nodeStateCleanup(); return VIR_DRV_STATE_INIT_ERROR; @@ -2430,6 +2470,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
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... Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 b2c5d11acbdf..2c9c7466da4a 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2226,6 +2226,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
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. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 | 73 ++++++++++++++++------------ 3 files changed, 45 insertions(+), 36 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 2c9c7466da4a..4f8dae3f85c8 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -361,7 +361,8 @@ 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 +373,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 +1392,13 @@ 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: @@ -1444,13 +1446,14 @@ udevGetDeviceDetails(struct udev_device *device, 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; @@ -1471,20 +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 */ - if (nodeDeviceUpdateMediatedDevices() < 0) + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) VIR_WARN("mdevctl failed to update mediated devices"); - 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; @@ -1507,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); @@ -1525,7 +1529,8 @@ 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; @@ -1556,15 +1561,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) @@ -1582,7 +1587,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; @@ -1604,7 +1609,7 @@ udevAddOneDevice(struct udev_device *device) /* 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 ((has_mdev_types || is_mdev) && (nodeDeviceUpdateMediatedDevices() < 0)) { + if ((has_mdev_types || is_mdev) && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { VIR_WARN("Update of mediated device %s failed", NULLSTR_EMPTY(sysfs_path)); } @@ -1612,7 +1617,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, @@ -1625,7 +1630,8 @@ 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; @@ -1637,7 +1643,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); } @@ -1675,7 +1681,8 @@ 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; @@ -1691,7 +1698,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; @@ -1746,12 +1753,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")) { @@ -1760,10 +1767,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; @@ -1989,11 +1996,11 @@ nodeStateInitializeEnumerate(void *opaque) udevEventData *priv = driver->privateData; /* 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: @@ -2041,9 +2048,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"); } @@ -2060,7 +2069,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
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. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_driver.c | 9 +- src/node_device/node_device_udev.c | 241 +++++++++++++++++++-------- src/test/test_driver.c | 8 +- 3 files changed, 185 insertions(+), 73 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 59c5f9b417a4..a51537d87ceb 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -1421,10 +1421,11 @@ nodeDeviceDestroy(virNodeDevicePtr device) goto cleanup; /* Because we're about to release the lock and thus run into a race - * possibility (however improbable) with a udevAddOneDevice change - * event which would essentially free the existing @def (obj->def) and - * replace it with something new, we need to grab the parent field - * and then find the parent obj in order to manage the vport */ + * possibility (however improbable) with a + * processNodeDeviceAddAndChangeEvent change event which would + * essentially free the existing @def (obj->def) and replace it with + * something new, we need to grab the parent field and then find the + * parent obj in order to manage the vport */ parent = g_strdup(def->parent); virNodeDeviceObjEndAPI(&obj); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4f8dae3f85c8..1f7123a5fafa 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, @@ -1446,8 +1507,8 @@ udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, static int -udevRemoveOneDeviceSysPath(virNodeDeviceDriverState *driver_state, - const char *path) +processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, + const char *path) { virNodeDeviceObj *obj = NULL; virNodeDeviceDef *def; @@ -1529,8 +1590,8 @@ udevSetParent(virNodeDeviceDriverState *driver_state, } 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; @@ -1643,7 +1704,7 @@ udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, 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); } @@ -1752,26 +1813,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; } @@ -1990,23 +2048,24 @@ udevSetupSystemDev(void) static void -nodeStateInitializeEnumerate(void *opaque) +processNodeStateInitializeEnumerate(virNodeDeviceDriverState *driver_state, + void *opaque) { struct udev *udev = opaque; - udevEventData *priv = driver->privateData; + udevEventData *priv = driver_state->privateData; /* 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; @@ -2048,31 +2107,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); } @@ -2167,7 +2211,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) @@ -2175,12 +2219,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); } @@ -2220,6 +2264,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. */ @@ -2255,6 +2355,9 @@ nodeStateShutdownPrepare(void) priv->udevThreadQuit = true; virCondSignal(&priv->udevThreadCond); } + + if (priv->workerPool) + virThreadPoolStop(priv->workerPool); return 0; } @@ -2275,11 +2378,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; } @@ -2350,6 +2454,19 @@ nodeStateInitialize(bool privileged, driver->parserCallbacks.postParse = nodeDeviceDefPostParse; driver->parserCallbacks.validate = nodeDeviceDefValidate; + /* With the current design, we can only have exactly *one* worker thread as + * otherwise we cannot guarantee that the 'order(udev_events) == + * order(nodedev_events)' is preserved. The worker pool 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; @@ -2407,13 +2524,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; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 81b1ba4294bd..76f89a224f21 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7750,10 +7750,10 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) if (virNodeDeviceGetWWNs(def, &wwnn, &wwpn) == -1) goto cleanup; - /* Unlike the real code we cannot run into the udevAddOneDevice race - * which would replace obj->def, so no need to save off the parent, - * but do need to drop the @obj lock so that the FindByName code doesn't - * deadlock on ourselves */ + /* Unlike the real code we cannot run into the + * processNodeDeviceAddAndChangeEvent race which would replace obj->def, so + * no need to save off the parent, but do need to drop the @obj lock so that + * the FindByName code doesn't deadlock on ourselves */ virObjectUnlock(obj); /* We do this just for basic validation and throw away the parentobj -- 2.34.1
There is only one case where force is true, therefore let's inline that case. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 1f7123a5fafa..73b71607489f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2210,21 +2210,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); } @@ -2259,7 +2252,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
Use this feature in `udevEventDataNew`. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 73b71607489f..8687be942722 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
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 Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> 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 8687be942722..14d44d5bcd0e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1863,7 +1863,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). @@ -1882,9 +1882,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 */ @@ -1950,9 +1950,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)) @@ -2489,7 +2489,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; @@ -2505,7 +2505,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
Signed-off-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 14d44d5bcd0e..cc725997a39e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -791,7 +791,7 @@ udevGetSCSIType(virNodeDeviceDef *def G_GNUC_UNUSED, static int -udevProcessSCSIDevice(struct udev_device *device G_GNUC_UNUSED, +udevProcessSCSIDevice(struct udev_device *device, virNodeDeviceDef *def) { int ret = -1; -- 2.34.1
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/23/24 20:09, Marc Hartmayer wrote:
Signed-off-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 14d44d5bcd0e..cc725997a39e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -791,7 +791,7 @@ udevGetSCSIType(virNodeDeviceDef *def G_GNUC_UNUSED,
static int -udevProcessSCSIDevice(struct udev_device *device G_GNUC_UNUSED, +udevProcessSCSIDevice(struct udev_device *device, virNodeDeviceDef *def) { int ret = -1;
-- 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
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/23/24 8:09 PM, Marc Hartmayer wrote:
Signed-off-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 14d44d5bcd0e..cc725997a39e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -791,7 +791,7 @@ udevGetSCSIType(virNodeDeviceDef *def G_GNUC_UNUSED,
static int -udevProcessSCSIDevice(struct udev_device *device G_GNUC_UNUSED, +udevProcessSCSIDevice(struct udev_device *device, virNodeDeviceDef *def) { int ret = -1;
-- 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 Tue, Apr 23, 2024 at 08:08 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
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.
[…snip…]
src/test/test_driver.c | 11 +- src/util/virmdev.c | 20 ++ src/libvirt_private.syms | 2 + 8 files changed, 398 insertions(+), 198 deletions(-)
base-commit: c38720b337f74337ec94c0fe2e97a7c2c57188ae -- 2.34.1 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
Polite ping. Should I rebase or how do we want to proceed with this series? -- 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 6/13/24 6:13 AM, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 08:08 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
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.
[…snip…]
src/test/test_driver.c | 11 +- src/util/virmdev.c | 20 ++ src/libvirt_private.syms | 2 + 8 files changed, 398 insertions(+), 198 deletions(-)
base-commit: c38720b337f74337ec94c0fe2e97a7c2c57188ae -- 2.34.1 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
Polite ping. Should I rebase or how do we want to proceed with this series?
My apologies. I thought I remembered reviewing this series and, looking back at the archives, it seems that I did. Do you just need somebody to push it upstream? Thanks, Jonathon
On 6/13/24 11:14 PM, Jonathon Jongsma wrote:
On 6/13/24 6:13 AM, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 08:08 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
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.
[…snip…]
src/test/test_driver.c | 11 +- src/util/virmdev.c | 20 ++ src/libvirt_private.syms | 2 + 8 files changed, 398 insertions(+), 198 deletions(-)
base-commit: c38720b337f74337ec94c0fe2e97a7c2c57188ae -- 2.34.1 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
Polite ping. Should I rebase or how do we want to proceed with this series?
My apologies. I thought I remembered reviewing this series and, looking back at the archives, it seems that I did. Do you just need somebody to push it upstream?
Thanks, Jonathon
Hi Jonathon, I think Marc is looking for someone to push his series. Seems like patches 2, 3 and 20 are missing your rb or did Marc forget to add them? @Marc, could you send a rebased v3 since it seems the series no longer cleanly applies on master. -- 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 6/18/24 6:33 AM, Boris Fiuczynski wrote:
On 6/13/24 11:14 PM, Jonathon Jongsma wrote:
On 6/13/24 6:13 AM, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 08:08 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
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.
[…snip…]
src/test/test_driver.c | 11 +- src/util/virmdev.c | 20 ++ src/libvirt_private.syms | 2 + 8 files changed, 398 insertions(+), 198 deletions(-)
base-commit: c38720b337f74337ec94c0fe2e97a7c2c57188ae -- 2.34.1 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
Polite ping. Should I rebase or how do we want to proceed with this series?
My apologies. I thought I remembered reviewing this series and, looking back at the archives, it seems that I did. Do you just need somebody to push it upstream?
Thanks, Jonathon
Hi Jonathon, I think Marc is looking for someone to push his series. Seems like patches 2, 3 and 20 are missing your rb or did Marc forget to add them? @Marc, could you send a rebased v3 since it seems the series no longer cleanly applies on master.
For the v2 patch series I acked the cover letter for the series. So I'll just rebase and add my r-b to all of them and push. Sorry for the delay. Jonathon
On Tue, Jun 18, 2024 at 08:59 AM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
On 6/18/24 6:33 AM, Boris Fiuczynski wrote:
On 6/13/24 11:14 PM, Jonathon Jongsma wrote:
On 6/13/24 6:13 AM, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 08:08 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
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.
[…snip…]
src/test/test_driver.c | 11 +- src/util/virmdev.c | 20 ++ src/libvirt_private.syms | 2 + 8 files changed, 398 insertions(+), 198 deletions(-)
base-commit: c38720b337f74337ec94c0fe2e97a7c2c57188ae -- 2.34.1 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
Polite ping. Should I rebase or how do we want to proceed with this series?
My apologies. I thought I remembered reviewing this series and, looking back at the archives, it seems that I did. Do you just need somebody to push it upstream?
Thanks, Jonathon
Hi Jonathon, I think Marc is looking for someone to push his series. Seems like patches 2, 3 and 20 are missing your rb or did Marc forget to add them? @Marc, could you send a rebased v3 since it seems the series no longer cleanly applies on master.
For the v2 patch series I acked the cover letter for the series. So I'll just rebase and add my r-b to all of them and push. Sorry for the delay.
Thanks and no problems.
Jonathon
-- 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
participants (3)
-
Boris Fiuczynski -
Jonathon Jongsma -
Marc Hartmayer