Re: [RFC PATCH v1 12/15] node_device_udev: Use a worker pool for processing the udev events

19 Apr 2024

      On Thu, Apr 18, 2024 at 05:19 PM -0500, Jonathon Jongsma <jjongsma@redhat.com> wrote:
...
On 4/12/24 8:36 AM, Marc Hartmayer wrote:
...
Use a worker pool for processing the udev events and the initialization instead
of a separate initThread and a mdevctl-thread. This has the large advantage that
we can leverage the job API and now this thread pool is responsible to do all
the "costly-work" and the libvirt nodedev event creation.
TODOs:
+ IMO, it's better practice for all functions called by the virThreadPool's
   worker thread to pass the driver via parameter and not global variables. Easier
   to test and cleaner.
I'm not opposed, but I think it should be a separate commit since it 
introduces a lot of churn that is unrelated to the thread pool.
Okay. I did not want to go to the trouble of doing the split before I
knew that the change made sense :)

[…snip…]
...
...
}
+typedef enum {
+  NODE_DEVICE_EVENT_INIT = 0,
+  NODE_DEVICE_EVENT_ADD,
+  NODE_DEVICE_EVENT_REMOVE,
+  NODE_DEVICE_EVENT_CHANGE,
+  NODE_DEVICE_EVENT_MOVE,
These events are from the udev subsystem, so I think it would be nicer 
to name them such. NODE_DEVICE_EVENT_UDEV_ADD, etc.
Okay, makes sense. What name would you suggest for the …_EVENT_INIT?
...
...
+  NODE_DEVICE_EVENT_UPDATE,
And this isn't really an event -- it's a description of what you want to 
do in response to the event. It might make more sense to be named 
something like:
NODE_DEVICE_EVENT_MDEVCTL_CONFIG_CHANGED
Yep.
...
I don't mind too much either way.
Sounds better, so I’m gonna change it.
...
...
+
+  NODE_DEVICE_EVENT_LAST
+} nodeDeviceEventType;
+
+struct _nodeDeviceEvent {
+    nodeDeviceEventType eventType;
+    void *data;
+};
I think it might be nicer to have the caller decide how the data should 
be freed by passing a free function like so:
struct _nodeDeviceEvent {
     nodeDeviceEventType eventType;
     void *data;
     virFreeCallback dataFree;
};
typedef struct _nodeDeviceEvent nodeDeviceEvent;
static nodeDeviceEvent*
nodeDeviceEventNew(nodeDeviceEventType event_type, gpointer data, 
virFreeCallback dataFree)
{
     nodeDeviceEvent *e = g_new0(nodeDeviceEvent, 1);
     e->eventType = event_type;
     e->data = data;
     e->dataFree = dataFree;
return e;
}
Oh yes, indeed :) Thanks.
...
...
[…snip…]
...
Then you don't have any decisions to make here about what to do for 
different event types, you just do something like:
if (event->data && event->dataFree)
         event->dataFree(event->data);
This reads little better:

      if (event->dataFree && event->data)
          event->dataFree(event->data);

Although, I would rather change it to:

      if (event->dataFree)
          event->dataFree(event->data);

as …Free should be able a handle a null pointer. But I don’t have strong
opinion about that.

[…snip]

What do you think about the idea from Boris to make the workers
configurable via virtnodedevd.conf?
...
-- 
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