[libvirt] [PATCH] conf: nodedev: Update capabilities from within virNodeDeviceObjHasCap
Commit d18feadc0c put this into virNodeDeviceMatch, but this should have rather been part of virNodeDeviceObjHasCap from the beginning, since virNodeDeviceObjHasCap is the last helper to be called (in the calltree) whereas virNodeDeviceMatch is called from a single place only - virNodeDeviceObjListExportCallback. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/virnodedeviceobj.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ad0f27ee4..0417664ad 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -644,6 +644,10 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, { virNodeDevCapsDefPtr cap = NULL; + /* Refresh the capabilities first, e.g. due to a driver change */ + if (virNodeDeviceUpdateCaps(obj->def) < 0) + return false; + for (cap = obj->def->caps; cap; cap = cap->next) { if (type == cap->data.type) return true; @@ -811,10 +815,6 @@ static bool virNodeDeviceMatch(virNodeDeviceObjPtr obj, unsigned int flags) { - /* Refresh the capabilities first, e.g. due to a driver change */ - if (virNodeDeviceUpdateCaps(obj->def) < 0) - return false; - /* filter by cap type */ if (flags & VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP) { if (!(MATCH(SYSTEM) || -- 2.13.6
On 03/06/2018 10:32 AM, Erik Skultety wrote:
Commit d18feadc0c put this into virNodeDeviceMatch, but this should have rather been part of virNodeDeviceObjHasCap from the beginning, since virNodeDeviceObjHasCap is the last helper to be called (in the calltree) whereas virNodeDeviceMatch is called from a single place only - virNodeDeviceObjListExportCallback.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/virnodedeviceobj.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ad0f27ee4..0417664ad 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -644,6 +644,10 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, { virNodeDevCapsDefPtr cap = NULL;
+ /* Refresh the capabilities first, e.g. due to a driver change */ + if (virNodeDeviceUpdateCaps(obj->def) < 0) + return false; +
While I understand why you put it here - in order to be somewhere common for nodeDeviceCreateXML, nodeNumOfDevices, nodeListDevices, and nodeConnectListAllNodeDevices. However, I think this'll impact "performance" of virNodeDeviceMatch as virNodeDeviceUpdateCaps would be called 16 times due to the logic of "if !(MATCH() || MATCH() || MATCH() ...)))" Of course the alternative is calling virNodeDeviceUpdateCaps from nodeDeviceCreateXML, nodeNumOfDevices, nodeListDevices, and nodeConnectListAllNodeDevices. (or am I reading the !(a || b || c || ... ) logic wrong? John
for (cap = obj->def->caps; cap; cap = cap->next) { if (type == cap->data.type) return true; @@ -811,10 +815,6 @@ static bool virNodeDeviceMatch(virNodeDeviceObjPtr obj, unsigned int flags) { - /* Refresh the capabilities first, e.g. due to a driver change */ - if (virNodeDeviceUpdateCaps(obj->def) < 0) - return false; - /* filter by cap type */ if (flags & VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP) { if (!(MATCH(SYSTEM) ||
On Wed, Mar 14, 2018 at 03:28:36PM -0400, John Ferlan wrote:
On 03/06/2018 10:32 AM, Erik Skultety wrote:
Commit d18feadc0c put this into virNodeDeviceMatch, but this should have rather been part of virNodeDeviceObjHasCap from the beginning, since virNodeDeviceObjHasCap is the last helper to be called (in the calltree) whereas virNodeDeviceMatch is called from a single place only - virNodeDeviceObjListExportCallback.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/virnodedeviceobj.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ad0f27ee4..0417664ad 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -644,6 +644,10 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, { virNodeDevCapsDefPtr cap = NULL;
+ /* Refresh the capabilities first, e.g. due to a driver change */ + if (virNodeDeviceUpdateCaps(obj->def) < 0) + return false; +
While I understand why you put it here - in order to be somewhere common for nodeDeviceCreateXML, nodeNumOfDevices, nodeListDevices, and nodeConnectListAllNodeDevices.
However, I think this'll impact "performance" of virNodeDeviceMatch as virNodeDeviceUpdateCaps would be called 16 times due to the logic of "if !(MATCH() || MATCH() || MATCH() ...)))"
Of course the alternative is calling virNodeDeviceUpdateCaps from nodeDeviceCreateXML, nodeNumOfDevices, nodeListDevices, and nodeConnectListAllNodeDevices.
(or am I reading the !(a || b || c || ... ) logic wrong?
No, you're right, unless you don't specify any flags yourself in ListAllNodeDevices, it will try to match everything. Hmm, so as I responded to [1], the reason why I put the update where I did was to avoid having to collect the list of objs twice (resulting in 3xO(n)), once to update every single object's flags and then to actually filter the objs into the final list to be returned to the caller, so depends what you think is going to be a bigger hit for performance. Of course, I can revert it back and go with that solution which would make things easier for Cole I guess or there's another option (apart from acknowledging that there's going to be a certain performance hit either way - going with this patch or updating the devices before actually proceeding with and API) to continue with what Cole's suggested in his [1] series, add a 'update' bool to the virNodeDeviceObj structure, set it to true at the beginning of virNodeDeviceMatch, let the first MATCH's virNodeDeviceObjHasCap call update it, set it back to false, which means that a single obj will always be updated once in the span of a single API call. Actually, there's one more way, we could revert this patch, leave the update in virNodeDeviceMatch and add the update call to virNodeDeviceObjHasCapStr and we should be settled for all the APIs, new and legacy ones. Let me know what your thoughts on my proposals are. Erik [1] https://www.redhat.com/archives/libvir-list/2018-February/msg01136.html
On 03/15/2018 04:35 AM, Erik Skultety wrote:
On Wed, Mar 14, 2018 at 03:28:36PM -0400, John Ferlan wrote:
On 03/06/2018 10:32 AM, Erik Skultety wrote:
Commit d18feadc0c put this into virNodeDeviceMatch, but this should have rather been part of virNodeDeviceObjHasCap from the beginning, since virNodeDeviceObjHasCap is the last helper to be called (in the calltree) whereas virNodeDeviceMatch is called from a single place only - virNodeDeviceObjListExportCallback.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/virnodedeviceobj.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ad0f27ee4..0417664ad 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -644,6 +644,10 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, { virNodeDevCapsDefPtr cap = NULL;
+ /* Refresh the capabilities first, e.g. due to a driver change */ + if (virNodeDeviceUpdateCaps(obj->def) < 0) + return false; +
While I understand why you put it here - in order to be somewhere common for nodeDeviceCreateXML, nodeNumOfDevices, nodeListDevices, and nodeConnectListAllNodeDevices.
However, I think this'll impact "performance" of virNodeDeviceMatch as virNodeDeviceUpdateCaps would be called 16 times due to the logic of "if !(MATCH() || MATCH() || MATCH() ...)))"
Of course the alternative is calling virNodeDeviceUpdateCaps from nodeDeviceCreateXML, nodeNumOfDevices, nodeListDevices, and nodeConnectListAllNodeDevices.
(or am I reading the !(a || b || c || ... ) logic wrong?
No, you're right, unless you don't specify any flags yourself in ListAllNodeDevices, it will try to match everything.> Hmm, so as I responded to [1], the reason why I put the update where I did was to avoid having to collect the list of objs twice (resulting in 3xO(n)), once to
Yeah I had re-read (mostly scanned) Cole's series before sending this response...
update every single object's flags and then to actually filter the objs into the final list to be returned to the caller, so depends what you think is going to be a bigger hit for performance. Of course, I can revert it back and go with that solution which would make things easier for Cole I guess or there's another option (apart from acknowledging that there's going to be a certain performance hit either way - going with this patch or updating the devices before actually proceeding with and API) to continue with what Cole's suggested in his [1] series, add a 'update' bool to the virNodeDeviceObj structure, set it to true at the beginning of virNodeDeviceMatch, let the first MATCH's virNodeDeviceObjHasCap call update it, set it back to false, which means that a single obj will always be updated once in the span of a single API call.
To a degree that has some merit, albeit a bit of a hack.
Actually, there's one more way, we could revert this patch, leave the update in virNodeDeviceMatch and add the update call to virNodeDeviceObjHasCapStr and we should be settled for all the APIs, new and legacy ones.
This probably is closer... But why leave the call in Match which will get called virHashForEach from virNodeDeviceObjListExportCallback. Perhaps more appropriate to perform the UpdateCaps from ListExport since I doubt much changes ForEach nodedev in the hash table while we're exporting the list. As for virNodeDeviceObjHasCapStr - sure that works, but then again that's called by various Callback API's as part of virHashSearch or virHashForEach calls. IOW it's called multiple times for what purpose? I think we just need to make the UpdateCaps call once before culling the ObjList via Search for ForEach. Of course that brings us back to perhaps we should only call UpdateCaps at the driver level API's instead of the conf level. John As an aside, IIRC it seems we're in this dilemma because we've found the mdev's have updates that don't come through the change event. Ironically UpdateCaps doesn't have a case for CAP_MDEV instead it uses CAP_PCI_DEV in order to call virNodeDeviceGetPCIMdevTypesCaps. Yes, I know the relationship. Also note that the virNodeDeviceGetPCIDynamicCaps has a comment that indicates "These must be refreshed anytime full XML of the device is requested, because they can change with no corresponding notification from the kernel/udev." - it doesn't indicate we need to also perform this update for all the various ways to count or capture nodedev names/devices although I suppose that could have been part of some commit message that I didn't look at.
Let me know what your thoughts on my proposals are. Erik
[1] https://www.redhat.com/archives/libvir-list/2018-February/msg01136.html
participants (2)
-
Erik Skultety -
John Ferlan