On Thu, Apr 18, 2024 at 05:19 PM -0500, Jonathon Jongsma <jjongsma@xxxxxxxxxx> 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
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
