On Tue, 16 Feb 2021 13:37:41 +0100
Erik Skultety <eskultet@xxxxxxxxxx> wrote:
> On Wed, Feb 03, 2021 at 11:38:53AM -0600, Jonathon Jongsma wrote:
> > At startup, query devices that are defined by 'mdevctl' and add
> > them to the node device list.
> >
> > This adds a complication: we now have two potential sources of
> > information for a node device:
> > - udev for all devices and for activated mediated devices
> > - mdevctl for persistent mediated devices
> >
> > Unfortunately, neither backend returns full information for a
> > mediated device. For example, if a persistent mediated device in
> > the list (with information provided from mdevctl) is 'started',
> > that same device will now be detected by udev. If we simply
> > overwrite the existing device definition with the new one provided
> > by the udev backend, we will lose extra information that was
> > provided by mdevctl (e.g. attributes, etc). To avoid this, make
> > sure to copy the extra information into the new device definition.
> >
> > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > ---
> > src/node_device/node_device_driver.c | 164
> > +++++++++++++++++++++++++++ src/node_device/node_device_driver.h |
> > 6 + src/node_device/node_device_udev.c | 31 ++++-
> > 3 files changed, 196 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/node_device/node_device_driver.c
> > b/src/node_device/node_device_driver.c index 2778e41f97..fd57dcacc1
> > 100644 --- a/src/node_device/node_device_driver.c
> > +++ b/src/node_device/node_device_driver.c
> > @@ -1156,3 +1156,167 @@ nodeDeviceGenerateName(virNodeDeviceDefPtr
> > def, *(def->name + i) = '_';
> > }
> > }
> > +
> > +
> > +static int
> > +virMdevctlListDefined(virNodeDeviceDefPtr **devs)
> > +{
> > + int status;
> > + g_autofree char *output = NULL;
> > + g_autoptr(virCommand) cmd =
> > nodeDeviceGetMdevctlListCommand(true, &output); +
> > + if (virCommandRun(cmd, &status) < 0)
> > + return -1;
> > +
> > + if (!output)
> > + return -1;
> > +
> > + return nodeDeviceParseMdevctlJSON(output, devs);
> > +}
> > +
> > +
> > +typedef struct _virMdevctlForEachData virMdevctlForEachData;
> > +struct _virMdevctlForEachData {
> > + int ndefs;
> > + virNodeDeviceDefPtr *defs;
> > +};
> > +
>
> ^This struct doesn't seem to be used anywhere in the patch, so please
> define it in the patch that makes use of it.
>
> > +
> > +int
> > +nodeDeviceUpdateMediatedDevices(void)
> > +{
> > + g_autofree virNodeDeviceDefPtr *defs = NULL;
> > + int ndefs;
> > + GList * tofree = NULL;
> > + size_t i;
> > +
> > + if ((ndefs = virMdevctlListDefined(&defs)) < 0) {
> > + virReportSystemError(errno, "%s",
> > + _("failed to query mdevs from
> > mdevctl"));
> > + return -1;
> > + }
> > +
> > + for (i = 0; i < ndefs; i++) {
> > + virNodeDeviceObjPtr obj;
> > + virObjectEventPtr event;
> > + virNodeDeviceDefPtr def = defs[i];
> > + bool was_defined = false;
> > +
> > + def->driver = g_strdup("vfio_mdev");
> > +
> > + if ((obj = virNodeDeviceObjListFindByName(driver->devs,
> > def->name))) {
>
> ^This condition should be flipped so that the 'else' block
> becomes the larger one.
>
> > + bool changed;
> > + virNodeDeviceDefPtr olddef =
> > virNodeDeviceObjGetDef(obj); +
> > + was_defined = virNodeDeviceObjIsPersistent(obj);
>
> "defined" as a name will do just fine
>
> > + /* Active devices contain some additional information
> > (e.g. sysfs
> > + * path) that is not provided by mdevctl, so re-use
> > the existing
> > + * definition and copy over new mdev data */
> > + changed = nodeDeviceDefCopyFromMdevctl(olddef, def);
> > +
> > + /* Re-use the existing definition (after merging new
> > data from
> > + * mdevctl), so def needs to be freed at the end of
> > the function */
> > + tofree = g_list_prepend(tofree, def);
>
> I'm not a fan of ^this. Plain g_autoptr on virNodeDeviceDef would
> do just fine.
I'm not a fan of it either, but g_autoptr is not ideal here either.
The def must not be freed if it is a new device (because
virNodeDeviceObjListAssignDef() takes ownership of the def). So we'd
have to make sure we steal the autoptr in that case. But we have to use
it after we'd need to clear it (in order to emit the lifecycle event,
etc).
But there's also another reason I took this approach. In patch 11
("handle mdevs that disappear..."), we need to use the entire list of
devices ('defs') to scan for devices that have disappeared. So I had
to keep the entire list of defs until the end of the function where we
traversed the list looking for removed devices to emit a lifecycle
event. I could potentially move this check to the beginning of the
function to get around the need to keep the defs around until the end
of the function, though...
I can try to rework it if you think it necessary.
>
> > +
> > + if (was_defined && !changed) {
> > + /* if this device was already defined and the
> > definition
> > + * hasn't changed, there's nothing to do for this
> > device */
> > + virNodeDeviceObjEndAPI(&obj);
> > + continue;
> > + }
> > + } else {
> > + if (!(obj =
> > virNodeDeviceObjListAssignDef(driver->devs, def))) {
> > + virNodeDeviceDefFree(def);
> > + goto cleanup;
> > + }
> > + }
> > +
> > + /* all devices returned by virMdevctlListDefined() are
> > persistent */
> > + virNodeDeviceObjSetPersistent(obj, true);
> > +
> > + if (!was_defined)
> > + event = virNodeDeviceEventLifecycleNew(def->name,
> > +
> > VIR_NODE_DEVICE_EVENT_DEFINED,
> > + 0);
> > + else
> > + event = virNodeDeviceEventUpdateNew(def->name);
> > +
> > + virNodeDeviceObjEndAPI(&obj);
> > + virObjectEventStateQueue(driver->nodeDeviceEventState,
> > event);
> > + }
> > +
> > + cleanup:
> > + g_list_free_full(tofree, (GDestroyNotify)virNodeDeviceDefFree);
> > +
> > + return 0;
> > +}
> > +
> > +
> > +/* returns true if any attributes were copied, else returns false
> > */ +static bool
> > +virMediatedDeviceAttrsCopy(virNodeDevCapMdevPtr dst,
> > + virNodeDevCapMdevPtr src)
> > +{
> > + bool ret = false;
> > + size_t i;
> > +
> > + if (src->nattributes != dst->nattributes) {
> > + ret = true;
> > + for (i = 0; i < dst->nattributes; i++)
> > + virMediatedDeviceAttrFree(dst->attributes[i]);
> > + g_free(dst->attributes);
> > +
> > + dst->nattributes = src->nattributes;
> > + dst->attributes = g_new0(virMediatedDeviceAttrPtr,
> > + src->nattributes);
> > + for (i = 0; i < dst->nattributes; i++)
> > + dst->attributes[i] = virMediatedDeviceAttrNew();
> > + }
> > +
> > + for (i = 0; i < src->nattributes; i++) {
> > + if (STRNEQ_NULLABLE(src->attributes[i]->name,
> > + dst->attributes[i]->name)) {
> > + ret = true;
> > + dst->attributes[i]->name =
> > + g_strdup(src->attributes[i]->name);
>
> ^This could leak memory if src->attributes == dst->attributes
> and the loop above therefore did not execute.
> > + }
> > + if (STRNEQ_NULLABLE(src->attributes[i]->value,
> > + dst->attributes[i]->value)) {
> > + ret = true;
> > + dst->attributes[i]->value =
> > + g_strdup(src->attributes[i]->value);
> > + }
> > + }
>
> I think we can just straight overwrite all the data in question, yes,
> some CPU cycles will be wasted, but time complexity remains at O(n)
> and readability improves significantly. These are mdev devices, so I
> doubt this is an RT critical feature. The function can then remain as
> void.
It's perhaps not obvious, but I specifically added these checks and
return value in order to avoid an issue that Daniel pointed out where I
was emitting update events for every device regardless of whether they
had changed or not. So rather than inventing a new function to check for
node device equality and only copy over the new data if they weren't
equal, I incorporated the equality check into the update function so
that I could check it and update it in the same call. See above where I
use the 'changed' variable in nodeDeviceUpdateMediatedDevices().
>
> > +
> > + return ret;
> > +}
> > +
> > +
> > +/* A mediated device definitions from mdevctl contains additional
> > info that is
> > + * not available from udev. Transfer this data to the new
> > definition.
> > + * Returns true if anything was copied, else returns false */
> > +bool
> > +nodeDeviceDefCopyFromMdevctl(virNodeDeviceDefPtr dst,
> > + virNodeDeviceDefPtr src)
> > +{
> > + bool ret = false;
> > + virNodeDevCapMdevPtr srcmdev = &src->caps->data.mdev;
> > + virNodeDevCapMdevPtr dstmdev = &dst->caps->data.mdev;
> > +
> > + if (STRNEQ_NULLABLE(dstmdev->type, srcmdev->type)) {
> > + ret = true;
> > + g_free(dstmdev->type);
> > + dstmdev->type = g_strdup(srcmdev->type);
> > + }
> > +
> > + if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) {
> > + ret = true;
> > + g_free(dstmdev->uuid);
> > + dstmdev->uuid = g_strdup(srcmdev->uuid);
> > + }
> > +
> > + if (virMediatedDeviceAttrsCopy(dstmdev, srcmdev))
> > + ret = true;
>
> Same as above, just overwrite all the data in question directly with
> those coming from mdevctl.
>
> > +
> > + return ret;
> > +}
> > diff --git a/src/node_device/node_device_driver.h
> > b/src/node_device/node_device_driver.h index 80ac7c5320..d8837e4ef8
> > 100644 --- a/src/node_device/node_device_driver.h
> > +++ b/src/node_device/node_device_driver.h
> > @@ -126,8 +126,14 @@ int
> > nodeDeviceParseMdevctlJSON(const char *jsonstring,
> > virNodeDeviceDefPtr **devs);
> >
> > +int
> > +nodeDeviceUpdateMediatedDevices(void);
> > +
> > void
> > nodeDeviceGenerateName(virNodeDeviceDefPtr def,
> > const char *subsystem,
> > const char *sysname,
> > const char *s);
> > +
> > +bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDefPtr dst,
> > + virNodeDeviceDefPtr src);
> > diff --git a/src/node_device/node_device_udev.c
> > b/src/node_device/node_device_udev.c index 5d1a192411..038941ec51
> > 100644 --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1416,9 +1416,17 @@ udevRemoveOneDeviceSysPath(const char *path)
> > VIR_NODE_DEVICE_EVENT_DELETED,
> > 0);
> >
> > - VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
> > - def->name, path);
> > - virNodeDeviceObjListRemove(driver->devs, obj);
> > + /* If the device is a mediated device that has been 'stopped',
> > it may still
> > + * be defined by mdevctl and can therefore be started again.
> > Don't drop it
> > + * from the list of node devices */
> > + if (virNodeDeviceObjIsPersistent(obj)) {
> > + VIR_FREE(def->sysfs_path);
> > + virNodeDeviceObjSetActive(obj, false);
> > + } else {
> > + VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
> > + def->name, path);
> > + virNodeDeviceObjListRemove(driver->devs, obj);
> > + }
> > virNodeDeviceObjEndAPI(&obj);
> >
> > virObjectEventStateQueue(driver->nodeDeviceEventState, event);
> > @@ -1476,7 +1484,6 @@ udevSetParent(struct udev_device *device,
> > return 0;
> > }
> >
> > -
> > static int
> > udevAddOneDevice(struct udev_device *device)
> > {
> > @@ -1486,6 +1493,7 @@ udevAddOneDevice(struct udev_device *device)
> > virObjectEventPtr event = NULL;
> > bool new_device = true;
> > int ret = -1;
> > + bool was_persistent = false;
> >
> > def = g_new0(virNodeDeviceDef, 1);
> >
> > @@ -1509,14 +1517,23 @@ udevAddOneDevice(struct udev_device *device)
> > goto cleanup;
> >
> > if ((obj = virNodeDeviceObjListFindByName(driver->devs,
> > def->name))) {
> > + objdef = virNodeDeviceObjGetDef(obj);
> > +
> > + if (def->caps->data.type == VIR_NODE_DEV_CAP_MDEV)
> > + nodeDeviceDefCopyFromMdevctl(def, objdef);
> > + was_persistent = virNodeDeviceObjIsPersistent(obj);
> > + /* If the device was defined by mdevctl and was never
> > instantiated, it
> > + * won't have a sysfs path. We need to emit a CREATED
> > event... */
>
> Why CREATED instead of DEFINED?
Because this is handling a udev event. udev only deals with with
instantiated mdevs, not persistent mdev definitions.
In other words, mdevctl determines whether a mdev is DEFINED or
UNDEFINED, and udev determines whether a devices is CREATED or DELETED.
>
> Erik
>
> > + new_device = (objdef->sysfs_path == NULL);
> > +
> > virNodeDeviceObjEndAPI(&obj);
> > - new_device = false;
> > }
> >
> > /* 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)))
> > goto cleanup;
> > + virNodeDeviceObjSetPersistent(obj, was_persistent);
> > objdef = virNodeDeviceObjGetDef(obj);
> >
> > if (new_device)
> > @@ -1935,6 +1952,10 @@ nodeStateInitializeEnumerate(void *opaque)
> > /* Populate with known devices */
> > if (udevEnumerateDevices(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)
> > + goto error;
> >
> > nodeDeviceLock();
> > driver->initialized = true;
> > --
> > 2.26.2
> >
