[v1] node_dev_udev: use workerpool and improve nodedev events

[PATCH v1 07/20] node_device_udev: Don't take `mdevctlLock` for `mdevctl list` and add comments about locking

Posted by Marc Hartmayer 1 year, 9 months ago

Commit a99d876a0f58 ("node_device: Use automatic mutex management") replaced the
locking mechanism and accidentally removed the comment with the reason why the
lock is taken. The reason was to "ensure only a single thread can query mdevctl
at a time", but this reason is no longer valid or maybe it never was. Therefore,
let's remove this lock and add a comment to `mdevCtl` what it protects.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 src/node_device/node_device_udev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 049501c62870..757febffa2f8 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -72,8 +72,9 @@ struct _udevEventData {
     /* init thread */
     virThread *initThread;
 
-    GList *mdevctlMonitors;
+    /* Protects @mdevctlMonitors */
     virMutex mdevctlLock;
+    GList *mdevctlMonitors;
     int mdevctlTimeout;
 };
 
@@ -2074,9 +2075,6 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
 static void
 mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED)
 {
-    udevEventData *priv = driver->privateData;
-    VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock);
-
     if (nodeDeviceUpdateMediatedDevices() < 0)
         VIR_WARN("mdevctl failed to update mediated devices");
 }
-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH v1 07/20] node_device_udev: Don't take `mdevctlLock` for `mdevctl list` and add comments about locking

Posted by Boris Fiuczynski 1 year, 9 months ago

Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

On 4/19/24 16:49, Marc Hartmayer wrote:
> Commit a99d876a0f58 ("node_device: Use automatic mutex management") replaced the
> locking mechanism and accidentally removed the comment with the reason why the
> lock is taken. The reason was to "ensure only a single thread can query mdevctl
> at a time", but this reason is no longer valid or maybe it never was. Therefore,
> let's remove this lock and add a comment to `mdevCtl` what it protects.
> 
> Signed-off-by: Marc Hartmayer<mhartmay@linux.ibm.com>

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH v1 07/20] node_device_udev: Don't take `mdevctlLock` for `mdevctl list` and add comments about locking

Posted by Jonathon Jongsma 1 year, 9 months ago

On 4/19/24 9:49 AM, Marc Hartmayer wrote:
> Commit a99d876a0f58 ("node_device: Use automatic mutex management") replaced the
> locking mechanism and accidentally removed the comment with the reason why the
> lock is taken. The reason was to "ensure only a single thread can query mdevctl
> at a time", but this reason is no longer valid or maybe it never was. Therefore,
> let's remove this lock and add a comment to `mdevCtl` what it protects.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>   src/node_device/node_device_udev.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 049501c62870..757febffa2f8 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -72,8 +72,9 @@ struct _udevEventData {
>       /* init thread */
>       virThread *initThread;
>   
> -    GList *mdevctlMonitors;
> +    /* Protects @mdevctlMonitors */
>       virMutex mdevctlLock;
> +    GList *mdevctlMonitors;
>       int mdevctlTimeout;
>   };
>   
> @@ -2074,9 +2075,6 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
>   static void
>   mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED)
>   {
> -    udevEventData *priv = driver->privateData;
> -    VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock);
> -
>       if (nodeDeviceUpdateMediatedDevices() < 0)
>           VIR_WARN("mdevctl failed to update mediated devices");
>   }

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org