[libvirt] [PATCH v3 0/3] nodedev: update caps before quering nodedev infos
Some capabilities of node devices rely on what driver they bound to, and therefore, these capabilities may change when the driver change. In current implemention, it is not consistent between real status and the status we get by invoking nodedev interfaces. So, this series of patches try to manually update devices' capabilities each time before nodedev driver interfaces invoked. Wu Zongyong (3): v2->v3: 1. split a single patch to three part 2. fix memory leak and lock problems 3. update caps before invoking nodedev driver interfaces (refer to patch 3/3) nodedev: add macro guard to node_device_udev header file nodedev: update mdev_types caps before dumpxml nodedev: update caps before invoking nodedev driver interfaces src/node_device/node_device_driver.c | 55 +++++++++++++++++++++++++++++++ src/node_device/node_device_linux_sysfs.c | 22 +++++++++++++ src/node_device/node_device_udev.c | 37 +++++++++++++++++---- src/node_device/node_device_udev.h | 18 +++++++--- 4 files changed, 121 insertions(+), 11 deletions(-) -- 1.9.1
Signed-off-by: Wu Zongyong <cordius.wu@huawei.com> --- src/node_device/node_device_udev.h | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h index f15e520..300f57e 100644 --- a/src/node_device/node_device_udev.h +++ b/src/node_device/node_device_udev.h @@ -19,10 +19,15 @@ * * Author: Dave Allan <dallan@redhat.com> */ +#ifndef __VIR_NODE_DEVICE_UDEV_H__ +# define __VIR_NODE_DEVICE_UDEV_H__ -#include <libudev.h> -#include <stdint.h> +# include <libudev.h> +# include <stdint.h> -#define SYSFS_DATA_SIZE 4096 -#define DMI_DEVPATH "/sys/devices/virtual/dmi/id" -#define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id" +# define SYSFS_DATA_SIZE 4096 +# define DMI_DEVPATH "/sys/devices/virtual/dmi/id" +# define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id" + + +#endif /* __VIR_NODE_DEVICE_UDEV_H__ */ -- 1.9.1
In current implemention, mdev_types caps keep constant all the time. But, it is possible that a device capable of mdev_types sometime(for example:bind to proper driver) and incapable of mdev_types at other times(for example: unbind from its driver). We should keep the info of xml dumped out consistent with real status of the device. Signed-off-by: Wu Zongyong <cordius.wu@huawei.com> --- src/node_device/node_device_linux_sysfs.c | 22 ++++++++++++++++++ src/node_device/node_device_udev.c | 37 ++++++++++++++++++++++++++----- src/node_device/node_device_udev.h | 3 +++ 3 files changed, 56 insertions(+), 6 deletions(-) diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 6f438e5..388ffb4 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -29,6 +29,7 @@ #include "dirname.h" #include "node_device_driver.h" #include "node_device_hal.h" +#include "node_device_udev.h" #include "node_device_linux_sysfs.h" #include "virerror.h" #include "viralloc.h" @@ -175,6 +176,25 @@ nodeDeviceSysfsGetPCIIOMMUGroupCaps(virNodeDevCapPCIDevPtr pci_dev) return ret; } +static int +nodeDeviceSysfsGetPCIMdevTypesCaps(const char *sysfsPath, + virNodeDevCapPCIDevPtr pci_dev) +{ + size_t i; + + /* this could be a refresh, so clear out the old data */ + for (i = 0; i < pci_dev->nmdev_types; i++) + virNodeDevCapMdevTypeFree(pci_dev->mdev_types[i]); + VIR_FREE(pci_dev->mdev_types); + pci_dev->nmdev_types = 0; + pci_dev->flags &= ~VIR_NODE_DEV_CAP_FLAG_PCI_MDEV; + + if (udevPCISysfsGetMdevTypesCap(sysfsPath, pci_dev) < 0) + return -1; + + return 0; +} + /* nodeDeviceSysfsGetPCIRelatedCaps() get info that is stored in sysfs * about devices related to this device, i.e. things that can change @@ -190,6 +210,8 @@ nodeDeviceSysfsGetPCIRelatedDevCaps(const char *sysfsPath, return -1; if (nodeDeviceSysfsGetPCIIOMMUGroupCaps(pci_dev) < 0) return -1; + if (nodeDeviceSysfsGetPCIMdevTypesCaps(sysfsPath, pci_dev) < 0) + return -1; return 0; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e0fca61..7b0014e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -506,6 +506,37 @@ udevPCIGetMdevTypesCap(struct udev_device *device, } +int +udevPCISysfsGetMdevTypesCap(const char *sysfsPath, + virNodeDevCapPCIDevPtr pci_dev) +{ + int ret = -1; + struct udev *udev = NULL; + struct udev_device *device = NULL; + udevEventDataPtr priv = driver->privateData; + + virObjectLock(priv); + udev = udev_monitor_get_udev(priv->udev_monitor); + device = udev_device_new_from_syspath(udev, sysfsPath); + virObjectUnlock(priv); + + if (!device) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to create udev device from path %s"), + sysfsPath); + goto cleanup; + } + + if (udevPCIGetMdevTypesCap(device, pci_dev) < 0) + goto cleanup; + + ret = 0; + cleanup: + udev_device_unref(device); + return ret; +} + + static int udevProcessPCI(struct udev_device *device, virNodeDeviceDefPtr def) @@ -597,12 +628,6 @@ udevProcessPCI(struct udev_device *device, } } - /* check whether the device is mediated devices framework capable, if so, - * process it - */ - if (udevPCIGetMdevTypesCap(device, pci_dev) < 0) - goto cleanup; - ret = 0; cleanup: diff --git a/src/node_device/node_device_udev.h b/src/node_device/node_device_udev.h index 300f57e..bbdc70f 100644 --- a/src/node_device/node_device_udev.h +++ b/src/node_device/node_device_udev.h @@ -29,5 +29,8 @@ # define DMI_DEVPATH "/sys/devices/virtual/dmi/id" # define DMI_DEVPATH_FALLBACK "/sys/class/dmi/id" +int +udevPCISysfsGetMdevTypesCap(const char *sysfsPath, virNodeDevCapPCIDevPtr pci_dev); + #endif /* __VIR_NODE_DEVICE_UDEV_H__ */ -- 1.9.1
On Wed, Jan 10, 2018 at 08:14:50PM +0800, Wu Zongyong wrote:
In current implemention, mdev_types caps keep constant all the time. But, it is possible that a device capable of mdev_types sometime(for example:bind to proper driver) and incapable of mdev_types at other times(for example: unbind from its driver). We should keep the info of xml dumped out consistent with real status of the device.
Signed-off-by: Wu Zongyong <cordius.wu@huawei.com>
For patches 1 and 2 (I'll tune the commit message for this one a bit before pushing): Reviewed-by: Erik Skultety <eskultet@redhat.com>
Would you push this two patches before release of 4.0.0? Thanks, Zongyong Wu
-----Original Message----- From: Erik Skultety [mailto:eskultet@redhat.com] Sent: Thursday, January 11, 2018 6:07 PM To: Wuzongyong (Euler Dept) <cordius.wu@huawei.com> Cc: libvir-list@redhat.com; weijinfen <weijinfen@huawei.com>; Wanzongshun (Vincent) <wanzongshun@huawei.com> Subject: Re: [PATCH 2/3] nodedev: update mdev_types caps before dumpxml
On Wed, Jan 10, 2018 at 08:14:50PM +0800, Wu Zongyong wrote:
In current implemention, mdev_types caps keep constant all the time. But, it is possible that a device capable of mdev_types sometime(for example:bind to proper driver) and incapable of mdev_types at other times(for example: unbind from its driver). We should keep the info of xml dumped out consistent with real status of the device.
Signed-off-by: Wu Zongyong <cordius.wu@huawei.com>
For patches 1 and 2 (I'll tune the commit message for this one a bit before pushing):
Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Thu, Jan 18, 2018 at 02:09:14AM +0000, Wuzongyong (Euler Dept) wrote:
Would you push this two patches before release of 4.0.0?
Well, I don't think it's that critical, in fact I think we can easily wait until 4.1, especially since we're in RC2, I'd rather not push anything unnecessary just before the release, hope you can understand that. Thanks, Erik
Some capabilities of node devices rely on what driver they bound to, and therefore, these capabilities may change when the driver change. So, it is necessary to manually update devices' capabilities each time before nodedev driver interfaces invoked. Signed-off-by: Wu Zongyong <cordius.wu@huawei.com> --- src/node_device/node_device_driver.c | 55 ++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index facfeb6..d854516 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -155,6 +155,42 @@ nodeDeviceUpdateDriverName(virNodeDeviceDefPtr def ATTRIBUTE_UNUSED) #endif +static int +nodeConnectUpdateAllNodeDevicesCaps(virConnectPtr conn, + virNodeDeviceObjListFilter filter) +{ + int ret = -1; + size_t i; + virNodeDevicePtr *devices; + + if (virNodeDeviceObjListExport(conn, driver->devs, &devices, filter, 0) < 0) + return -1; + + for (i = 0; devices[i]; i++) { + virNodeDeviceObjPtr obj; + virNodeDeviceDefPtr def; + + if (!(obj = virNodeDeviceObjListFindByName(driver->devs, devices[i]->name))) + goto cleanup; + def = virNodeDeviceObjGetDef(obj); + + if (nodeDeviceUpdateCaps(def) < 0) { + virNodeDeviceObjEndAPI(&obj); + goto cleanup; + } + + virNodeDeviceObjEndAPI(&obj); + } + + ret = 0; + cleanup: + for (i = 0; devices[i]; i++) + virObjectUnref(devices[i]); + VIR_FREE(devices); + return ret; +} + + void nodeDeviceLock(void) { @@ -179,6 +215,9 @@ nodeNumOfDevices(virConnectPtr conn, virCheckFlags(0, -1); + if (nodeConnectUpdateAllNodeDevicesCaps(conn, virNodeNumOfDevicesCheckACL) < 0) + return -1; + return virNodeDeviceObjListNumOfDevices(driver->devs, conn, cap, virNodeNumOfDevicesCheckACL); } @@ -196,6 +235,9 @@ nodeListDevices(virConnectPtr conn, virCheckFlags(0, -1); + if (nodeConnectUpdateAllNodeDevicesCaps(conn, virNodeListDevicesCheckACL) < 0) + return -1; + return virNodeDeviceObjListGetNames(driver->devs, conn, virNodeListDevicesCheckACL, cap, names, maxnames); @@ -212,6 +254,10 @@ nodeConnectListAllNodeDevices(virConnectPtr conn, if (virConnectListAllNodeDevicesEnsureACL(conn) < 0) return -1; + if (nodeConnectUpdateAllNodeDevicesCaps(conn, + virConnectListAllNodeDevicesCheckACL) < 0) + return -1; + return virNodeDeviceObjListExport(conn, driver->devs, devices, virConnectListAllNodeDevicesCheckACL, flags); @@ -248,6 +294,9 @@ nodeDeviceLookupByName(virConnectPtr conn, if (virNodeDeviceLookupByNameEnsureACL(conn, def) < 0) goto cleanup; + if (nodeDeviceUpdateCaps(def) < 0) + goto cleanup; + if ((device = virGetNodeDevice(conn, name))) { if (VIR_STRDUP(device->parent, def->parent) < 0) { virObjectUnref(device); @@ -370,6 +419,9 @@ nodeDeviceNumOfCaps(virNodeDevicePtr device) if (virNodeDeviceNumOfCapsEnsureACL(device->conn, def) < 0) goto cleanup; + if (nodeDeviceUpdateCaps(def) < 0) + goto cleanup; + for (caps = def->caps; caps; caps = caps->next) { ++ncaps; @@ -411,6 +463,9 @@ nodeDeviceListCaps(virNodeDevicePtr device, if (virNodeDeviceListCapsEnsureACL(device->conn, def) < 0) goto cleanup; + if (nodeDeviceUpdateCaps(def) < 0) + goto cleanup; + for (caps = def->caps; caps && ncaps < maxnames; caps = caps->next) { if (VIR_STRDUP(names[ncaps++], virNodeDevCapTypeToString(caps->data.type)) < 0) goto cleanup; -- 1.9.1
On Wed, Jan 10, 2018 at 08:14:51PM +0800, Wu Zongyong wrote:
Some capabilities of node devices rely on what driver they bound to, and therefore, these capabilities may change when the driver change. So, it is necessary to manually update devices' capabilities each time before nodedev driver interfaces invoked.
Signed-off-by: Wu Zongyong <cordius.wu@huawei.com> ---
Thank you for posting the patch, since I hadn't noticed the problem with other APIs until I read it. It was a sad realization that a driver change is not reflected by a udev/kernel CHANGE event, that would make things much much simpler. I have an idea to either make this patch shorter or not needed at all though, we'll see when I finish my patch (it's a long-needed refactor). Despite I haven't found any major flaws in this patch, let's just put it on hold for a while until I finish my investigation/work on my patch and see we can really do better here. Thanks, Erik
It is a bit inefficient about my patch, and it is imperfect. I wish to see a better idea to fix this problem. And again, thank you for your correction to my patches. Thanks, Zongyong Wu
-----Original Message----- From: Erik Skultety [mailto:eskultet@redhat.com] Sent: Thursday, January 11, 2018 6:16 PM To: Wuzongyong (Euler Dept) <cordius.wu@huawei.com> Cc: libvir-list@redhat.com; weijinfen <weijinfen@huawei.com>; Wanzongshun (Vincent) <wanzongshun@huawei.com> Subject: Re: [PATCH 3/3] nodedev: update caps before invoking nodedev driver interfaces
On Wed, Jan 10, 2018 at 08:14:51PM +0800, Wu Zongyong wrote:
Some capabilities of node devices rely on what driver they bound to, and therefore, these capabilities may change when the driver change. So, it is necessary to manually update devices' capabilities each time before nodedev driver interfaces invoked.
Signed-off-by: Wu Zongyong <cordius.wu@huawei.com> ---
Thank you for posting the patch, since I hadn't noticed the problem with other APIs until I read it. It was a sad realization that a driver change is not reflected by a udev/kernel CHANGE event, that would make things much much simpler. I have an idea to either make this patch shorter or not needed at all though, we'll see when I finish my patch (it's a long-needed refactor). Despite I haven't found any major flaws in this patch, let's just put it on hold for a while until I finish my investigation/work on my patch and see we can really do better here.
Thanks, Erik
On Wed, Jan 10, 2018 at 08:14:48PM +0800, Wu Zongyong wrote:
Some capabilities of node devices rely on what driver they bound to, and therefore, these capabilities may change when the driver change. In current implemention, it is not consistent between real status and the status we get by invoking nodedev interfaces. So, this series of patches try to manually update devices' capabilities each time before nodedev driver interfaces invoked.
Hi, so I finally posted the series I mentioned in my previous reviews, unfortunately, after applying your patches there were a lot of merge conflicts so I had to write them from scratch, so I at least added you as an honorable mention in patch equivalents of your patches 2 and 3. Unless someone objects to the series as a whole, your patch 1 will be unnecessary. Feel free to have a look at the series [1] and test whether it works as expected (it did in my testing, both virsh and python). Erik [1] https://www.redhat.com/archives/libvir-list/2018-January/msg00851.html
Wu Zongyong (3):
v2->v3: 1. split a single patch to three part 2. fix memory leak and lock problems 3. update caps before invoking nodedev driver interfaces (refer to patch 3/3)
nodedev: add macro guard to node_device_udev header file nodedev: update mdev_types caps before dumpxml nodedev: update caps before invoking nodedev driver interfaces
src/node_device/node_device_driver.c | 55 +++++++++++++++++++++++++++++++ src/node_device/node_device_linux_sysfs.c | 22 +++++++++++++ src/node_device/node_device_udev.c | 37 +++++++++++++++++---- src/node_device/node_device_udev.h | 18 +++++++--- 4 files changed, 121 insertions(+), 11 deletions(-)
-- 1.9.1
participants (3)
-
Erik Skultety -
Wu Zongyong -
Wuzongyong (Euler Dept)