Re: [RFC PATCH v1 12/15] node_device_udev: Use a worker pool for processing the udev events
On Thu, Apr 18, 2024 at 05:19 PM -0500, Jonathon Jongsma <jjongsma(a)redhat.com>
wrote:
--
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
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?