[RFC PATCH v1 0/5] node_device_udev: small improvements
The first patch fixes a resource leak, the other patches are small improvements and locking improvements. Marc Hartmayer (5): node_device_udev: Remove the timeout if the data is disposed node_device_udev: Test for mdevctlTimeout != -1 node_device_udev: Add comments about locking node_device_udev: Take lock if driver->privateData is modified node_device_udev: Rename `th` to `udevThread` src/node_device/node_device_udev.c | 42 ++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 14 deletions(-) base-commit: e2a7dd3f7e9843b0c0753cf6b6d9792351f8c6e1 -- 2.34.1
Remove the timeout when the udevEventData is disposed, analogous to priv->watch. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f1e402f8f7f6..b4bdfbeec841 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
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/3/24 16:03, Marc Hartmayer wrote:
Remove the timeout when the udevEventData is disposed, analogous to priv->watch.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index f1e402f8f7f6..b4bdfbeec841 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;
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
It is done a little differently everywhere in libvirt, but most common is to test for != -1. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b4bdfbeec841..9f2abb9eb0dd 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; } @@ -2077,7 +2078,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; } @@ -2187,7 +2188,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
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/3/24 16:03, Marc Hartmayer wrote:
It is done a little differently everywhere in libvirt, but most common is to test for != -1.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b4bdfbeec841..9f2abb9eb0dd 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; } @@ -2077,7 +2078,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; } @@ -2187,7 +2188,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);
-- 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
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. Restore this comment and add a new comment about the lock. Note: If the comment is/was correct, then each caller of `nodeDeviceUpdateMediatedDevices` must hold this lock. Currently, that's not the case. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9f2abb9eb0dd..8b4a62537c06 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -72,8 +72,10 @@ struct _udevEventData { /* init thread */ virThread *initThread; - GList *mdevctlMonitors; + /* Protects @mdevctlMonitors and must be taken when `mdevctl` command is + * called to make sure only one thread can query mdevctl at a time. */ virMutex mdevctlLock; + GList *mdevctlMonitors; int mdevctlTimeout; }; @@ -2065,6 +2067,7 @@ static void mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) { udevEventData *priv = driver->privateData; + /* ensure only a single thread can query mdevctl at a time */ VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock); if (nodeDeviceUpdateMediatedDevices() < 0) -- 2.34.1
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/3/24 16:03, 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. Restore this comment and add a new comment about the lock.
Note: If the comment is/was correct, then each caller of `nodeDeviceUpdateMediatedDevices` must hold this lock. Currently, that's not the case.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9f2abb9eb0dd..8b4a62537c06 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -72,8 +72,10 @@ struct _udevEventData { /* init thread */ virThread *initThread;
- GList *mdevctlMonitors; + /* Protects @mdevctlMonitors and must be taken when `mdevctl` command is + * called to make sure only one thread can query mdevctl at a time. */ virMutex mdevctlLock; + GList *mdevctlMonitors; int mdevctlTimeout; };
@@ -2065,6 +2067,7 @@ static void mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED) { udevEventData *priv = driver->privateData; + /* ensure only a single thread can query mdevctl at a time */ VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock);
if (nodeDeviceUpdateMediatedDevices() < 0)
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Since @driver->privateData is modified take the lock. Question: In theory we could take the udevEventData->mdevctlLock? Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 8b4a62537c06..ca60f6f7db82 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; @@ -1609,8 +1611,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); + } + } ret = 0; @@ -2232,7 +2237,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
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/3/24 16:03, Marc Hartmayer wrote:
Since @driver->privateData is modified take the lock.
Question: In theory we could take the udevEventData->mdevctlLock?
Isn't that protecting the access to the mdevctlMonitor? On the first sight scheduleMdevctlUpdate does not directly make use of mdevctlMonitor.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 8b4a62537c06..ca60f6f7db82 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; @@ -1609,8 +1611,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); + } + }
ret = 0;
@@ -2232,7 +2237,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)); + } }
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, Apr 11, 2024 at 05:50 PM +0200, Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
On 4/3/24 16:03, Marc Hartmayer wrote:
Since @driver->privateData is modified take the lock.
Question: In theory we could take the udevEventData->mdevctlLock?
Isn't that protecting the access to the mdevctlMonitor? On the first sight scheduleMdevctlUpdate does not directly make use of mdevctlMonitor.
That’s the question… what is the scope of @mdevctlLock :) -- 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
The new thread name makes it easier to understand the purpose of the thread. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ca60f6f7db82..87be5ae254ac 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -64,7 +64,7 @@ struct _udevEventData { int watch; /* Thread data */ - virThread *th; + virThread *udevThread; virCond threadCond; bool threadQuit; bool dataReady; @@ -1740,9 +1740,9 @@ nodeStateCleanup(void) 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); } } @@ -2335,12 +2335,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
On Wed, Apr 03, 2024 at 04:03 PM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
The new thread name makes it easier to understand the purpose of the thread.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ca60f6f7db82..87be5ae254ac 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -64,7 +64,7 @@ struct _udevEventData { int watch;
/* Thread data */ - virThread *th; + virThread *udevThread; virCond threadCond; bool threadQuit; bool dataReady;
It probably makes sense to add the prefix `udev` to threadCond, threadQuite and dataReady as well. […snip…]
2.34.1 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org -- 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
Makes sens and also adding the prefix to the other three attributes you mentioned in you follow up email. To all four... Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> On 4/3/24 16:03, Marc Hartmayer wrote:
The new thread name makes it easier to understand the purpose of the thread.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> --- src/node_device/node_device_udev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ca60f6f7db82..87be5ae254ac 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -64,7 +64,7 @@ struct _udevEventData { int watch;
/* Thread data */ - virThread *th; + virThread *udevThread; virCond threadCond; bool threadQuit; bool dataReady; @@ -1740,9 +1740,9 @@ nodeStateCleanup(void) 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); } }
@@ -2335,12 +2335,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; }
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Wolfgang Wendt Geschäftsführung: David Faller Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
Boris Fiuczynski -
Marc Hartmayer