conf: nodedev: Update capabilities from within virNodeDeviceObjHasCap

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

Posted by Erik Skultety 6 years, 1 month ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 6 years, 1 month ago


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 6 years, 1 month ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 6 years, 1 month ago

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
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list