Re: [RFC PATCH v1 07/15] node_device_udev: Add comments about locking

On 4/12/24 8:36 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. Restore this comment and add a new comment about the lock. > > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; > --- > 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 469538a1395d..5d474acdc2e0 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; > }; > > @@ -2074,6 +2076,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) Serializing mdevctl queries is not strictly necessary, and in fact is removed in a later patch in this series (14), so I think we can just drop this patch, tbh. I'm not sure that this mdevctlLock is even necessary. The udevEventData struct is already a lockable object, so I think we could simply get rid of this lock and use the object lock to protect the mdevctlMonitors variable if we wanted.

Jonathon