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

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

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.

> + how many worker threads should we have at maximum?
> + there are still TODO's in the code
> + improve error reporting
> + improve naming - e.g. rename more udevXXX functions?
>
> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxx>
> ---
>   src/node_device/node_device_driver.h |   2 +-
>   src/node_device/node_device_driver.c |   6 +-
>   src/node_device/node_device_udev.c   | 295 +++++++++++++++++++--------
>   3 files changed, 209 insertions(+), 94 deletions(-)
>
> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
> index f195cfef9d49..2781ad136d68 100644
> --- a/src/node_device/node_device_driver.h
> +++ b/src/node_device/node_device_driver.h
> @@ -147,7 +147,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
>                              bool defined);
>   
>   int
> -nodeDeviceUpdateMediatedDevices(void);
> +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *driver);
>   
>   void
>   nodeDeviceGenerateName(virNodeDeviceDef *def,
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index f623339dc973..59c5f9b417a4 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -1887,7 +1887,7 @@ removeMissingPersistentMdev(virNodeDeviceObj *obj,
>   
>   
>   int
> -nodeDeviceUpdateMediatedDevices(void)
> +nodeDeviceUpdateMediatedDevices(virNodeDeviceDriverState *node_driver)
>   {
>       g_autofree virNodeDeviceDef **defs = NULL;
>       g_autofree virNodeDeviceDef **act_defs = NULL;
> @@ -1911,7 +1911,7 @@ nodeDeviceUpdateMediatedDevices(void)
>       /* Any mdevs that were previously defined but were not returned in the
>        * latest mdevctl query should be removed from the device list */
>       data.defs = defs;
> -    virNodeDeviceObjListForEachRemove(driver->devs,
> +    virNodeDeviceObjListForEachRemove(node_driver->devs,
>                                         removeMissingPersistentMdev, &data);
>   
>       for (i = 0; i < data.ndefs; i++)
> @@ -2374,7 +2374,7 @@ nodeDeviceUpdate(virNodeDevice *device,
>    cleanup:
>       virNodeDeviceObjEndAPI(&obj);
>       if (updated)
> -        nodeDeviceUpdateMediatedDevices();
> +        nodeDeviceUpdateMediatedDevices(driver);
>   
>       return ret;
>   }

So far, all of the changes have only been related to the change of the 
global driver variable.


> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index cec7d837c43e..2a252d8fe62b 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -43,6 +43,7 @@
>   #include "virnetdev.h"
>   #include "virmdev.h"
>   #include "virutil.h"
> +#include "virthreadpool.h"
>   
>   #include "configmake.h"
>   
> @@ -69,14 +70,14 @@ struct _udevEventData {
>       bool udevThreadQuit;
>       bool udevDataReady;
>   
> -    /* init thread */
> -    virThread *initThread;
> -
>       /* 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;
> +
> +    /* Immutable pointer, self-locking APIs */
> +    virThreadPool *workerPool;
>   };
>   
>   static virClass *udevEventDataClass;
> @@ -146,6 +147,79 @@ udevEventDataNew(void)
>       return ret;
>   }
>   
> +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.

> +  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

I don't mind too much either way.

> +
> +  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;
}

...

> +typedef struct _nodeDeviceEvent nodeDeviceEvent;
> +
> +static void
> +nodeDeviceEventFree(nodeDeviceEvent *event)
> +{
> +    if (!event)
> +        return;
> +
> +    switch (event->eventType) {
> +    case NODE_DEVICE_EVENT_INIT:
> +        udev_unref(event->data);
> +        break;
> +    case NODE_DEVICE_EVENT_ADD:
> +    case NODE_DEVICE_EVENT_CHANGE:
> +    case NODE_DEVICE_EVENT_MOVE:
> +    case NODE_DEVICE_EVENT_REMOVE:
> +        udev_device_unref(event->data);
> +        break;
> +    case NODE_DEVICE_EVENT_UPDATE:
> +        break;
> +    case NODE_DEVICE_EVENT_LAST:
> +        // TODO Bug!
> +        g_abort();

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);

> +        break;
> +    }
> +    g_free(event);
> +} > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(nodeDeviceEvent, nodeDeviceEventFree);
> +
> + /**
> +  * nodeDeviceEventSubmit:
> +  * @eventType: the event to be processed
> +  * @data: additional data for the event processor (the pointer is stolen and it
> +  *        will be properly freed)
> +  *
> +  * Submits @eventType to be processed by the asynchronous event handling
> +  * thread.
> +  */
> +static int nodeDeviceEventSubmit(nodeDeviceEventType eventType, void *data)
> +{
> +    nodeDeviceEvent *event = g_new0(nodeDeviceEvent, 1);
> +    udevEventData *priv = NULL;
> +
> +    /* BUG */
> +    if (!driver)
> +        g_abort();
> +
> +    priv = driver->privateData;
> +
> +    event->eventType = eventType;
> +    event->data = data;
> +    if (virThreadPoolSendJob(priv->workerPool, 0, event) < 0) {
> +        nodeDeviceEventFree(event);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
>   
>   static bool
>   udevHasDeviceProperty(struct udev_device *dev,
> @@ -364,7 +438,7 @@ udevTranslatePCIIds(unsigned int vendor,
>   
>   
>   static int
> -udevProcessPCI(struct udev_device *device,
> +udevProcessPCI(virNodeDeviceDriverState *driver_state, struct udev_device *device,
>                  virNodeDeviceDef *def)
>   {
>       virNodeDevCapPCIDev *pci_dev = &def->caps->data.pci_dev;
> @@ -375,8 +449,8 @@ udevProcessPCI(struct udev_device *device,
>       char *p;
>       bool privileged = false;
>   
> -    VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) {
> -        privileged = driver->privileged;
> +    VIR_WITH_MUTEX_LOCK_GUARD(&driver_state->lock) {
> +        privileged = driver_state->privileged;
>       }
>   
>       pci_dev->klass = -1;
> @@ -1394,12 +1468,12 @@ udevGetDeviceType(struct udev_device *device,
>   
>   
>   static int
> -udevGetDeviceDetails(struct udev_device *device,
> +udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, struct udev_device *device,
>                        virNodeDeviceDef *def)
>   {
>       switch (def->caps->data.type) {
>       case VIR_NODE_DEV_CAP_PCI_DEV:
> -        return udevProcessPCI(device, def);
> +        return udevProcessPCI(driver_state, device, def);
>       case VIR_NODE_DEV_CAP_USB_DEV:
>           return udevProcessUSBDevice(device, def);
>       case VIR_NODE_DEV_CAP_USB_INTERFACE:
> @@ -1450,13 +1524,13 @@ static void scheduleMdevctlUpdate(udevEventData *data, bool force);
>   
>   
>   static int
> -udevRemoveOneDeviceSysPath(const char *path)
> +processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char *path)
>   {
>       virNodeDeviceObj *obj = NULL;
>       virNodeDeviceDef *def;
>       virObjectEvent *event = NULL;
>   
> -    if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) {
> +    if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs, path))) {
>           VIR_DEBUG("Failed to find device to remove that has udev path '%s'",
>                     path);
>           return -1;
> @@ -1477,21 +1551,21 @@ udevRemoveOneDeviceSysPath(const char *path)
>       } else {
>           VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
>                     def->name, path);
> -        virNodeDeviceObjListRemove(driver->devs, obj);
> +        virNodeDeviceObjListRemove(driver_state->devs, obj);
>       }
>       virNodeDeviceObjEndAPI(&obj);
>   
>       /* cannot check for mdev_types since they have already been removed */
>       VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) {
> -        scheduleMdevctlUpdate(driver->privateData, false);
> +        scheduleMdevctlUpdate(driver_state->privateData, false);
>       }
>   
> -    virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> +    virObjectEventStateQueue(driver_state->nodeDeviceEventState, event);
>       return 0;
>   }
>   
>   static int
> -udevSetParent(struct udev_device *device,
> +udevSetParent(virNodeDeviceDriverState *driver_state, struct udev_device *device,
>                 virNodeDeviceDef *def)
>   {
>       struct udev_device *parent_device = NULL;
> @@ -1514,7 +1588,7 @@ udevSetParent(struct udev_device *device,
>               return -1;
>           }
>   
> -        if ((obj = virNodeDeviceObjListFindBySysfsPath(driver->devs,
> +        if ((obj = virNodeDeviceObjListFindBySysfsPath(driver_state->devs,
>                                                          parent_sysfs_path))) {
>               objdef = virNodeDeviceObjGetDef(obj);
>               def->parent = g_strdup(objdef->name);
> @@ -1532,7 +1606,7 @@ udevSetParent(struct udev_device *device,
>   }
>   
>   static int
> -udevAddOneDevice(struct udev_device *device)
> +processNodeDeviceAddAndChangeEvent(virNodeDeviceDriverState *driver_state, struct udev_device *device)
>   {
>       g_autofree char *sysfs_path = NULL;
>       virNodeDeviceDef *def = NULL;
> @@ -1563,15 +1637,15 @@ udevAddOneDevice(struct udev_device *device)
>       if (udevGetDeviceNodes(device, def) != 0)
>           goto cleanup;
>   
> -    if (udevGetDeviceDetails(device, def) != 0)
> +    if (udevGetDeviceDetails(driver_state, device, def) != 0)
>           goto cleanup;
>   
> -    if (udevSetParent(device, def) != 0)
> +    if (udevSetParent(driver_state, device, def) != 0)
>           goto cleanup;
>   
>       is_mdev = def->caps->data.type == VIR_NODE_DEV_CAP_MDEV;
>   
> -    if ((obj = virNodeDeviceObjListFindByName(driver->devs, def->name))) {
> +    if ((obj = virNodeDeviceObjListFindByName(driver_state->devs, def->name))) {
>           objdef = virNodeDeviceObjGetDef(obj);
>   
>           if (is_mdev)
> @@ -1589,7 +1663,7 @@ udevAddOneDevice(struct udev_device *device)
>   
>       /* If this is a device change, the old definition will be freed
>        * and the current definition will take its place. */
> -    if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
> +    if (!(obj = virNodeDeviceObjListAssignDef(driver_state->devs, def)))
>           goto cleanup;
>       /* @def is now owned by @obj */
>       def = NULL;
> @@ -1609,14 +1683,14 @@ udevAddOneDevice(struct udev_device *device)
>       virNodeDeviceObjEndAPI(&obj);
>   
>       if (has_mdev_types) {
> -        VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) {
> -            scheduleMdevctlUpdate(driver->privateData, false);
> +        VIR_WITH_OBJECT_LOCK_GUARD(driver_state->privateData) {
> +            scheduleMdevctlUpdate(driver_state->privateData, false);
>           }
>       }
>   
>       /* The added mdev needs an immediate active config update before
>        * the event is issued to allow sane API usage. */
> -    if (is_mdev && (nodeDeviceUpdateMediatedDevices() < 0)) {
> +    if (is_mdev && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) {
>           VIR_WARN("Update of mediated device %s failed",
>                    NULLSTR_EMPTY(sysfs_path));
>       }
> @@ -1624,7 +1698,7 @@ udevAddOneDevice(struct udev_device *device)
>       ret = 0;
>   
>    cleanup:
> -    virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> +    virObjectEventStateQueue(driver_state->nodeDeviceEventState, event);
>   
>       if (ret != 0) {
>           VIR_DEBUG("Discarding device %d %p %s", ret, def,
> @@ -1637,7 +1711,7 @@ udevAddOneDevice(struct udev_device *device)
>   
>   
>   static int
> -udevProcessDeviceListEntry(struct udev *udev,
> +udevProcessDeviceListEntry(virNodeDeviceDriverState *driver_state, struct udev *udev,
>                              struct udev_list_entry *list_entry)
>   {
>       struct udev_device *device;
> @@ -1649,7 +1723,7 @@ udevProcessDeviceListEntry(struct udev *udev,
>       device = udev_device_new_from_syspath(udev, name);
>   
>       if (device != NULL) {
> -        if (udevAddOneDevice(device) != 0) {
> +        if (processNodeDeviceAddAndChangeEvent(driver_state, device) != 0) {
>               VIR_DEBUG("Failed to create node device for udev device '%s'",
>                         name);
>           }
> @@ -1687,7 +1761,7 @@ udevEnumerateAddMatches(struct udev_enumerate *udev_enumerate)
>   
>   
>   static int
> -udevEnumerateDevices(struct udev *udev)
> +udevEnumerateDevices(virNodeDeviceDriverState *driver_state, struct udev *udev)
>   {
>       struct udev_enumerate *udev_enumerate = NULL;
>       struct udev_list_entry *list_entry = NULL;
> @@ -1703,7 +1777,7 @@ udevEnumerateDevices(struct udev *udev)
>       udev_list_entry_foreach(list_entry,
>                               udev_enumerate_get_list_entry(udev_enumerate)) {
>   
> -        udevProcessDeviceListEntry(udev, list_entry);
> +        udevProcessDeviceListEntry(driver_state, udev, list_entry);
>       }
>   
>       ret = 0;
> @@ -1736,8 +1810,8 @@ nodeStateCleanup(void)
>   
>       priv = driver->privateData;
>       if (priv) {
> -        g_clear_pointer(&priv->initThread, g_free);
>           g_clear_pointer(&priv->udevThread, g_free);
> +        virThreadPoolFree(priv->workerPool);
>       }
>   
>       virObjectUnref(priv);
> @@ -1765,26 +1839,19 @@ udevHandleOneDevice(struct udev_device *device)
>   
>       VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));
>   
> -    if (STREQ(action, "add") || STREQ(action, "change"))
> -        return udevAddOneDevice(device);
> -
> -    if (STREQ(action, "remove")) {
> -        const char *path = udev_device_get_syspath(device);
> -
> -        return udevRemoveOneDeviceSysPath(path);
> -    }
> -
> -    if (STREQ(action, "move")) {
> -        const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD");
> -
> -        if (devpath_old) {
> -            g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old);
> -
> -            udevRemoveOneDeviceSysPath(devpath_old_fixed);
> -        }
> -
> -        return udevAddOneDevice(device);
> +    /* Reference is either released via workerpool logic or at the end of this
> +     * function. */
> +    device = udev_device_ref(device);
> +    if (STREQ(action, "add")) {
> +        return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_ADD, device);
> +    } else if (STREQ(action, "change")) {
> +        return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_CHANGE, device);
> +    } else if (STREQ(action, "remove")) {
> +        return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_REMOVE, device);
> +    } else if (STREQ(action, "move")) {
> +        return nodeDeviceEventSubmit(NODE_DEVICE_EVENT_MOVE, device);
>       }
> +    udev_device_unref(device);
>   
>       return 0;
>   }
> @@ -2003,23 +2070,22 @@ udevSetupSystemDev(void)
>   
>   
>   static void
> -nodeStateInitializeEnumerate(void *opaque)
> +processNodeStateInitializeEnumerate(virNodeDeviceDriverState *event_driver, struct udev *udev)
>   {
> -    struct udev *udev = opaque;
> -    udevEventData *priv = driver->privateData;
> +    udevEventData *priv = event_driver->privateData;
>   
>       /* Populate with known devices */
> -    if (udevEnumerateDevices(udev) != 0)
> +    if (udevEnumerateDevices(event_driver, udev) != 0)
>           goto error;
>       /* Load persistent mdevs (which might not be activated yet) and additional
>        * information about active mediated devices from mdevctl */
> -    if (nodeDeviceUpdateMediatedDevices() != 0)
> +    if (nodeDeviceUpdateMediatedDevices(event_driver) != 0)
>           goto error;
>   
>    cleanup:
> -    VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) {
> -        driver->initialized = true;
> -        virCondBroadcast(&driver->initCond);
> +    VIR_WITH_MUTEX_LOCK_GUARD(&event_driver->lock) {
> +        event_driver->initialized = true;
> +        virCondBroadcast(&event_driver->initCond);
>       }
>   
>       return;
> @@ -2059,35 +2125,17 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
>       return 0;
>   }
>   
> -
>   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)
> -        VIR_WARN("mdevctl failed to update mediated devices");
> -}
> -
> -
> -static void
> -launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque)
> +submitMdevctlUpdate(int timer G_GNUC_UNUSED, void *opaque)
>   {
>       udevEventData *priv = opaque;
> -    virThread thread;
>   
>       if (priv->mdevctlTimeout != -1) {
>           virEventRemoveTimeout(priv->mdevctlTimeout);
>           priv->mdevctlTimeout = -1;
>       }
>   
> -    if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc,
> -                            "mdevctl-thread", false, NULL) < 0) {
> -        virReportSystemError(errno, "%s",
> -                             _("failed to create mdevctl thread"));
> -    }
> +    nodeDeviceEventSubmit(NODE_DEVICE_EVENT_UPDATE,  NULL);
>   }
>   
>   
> @@ -2182,7 +2230,7 @@ mdevctlEnableMonitor(udevEventData *priv)
>   /* Schedules an mdevctl update for 100ms in the future, canceling any existing
>    * timeout that may have been set. In this way, multiple update requests in
>    * quick succession can be collapsed into a single update. if @force is true,
> - * an update thread will be spawned immediately. */
> + * the worker job is submitted immediately. */
>   static void
>   scheduleMdevctlUpdate(udevEventData *data,
>                         bool force)
> @@ -2190,12 +2238,12 @@ scheduleMdevctlUpdate(udevEventData *data,
>       if (!force) {
>           if (data->mdevctlTimeout != -1)
>               virEventRemoveTimeout(data->mdevctlTimeout);
> -        data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread,
> +        data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate,
>                                                     data, NULL);
>           return;
>       }
>   
> -    launchMdevctlUpdateThread(-1, data);
> +    submitMdevctlUpdate(-1, data);
>   }
>   
>   
> @@ -2235,6 +2283,67 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
>   }
>   
>   
> +static void nodeDeviceEventHandler(void *data, void *opaque)
> +{
> +    virNodeDeviceDriverState *driver_state = opaque;
> +    g_autoptr(nodeDeviceEvent) processEvent = data;
> +
> +    switch (processEvent->eventType) {
> +    case NODE_DEVICE_EVENT_INIT:
> +    {
> +        struct udev *udev = processEvent->data;
> +
> +        processNodeStateInitializeEnumerate(driver_state, udev);
> +    }
> +    break;
> +    case NODE_DEVICE_EVENT_ADD:
> +    case NODE_DEVICE_EVENT_CHANGE:
> +    {
> +        struct udev_device *device = processEvent->data;
> +
> +        processNodeDeviceAddAndChangeEvent(driver_state, device);
> +    }
> +    break;
> +    case NODE_DEVICE_EVENT_REMOVE:
> +    {
> +        struct udev_device *device = processEvent->data;
> +        const char *path = udev_device_get_syspath(device);
> +
> +        processNodeDeviceRemoveEvent(driver_state, path);
> +    }
> +    break;
> +    case NODE_DEVICE_EVENT_MOVE:
> +    {
> +        struct udev_device *device = processEvent->data;
> +        const char *devpath_old = udevGetDeviceProperty(device, "DEVPATH_OLD");
> +
> +        if (devpath_old) {
> +            g_autofree char *devpath_old_fixed = g_strdup_printf("/sys%s", devpath_old);
> +
> +            processNodeDeviceRemoveEvent(driver_state, devpath_old_fixed);
> +        }
> +
> +        processNodeDeviceAddAndChangeEvent(driver_state, device);
> +    }
> +    break;
> +    case NODE_DEVICE_EVENT_UPDATE:
> +    {
> +        udevEventData *priv = driver_state->privateData;
> +        /* ensure only a single thread can query mdevctl at a time */
> +        VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock);
> +
> +        if (nodeDeviceUpdateMediatedDevices(driver_state) < 0)
> +            VIR_WARN("mdevctl failed to update mediated devices");
> +    }
> +    break;
> +    case NODE_DEVICE_EVENT_LAST:
> +        // TODO Bug!
> +        g_abort();
> +        break;
> +    }
> +}
> +
> +
>   static int
>   nodeStateInitialize(bool privileged,
>                       const char *root,
> @@ -2301,6 +2410,16 @@ nodeStateInitialize(bool privileged,
>       driver->parserCallbacks.postParse = nodeDeviceDefPostParse;
>       driver->parserCallbacks.validate = nodeDeviceDefValidate;
>   
> +    /* must be initialized before trying to reconnect to all the running mdevs
> +     * since there might occur some mdevctl monitor events that will be
> +     * dispatched to the worker pool */
> +    priv->workerPool = virThreadPoolNewFull(1, 10, 0, nodeDeviceEventHandler,
> +                                            "nodev-device-event",
> +                                            NULL,
> +                                            driver);
> +    if (!priv->workerPool)
> +        goto unlock;
> +
>       if (udevPCITranslateInit(privileged) < 0)
>           goto unlock;
>   
> @@ -2359,14 +2478,7 @@ nodeStateInitialize(bool privileged,
>       if (udevSetupSystemDev() != 0)
>           goto cleanup;
>   
> -    priv->initThread = g_new0(virThread, 1);
> -    if (virThreadCreateFull(priv->initThread, true, nodeStateInitializeEnumerate,
> -                            "nodedev-init", false, udev) < 0) {
> -        virReportSystemError(errno, "%s",
> -                             _("failed to create udev enumerate thread"));
> -        g_clear_pointer(&priv->initThread, g_free);
> -        goto cleanup;
> -    }
> +    nodeDeviceEventSubmit(NODE_DEVICE_EVENT_INIT, udev_ref(udev));
>   
>       return VIR_DRV_STATE_INIT_COMPLETE;
>   
> @@ -2446,6 +2558,9 @@ nodeStateShutdownPrepare(void)
>           priv->udevThreadQuit = true;
>           virCondSignal(&priv->udevThreadCond);
>       }
> +
> +    if (priv->workerPool)
> +        virThreadPoolStop(priv->workerPool);
>       return 0;
>   }
>   
> @@ -2461,11 +2576,11 @@ nodeStateShutdownWait(void)
>       if (!priv)
>           return 0;
>   
> -    if (priv->initThread)
> -      virThreadJoin(priv->initThread);
> -
>       if (priv->udevThread)
>         virThreadJoin(priv->udevThread);
> +
> +    if (priv->workerPool)
> +        virThreadPoolDrain(priv->workerPool);
>       return 0;
>   }
>   
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx