On Fri, Apr 12, 2024 at 03:36 PM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; wrote: This s-o-b is there by accident, will remove it in the next version. -- 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

Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; On 4/12/24 15:36, Marc Hartmayer wrote: -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski 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: s/immediate/immediately/ How about simply something like "so that full device information is available at the time that the 'created' event is emitted" Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;

From: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; When a mdev device is destroyed or stopped the udev remove event handling needs to reset the active config data of the node object representing a persisted mdev. Reviewed-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; Signed-off-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_driver.h | 3 +++ src/util/virmdev.h | 4 ++++ src/conf/node_device_conf.c | 10 ++-------- src/node_device/node_device_driver.c | 13 +++++++++++++ src/node_device/node_device_udev.c | 1 + src/util/virmdev.c | 20 ++++++++++++++++++++ src/libvirt_private.syms | 2 ++ 7 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h index b3bc4b2e96ed..f195cfef9d49 100644 --- a/src/node_device/node_device_driver.h +++ b/src/node_device/node_device_driver.h @@ -197,3 +197,6 @@ int nodeDeviceUpdate(virNodeDevice *dev, const char *xmlDesc, unsigned int flags); + +void +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def); diff --git a/src/util/virmdev.h b/src/util/virmdev.h index 853041ac0619..e8e69040e5d4 100644 --- a/src/util/virmdev.h +++ b/src/util/virmdev.h @@ -54,6 +54,10 @@ struct _virMediatedDeviceConfig { size_t nattributes; }; +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config); +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceConfig, virMediatedDeviceConfigFree); + typedef struct _virMediatedDeviceType virMediatedDeviceType; struct _virMediatedDeviceType { char *id; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 5cfbd6a7eb72..897c67d6af91 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2592,15 +2592,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps) g_free(data->sg.path); break; case VIR_NODE_DEV_CAP_MDEV: - g_free(data->mdev.defined_config.type); - g_free(data->mdev.active_config.type); g_free(data->mdev.uuid); - for (i = 0; i < data->mdev.defined_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]); - g_free(data->mdev.defined_config.attributes); - for (i = 0; i < data->mdev.active_config.nattributes; i++) - virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]); - g_free(data->mdev.active_config.attributes); + virMediatedDeviceConfigClear(&data->mdev.defined_config); + virMediatedDeviceConfigClear(&data->mdev.active_config); g_free(data->mdev.parent_addr); break; case VIR_NODE_DEV_CAP_CSS_DEV: diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d99b48138ebf..f623339dc973 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -2018,6 +2018,19 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst, } +/* A mediated device definition contains data from mdevctl about the active + * device. When the device is deactivated the active configuration data needs + * to be removed. */ +void +nodeDeviceDefResetMdevActiveConfig(virNodeDeviceDef *def) +{ + if (def->caps->data.type != VIR_NODE_DEV_CAP_MDEV) + return; + + virMediatedDeviceConfigClear(&def->caps->data.mdev.active_config); +} + + int nodeDeviceSetAutostart(virNodeDevice *device, int autostart) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 03ef0c14371a..3297bdd8d7ef 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1467,6 +1467,7 @@ udevRemoveOneDeviceSysPath(const char *path) if (virNodeDeviceObjIsPersistent(obj)) { VIR_FREE(def->sysfs_path); virNodeDeviceObjSetActive(obj, false); + nodeDeviceDefResetMdevActiveConfig(def); } else { VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, path); diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 992f3eb1b74c..6ecdbdf0ab77 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -516,6 +516,26 @@ void virMediatedDeviceAttrFree(virMediatedDeviceAttr *attr) g_free(attr); } +void virMediatedDeviceConfigFree(virMediatedDeviceConfig *config) +{ + if (!config) + return; + + virMediatedDeviceConfigClear(config); + g_free(config); +} + +void virMediatedDeviceConfigClear(virMediatedDeviceConfig *config) +{ + size_t i = 0; + + g_clear_pointer(&config->type, g_free); + for (i = 0; i < config->nattributes; i++) + virMediatedDeviceAttrFree(config->attributes[i]); + config->nattributes = 0; + g_clear_pointer(&config->attributes, g_free); +} + #define MDEV_BUS_DIR "/sys/class/mdev_bus" diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 84e30b711c67..fd413949dae2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2778,6 +2778,8 @@ virMacMapWriteFile; # util/virmdev.h virMediatedDeviceAttrFree; virMediatedDeviceAttrNew; +virMediatedDeviceConfigClear; +virMediatedDeviceConfigFree; virMediatedDeviceFree; virMediatedDeviceGetIOMMUGroupDev; virMediatedDeviceGetIOMMUGroupNum; -- 2.34.1

On Thu, Apr 18, 2024 at 10:03 AM -0500, Jonathon Jongsma <jjongsma(a)redhat.com&gt; wrote: Yep, was just for completeness. Shall I remove it? Maybe it will be used in the future and if we have a …Clear function, I guess it’s always good to have a …Free function as well. Don’t have a strong opinion about this, @Boris? […snip…] Thanks. -- 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: Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;

On 4/12/24 8:36 AM, Marc Hartmayer wrote: OK Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;

On 4/12/24 8:36 AM, Marc Hartmayer wrote: 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

Since @driver->privateData is modified take 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 | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 5d474acdc2e0..0c393b6233a4 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1482,7 +1482,9 @@ udevRemoveOneDeviceSysPath(const char *path) virNodeDeviceObjEndAPI(&obj); /* cannot check for mdev_types since they have already been removed */ - scheduleMdevctlUpdate(driver->privateData, false); + VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { + scheduleMdevctlUpdate(driver->privateData, false); + } virObjectEventStateQueue(driver->nodeDeviceEventState, event); return 0; @@ -1616,8 +1618,11 @@ udevAddOneDevice(struct udev_device *device) has_mdev_types = virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_MDEV_TYPES); virNodeDeviceObjEndAPI(&obj); - if (has_mdev_types) - scheduleMdevctlUpdate(driver->privateData, false); + if (has_mdev_types) { + VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { + scheduleMdevctlUpdate(driver->privateData, false); + } + } /* The added mdev needs an immediate active config update before * the event is issued to allow sane API usage. */ @@ -2241,7 +2246,9 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, * configuration change, try to coalesce these changes by waiting for the * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the * signal if that event never comes */ - scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + } } -- 2.34.1

On 4/12/24 8:36 AM, Marc Hartmayer wrote: Sure. Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;

Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; On 4/12/24 15:36, Marc Hartmayer wrote: -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski 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

Use the proper driver functions for the node state shutdown preparation and wait. In the next patch, these functions will be extended. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 54 +++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 672da8f5a19f..cec7d837c43e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1736,18 +1736,8 @@ nodeStateCleanup(void) priv = driver->privateData; if (priv) { - VIR_WITH_OBJECT_LOCK_GUARD(priv) { - priv->udevThreadQuit = true; - virCondSignal(&priv->udevThreadCond); - } - if (priv->initThread) { - virThreadJoin(priv->initThread); - g_clear_pointer(&priv->initThread, g_free); - } - if (priv->udevThread) { - virThreadJoin(priv->udevThread); - g_clear_pointer(&priv->udevThread, g_free); - } + g_clear_pointer(&priv->initThread, g_free); + g_clear_pointer(&priv->udevThread, g_free); } virObjectUnref(priv); @@ -2440,12 +2430,52 @@ static virConnectDriver udevConnectDriver = { .nodeDeviceDriver = &udevNodeDeviceDriver, }; +static int +nodeStateShutdownPrepare(void) +{ + udevEventData *priv = NULL; + + if (!driver) + return 0; + + priv = driver->privateData; + if (!priv) + return 0; + + VIR_WITH_OBJECT_LOCK_GUARD(priv) { + priv->udevThreadQuit = true; + virCondSignal(&priv->udevThreadCond); + } + return 0; +} + +static int +nodeStateShutdownWait(void) +{ + udevEventData *priv = NULL; + + if (!driver) + return 0; + + priv = driver->privateData; + if (!priv) + return 0; + + if (priv->initThread) + virThreadJoin(priv->initThread); + + if (priv->udevThread) + virThreadJoin(priv->udevThread); + return 0; +} static virStateDriver udevStateDriver = { .name = "udev", .stateInitialize = nodeStateInitialize, /* 0.7.3 */ .stateCleanup = nodeStateCleanup, /* 0.7.3 */ .stateReload = nodeStateReload, /* 0.7.3 */ + .stateShutdownPrepare = nodeStateShutdownPrepare, + .stateShutdownWait = nodeStateShutdownWait, }; -- 2.34.1

