9:03 a.m.
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
9:03 a.m.
New subject: [RFC PATCH v1 1/5] node_device_udev: Remove the timeout if the data is disposed
Remove the timeout when the udevEventData is disposed, analogous to priv->watch.
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 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
10:49 a.m.
New subject: [RFC PATCH v1 1/5] node_device_udev: Remove the timeout if the
data is disposed
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(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 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
9:03 a.m.
New subject: [RFC PATCH v1 2/5] node_device_udev: Test for mdevctlTimeout != -1
It is done a little differently everywhere in libvirt, but most common is to
test for != -1.
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 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
10:49 a.m.
New subject: [RFC PATCH v1 2/5] node_device_udev: Test for mdevctlTimeout !=
-1
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(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 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
9:03 a.m.
New subject: [RFC PATCH v1 3/5] node_device_udev: 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. 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(a)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
10:50 a.m.
New subject: [RFC PATCH v1 3/5] node_device_udev: Add comments about locking
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)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
9:03 a.m.
New subject: [RFC PATCH v1 4/5] node_device_udev: Take lock if driver->privateData is modified
Since @driver->privateData is modified take the lock.
Question: In theory we could take the udevEventData->mdevctlLock?
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 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
10:50 a.m.
New subject: [RFC PATCH v1 4/5] node_device_udev: Take lock if
driver->privateData is modified
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(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 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
3:58 a.m.
New subject: [RFC PATCH v1 4/5] node_device_udev: Take lock if
driver->privateData is modified
On Thu, Apr 11, 2024 at 05:50 PM +0200, Boris Fiuczynski <fiuczy(a)linux.ibm.com>
wrote:
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
9:03 a.m.
New subject: [RFC PATCH v1 5/5] node_device_udev: Rename `th` to `udevThread`
The new thread name makes it easier to understand the purpose of the thread.
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 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
9:08 a.m.
New subject: [RFC PATCH v1 5/5] node_device_udev: Rename `th` to `udevThread`
On Wed, Apr 03, 2024 at 04:03 PM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com>
wrote:
The new thread name makes it easier to understand the purpose of the
thread.
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 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(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)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
10:50 a.m.
New subject: [RFC PATCH v1 5/5] node_device_udev: Rename `th` to `udevThread`
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(a)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(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 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
225
days inactive
234
days old
12 comments
2 participants
participants (2)
-
Boris Fiuczynski -
Marc Hartmayer