[libvirt] [PATCH] conf: nodedev: Update capabilities from within virNodeDeviceObjHasCap

[John Ferlan]

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