On 4/12/24 8:36 AM, Marc Hartmayer wrote: Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt;

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. + 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(a)linux.ibm.com&gt; --- 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; } 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, + NODE_DEVICE_EVENT_UPDATE, + + NODE_DEVICE_EVENT_LAST +} nodeDeviceEventType; + +struct _nodeDeviceEvent { + nodeDeviceEventType eventType; + void *data; +}; +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(); + 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; } -- 2.34.1

On 4/12/24 8:36 AM, Marc Hartmayer wrote: 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. So far, all of the changes have only been related to the change of the global driver variable. These events are from the udev subsystem, so I think it would be nicer to name them such. NODE_DEVICE_EVENT_UDEV_ADD, etc. 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. 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; } ... 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);

When an udev event occurs the mdev active config data requires an update via mdevctl as the udev does not contain all config data. This update needs to occur immediate and to be finished before the libvirt nodedev event is issued to keep the API usage reliable. The only case where a direct `nodeDeviceUpdateMediatedDevices` is not wished is `mdevctlEventHandleCallback` - see commit 2c57b28191b9 ("nodedev: Refresh mdev devices when changes are detected") for details, but for this case there are no nodedev events created so the problem described above does not exist. `udevAddOneDevice` and `udevRemoveOneDeviceSysPath` are only called by the worker pool threads therefore it's possible to call the `nodeDeviceUpdateMediatedDevices` directly without blocking the udev thread. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 2a252d8fe62b..9282afdd3241 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1520,9 +1520,6 @@ udevGetDeviceDetails(virNodeDeviceDriverState *driver_state, struct udev_device } -static void scheduleMdevctlUpdate(udevEventData *data, bool force); - - static int processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char *path) { @@ -1556,9 +1553,8 @@ processNodeDeviceRemoveEvent(virNodeDeviceDriverState *driver_state, const char virNodeDeviceObjEndAPI(&obj); /* cannot check for mdev_types since they have already been removed */ - VIR_WITH_OBJECT_LOCK_GUARD(driver->privateData) { - scheduleMdevctlUpdate(driver_state->privateData, false); - } + if (nodeDeviceUpdateMediatedDevices(driver_state) < 0) + VIR_WARN("mdevctl failed to update mediated devices"); virObjectEventStateQueue(driver_state->nodeDeviceEventState, event); return 0; @@ -1682,15 +1678,9 @@ processNodeDeviceAddAndChangeEvent(virNodeDeviceDriverState *driver_state, struc has_mdev_types = virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_MDEV_TYPES); virNodeDeviceObjEndAPI(&obj); - if (has_mdev_types) { - 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(driver_state) < 0)) { + if ((is_mdev || has_mdev_types) && (nodeDeviceUpdateMediatedDevices(driver_state) < 0)) { VIR_WARN("Update of mediated device %s failed", NULLSTR_EMPTY(sysfs_path)); } -- 2.34.1

There is no reason to serialize the `mdevctl list` calls. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 9282afdd3241..77c35f981b66 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -70,8 +70,7 @@ struct _udevEventData { bool udevThreadQuit; bool udevDataReady; - /* Protects @mdevctlMonitors and must be taken when `mdevctl` command is - * called to make sure only one thread can query mdevctl at a time. */ + /* Protects @mdevctlMonitors */ virMutex mdevctlLock; GList *mdevctlMonitors; int mdevctlTimeout; @@ -2318,10 +2317,6 @@ static void nodeDeviceEventHandler(void *data, void *opaque) 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"); } -- 2.34.1

There is only one case where force is true, therefore let's inline that case. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; --- src/node_device/node_device_udev.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 77c35f981b66..0c566ed3824d 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2218,21 +2218,14 @@ 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, - * the worker job is submitted immediately. */ + * quick succession can be collapsed into a single update. */ static void -scheduleMdevctlUpdate(udevEventData *data, - bool force) +scheduleMdevctlUpdate(udevEventData *data) { - if (!force) { - if (data->mdevctlTimeout != -1) - virEventRemoveTimeout(data->mdevctlTimeout); - data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, - data, NULL); - return; - } - - submitMdevctlUpdate(-1, data); + if (data->mdevctlTimeout != -1) + virEventRemoveTimeout(data->mdevctlTimeout); + data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, + data, NULL); } @@ -2267,7 +2260,11 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, * CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the * signal if that event never comes */ VIR_WITH_OBJECT_LOCK_GUARD(priv) { - scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT)); + if (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) { + submitMdevctlUpdate(-1, priv); + } else { + scheduleMdevctlUpdate(priv); + } } } -- 2.34.1