[PATCH] node_device: Do not lock the driver state needlessly

Martin Kletzander posted 1 patch 10 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/f6f65ef53761e058dd95b3ec93d330727a887847.1738941142.git.mkletzan@redhat.com
src/node_device/node_device_udev.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
[PATCH] node_device: Do not lock the driver state needlessly
Posted by Martin Kletzander 10 months, 2 weeks ago
When processing the PCI devices we can only read the configs for each of
them if running as privileged.  That information is saved in the driver
state as a boolean introduced in commit 643c74abff01.  However since
that version it is only written to once during nodeStateInitialize() and
only read from that point (apart from some commits around v3.9.0 release
when it was not even set, but that was fixed before v3.10.0).  And it is
only read once, just to store that boolean in a temporary variable which
is also used in only one condition.

Rewrite this without locking and save few lines of code.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/node_device/node_device_udev.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 7b4ff80f8fb4..be5ee0108c85 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -430,7 +430,6 @@ udevProcessPCI(virNodeDeviceDriverState *driver_state,
     g_autofree char *linkpath = NULL;
     int ret = -1;
     char *p;
-    bool privileged = false;
 
     linkpath = g_strdup_printf("%s/config", udev_device_get_syspath(device));
     if (virFileWaitForExists(linkpath, 10, 100) < 0) {
@@ -440,10 +439,6 @@ udevProcessPCI(virNodeDeviceDriverState *driver_state,
         goto cleanup;
     }
 
-    VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) {
-        privileged = driver_state->privileged;
-    }
-
     pci_dev->klass = -1;
     if (udevGetIntProperty(device, "PCI_CLASS", &pci_dev->klass, 16) < 0)
         goto cleanup;
@@ -491,7 +486,7 @@ udevProcessPCI(virNodeDeviceDriverState *driver_state,
         goto cleanup;
 
     /* We need to be root to read PCI device configs */
-    if (privileged) {
+    if (driver_state->privileged) {
         if (virPCIGetHeaderType(pciDev, &pci_dev->hdrType) < 0)
             goto cleanup;
 
-- 
2.48.1
Re: [PATCH] node_device: Do not lock the driver state needlessly
Posted by Ján Tomko 10 months, 2 weeks ago
On a Friday in 2025, Martin Kletzander wrote:
>When processing the PCI devices we can only read the configs for each of
>them if running as privileged.  That information is saved in the driver
>state as a boolean introduced in commit 643c74abff01.  However since

Please don't put a period right after commit IDs.

>that version it is only written to once during nodeStateInitialize() and
>only read from that point (apart from some commits around v3.9.0 release
>when it was not even set, but that was fixed before v3.10.0).  And it is
>only read once, just to store that boolean in a temporary variable which
>is also used in only one condition.
>
>Rewrite this without locking and save few lines of code.
>
>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---
> src/node_device/node_device_udev.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
Re: [PATCH] node_device: Do not lock the driver state needlessly
Posted by Martin Kletzander 10 months, 1 week ago
On Fri, Feb 07, 2025 at 04:32:07PM +0100, Ján Tomko wrote:
>On a Friday in 2025, Martin Kletzander wrote:
>>When processing the PCI devices we can only read the configs for each of
>>them if running as privileged.  That information is saved in the driver
>>state as a boolean introduced in commit 643c74abff01.  However since
>
>Please don't put a period right after commit IDs.
>

That's fine, it's not a period, but a full stop.  Just like in commit
4a6b246d3941e8e6ff22a951dae103594b5d2096 for example.  If that is still an
issue please file a bug report for git revert messaging.

>>that version it is only written to once during nodeStateInitialize() and
>>only read from that point (apart from some commits around v3.9.0 release
>>when it was not even set, but that was fixed before v3.10.0).  And it is
>>only read once, just to store that boolean in a temporary variable which
>>is also used in only one condition.
>>
>>Rewrite this without locking and save few lines of code.
>>
>>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>---
>> src/node_device/node_device_udev.c | 7 +------
>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>
>
>Reviewed-by: Ján Tomko <jtomko@redhat.com>
>

Thank you

>Jano


Re: [PATCH] node_device: Do not lock the driver state needlessly
Posted by Ján Tomko 10 months, 1 week ago
On a Wednesday in 2025, Martin Kletzander wrote:
>On Fri, Feb 07, 2025 at 04:32:07PM +0100, Ján Tomko wrote:
>>On a Friday in 2025, Martin Kletzander wrote:
>>>When processing the PCI devices we can only read the configs for each of
>>>them if running as privileged.  That information is saved in the driver
>>>state as a boolean introduced in commit 643c74abff01.  However since
>>
>>Please don't put a period right after commit IDs.
>>
>
>That's fine, it's not a period, but a full stop.  Just like in commit
>4a6b246d3941e8e6ff22a951dae103594b5d2096 for example.  If that is still an
>issue please file a bug report for git revert messaging.
>
>>>that version it is only written to once during nodeStateInitialize() and
>>>only read from that point (apart from some commits around v3.9.0 release
>>>when it was not even set, but that was fixed before v3.10.0).  And it is
>>>only read once, just to store that boolean in a temporary variable which
>>>is also used in only one condition.
>>>
>>>Rewrite this without locking and save few lines of code.
>>>
>>>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>>---
>>>src/node_device/node_device_udev.c | 7 +------
>>>1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>
>>Reviewed-by: Ján Tomko <jtomko@redhat.com>
>>
>
>Thank you
>

You're welcome...

Jano