9:49 a.m.
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
9:49 a.m.
New subject: [PATCH v1 01/20] nodedev: fix mdev add udev event data handling
From: Boris Fiuczynski <fiuczy(a)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(a)linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 02/20] node_device_udev: Set @def to NULL
@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(a)linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 03/20] nodedev: immediate update of active config on udev add
From: Boris Fiuczynski <fiuczy(a)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(a)redhat.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 04/20] nodedev: reset active config data on udev remove event
From: Boris Fiuczynski <fiuczy(a)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(a)linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)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
9:49 a.m.
New subject: [PATCH v1 05/20] node_device_udev: Remove the timeout if the data is disposed
Remove the timeout when the udevEventData is disposed, analogous to priv->watch.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 06/20] node_device_udev: Test for mdevctlTimeout != -1
It is done a little differently everywhere in libvirt, but most common is to
test for != -1.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 07/20] node_device_udev: Don't take `mdevctlLock` for `mdevctl list` and add comments about locking
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(a)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
1:57 p.m.
New subject: [PATCH v1 07/20] node_device_udev: Don't take `mdevctlLock` for
`mdevctl list` and add comments about locking
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(a)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(a)redhat.com>
5:32 a.m.
New subject: [PATCH v1 07/20] node_device_udev: Don't take `mdevctlLock` for
`mdevctl list` and add comments about locking
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
9:49 a.m.
New subject: [PATCH v1 08/20] node_device_udev: Take lock if `driver->privateData` is modified
Since @driver->privateData is modified take the lock.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 09/20] node_device_udev: Add prefix `udev` for udev related data
The new names make it easier to understand the purpose of the data.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 10/20] node_device_udev: Inline `udevRemoveOneDevice`
Inline `udevRemoveOneDevice` as it's used only once.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 11/20] node_device_udev: Move responsibility to release `(init|udev)Thread` to `udevEventDataDispose`
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(a)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
2 p.m.
New subject: [PATCH v1 11/20] node_device_udev: Move responsibility to release
`(init|udev)Thread` to `udevEventDataDispose`
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(a)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(a)redhat.com>
5:28 a.m.
New subject: [PATCH v1 11/20] node_device_udev: Move responsibility to release
`(init|udev)Thread` to `udevEventDataDispose`
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
9:49 a.m.
New subject: [PATCH v1 12/20] node_device_udev: Fix leak of mdevctlLock, udevThreadCond, and mdevCtlMonitors
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(a)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
2 p.m.
New subject: [PATCH v1 12/20] node_device_udev: Fix leak of mdevctlLock,
udevThreadCond, and mdevCtlMonitors
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(a)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(a)redhat.com>
10:28 a.m.
New subject: [PATCH v1 12/20] node_device_udev: Fix leak of mdevctlLock,
udevThreadCond, and mdevCtlMonitors
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
9:49 a.m.
New subject: [PATCH v1 13/20] node_device_udev: Introduce and use `stateShutdownPrepare` and `stateShutdownWait`
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(a)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
2:11 p.m.
New subject: [PATCH v1 13/20] node_device_udev: Introduce and use
`stateShutdownPrepare` and `stateShutdownWait`
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(a)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(a)redhat.com>
10:29 a.m.
New subject: [PATCH v1 13/20] node_device_udev: Introduce and use
`stateShutdownPrepare` and `stateShutdownWait`
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
9:49 a.m.
New subject: [PATCH v1 14/20] node_device_udev: nodeStateShutdownPrepare: Disconnect the signals explicitly
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-han...
Signed-off-by: Marc Hartmayer <mhartmay(a)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
2:17 p.m.
New subject: [PATCH v1 14/20] node_device_udev: nodeStateShutdownPrepare:
Disconnect the signals explicitly
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-han...
Signed-off-by: Marc Hartmayer <mhartmay(a)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(a)redhat.com>
10:31 a.m.
New subject: [PATCH v1 14/20] node_device_udev: nodeStateShutdownPrepare:
Disconnect the signals explicitly
Reviewed-by: Boris Fiuczynski <fiuczy(a)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-h...
Signed-off-by: Marc Hartmayer<mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as parameter in preparation for the next commit
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(a)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
2:23 p.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as
parameter in preparation for the next commit
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(a)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(a)redhat.com>
3:03 a.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as
parameter in preparation for the next commit
On Fri, Apr 19, 2024 at 02:23 PM -0500, Jonathon Jongsma <jjongsma(a)redhat.com>
wrote:
--
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 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(a)redhat.com>
Thanks.
3:10 a.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as
parameter in preparation for the next commit
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(a)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 :|
3:46 a.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as
parameter in preparation for the next commit
On Tue, Apr 23, 2024 at 09:10 AM +0100, Daniel P. Berrangé <berrange(a)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(a)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
4:06 a.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as
parameter in preparation for the next commit
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(a)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(a)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 :|
4:41 a.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as
parameter in preparation for the next commit
On Tue, Apr 23, 2024 at 10:06 AM +0100, Daniel P. Berrangé <berrange(a)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(a)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(a)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 :)
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
5:57 p.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as
parameter in preparation for the next commit
On 4/23/24 3:41 AM, Marc Hartmayer wrote:
On Tue, Apr 23, 2024 at 10:06 AM +0100, Daniel P. Berrangé
<berrange(a)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(a)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(a)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 :|
>
10:32 a.m.
New subject: [PATCH v1 15/20] node_device_udev: Pass the driver state as
parameter in preparation for the next commit
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
9:49 a.m.
New subject: [PATCH v1 16/20] node_device_udev: Use a worker pool for processing events and emitting nodedev event
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(a)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
11:04 a.m.
New subject: [PATCH v1 16/20] node_device_udev: Use a worker pool for
processing events and emitting nodedev event
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer <mhartmay(a)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(a)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
2:39 p.m.
New subject: [PATCH v1 16/20] node_device_udev: Use a worker pool for
processing events and emitting nodedev event
On 4/19/24 11:04 AM, Marc Hartmayer wrote:
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer
<mhartmay(a)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(a)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(a)redhat.com>
7:45 a.m.
New subject: [PATCH v1 16/20] node_device_udev: Use a worker pool for
processing events and emitting nodedev event
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer <mhartmay(a)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(a)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
7:53 a.m.
New subject: [PATCH v1 16/20] node_device_udev: Use a worker pool for
processing events and emitting nodedev event
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(a)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(a)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 :|
10:40 a.m.
New subject: [PATCH v1 16/20] node_device_udev: Use a worker pool for
processing events and emitting nodedev event
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(a)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(a)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
2:10 a.m.
New subject: [PATCH v1 16/20] node_device_udev: Use a worker pool for
processing events and emitting nodedev event
On Fri, Apr 19, 2024 at 04:49 PM +0200, Marc Hartmayer <mhartmay(a)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(a)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
9:49 a.m.
New subject: [PATCH v1 17/20] node_device_udev: Call `nodeDeviceUpdateMediatedDevices` directly
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(a)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
2:54 p.m.
New subject: [PATCH v1 17/20] node_device_udev: Call
`nodeDeviceUpdateMediatedDevices` directly
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(a)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(a)redhat.com>
10:43 a.m.
New subject: [PATCH v1 17/20] node_device_udev: Call
`nodeDeviceUpdateMediatedDevices` directly
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(a)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(a)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
3:16 a.m.
New subject: [PATCH v1 17/20] node_device_udev: Call
`nodeDeviceUpdateMediatedDevices` directly
On Mon, Apr 22, 2024 at 05:43 PM +0200, Boris Fiuczynski <fiuczy(a)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(a)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
9:49 a.m.
New subject: [PATCH v1 18/20] node_device_udev: Make the code easier to read
There is only one case where force is true, therefore let's inline that case.
Signed-off-by: Marc Hartmayer <mhartmay(a)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
2:56 p.m.
New subject: [PATCH v1 18/20] node_device_udev: Make the code easier to read
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(a)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(a)redhat.com>
10:44 a.m.
New subject: [PATCH v1 18/20] node_device_udev: Make the code easier to read
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
9:49 a.m.
New subject: [PATCH v1 19/20] node_device_udev: Add support for `g_autoptr` to `udevEventData`
Use this feature in `udevEventDataNew`.
Signed-off-by: Marc Hartmayer <mhartmay(a)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
2:57 p.m.
New subject: [PATCH v1 19/20] node_device_udev: Add support for `g_autoptr` to
`udevEventData`
On 4/19/24 9:49 AM, Marc Hartmayer wrote:
Use this feature in `udevEventDataNew`.
Signed-off-by: Marc Hartmayer <mhartmay(a)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(a)redhat.com>
10:44 a.m.
New subject: [PATCH v1 19/20] node_device_udev: Add support for `g_autoptr` to
`udevEventData`
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
On 4/19/24 16:49, Marc Hartmayer wrote:
Use this feature in `udevEventDataNew`.
Signed-off-by: Marc Hartmayer <mhartmay(a)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
9:49 a.m.
New subject: [PATCH v1 20/20] node_device_udev: Pass the `udevEventData` via parameter and use refcounting
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(a)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
2:58 p.m.
New subject: [PATCH v1 20/20] node_device_udev: Pass the `udevEventData` via
parameter and use refcounting
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(a)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(a)redhat.com>
10:45 a.m.
New subject: [PATCH v1 20/20] node_device_udev: Pass the `udevEventData` via
parameter and use refcounting
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
211
days inactive
218
days old
53 comments
5 participants
participants (5)
-
Boris Fiuczynski -
Daniel P. Berrangé -
Jim Fehlig -
Jonathon Jongsma -
Marc Hartmayer