[libvirt] [PATCHv2] nodedev: add switchdev to NIC capabilities

[John Ferlan]

On 09/18/2017 10:57 AM, Ján Tomko wrote:
> On Thu, Sep 14, 2017 at 11:23:02AM -0400, Laine Stump wrote:
>> (Almost all of my comments result in "ok, no action needed". There are
>> just three items I would like to see changed (2 trivial, 1 also small
>> but Edan or John may think it prudent to re-test with the change before
>> pushing) - I marked those comments with [**].
>>
>> On 08/21/2017 05:19 AM, Edan David wrote:
>>> Adding functionality to libvirt that will allow querying the interface
>>> for the availability of switchdev Offloading NIC capabilities.
>>> The switchdev mode was introduced in kernel 4.8, the iproute2-devlink
>>> command to retrieve the swtichdev NIC feature,
>>> Command example:  devlink dev eswitch show pci/0000:03:00.0
>>> This feature is needed for Openstack so we can do a scheduling decision
>>> if the NIC is in Hardware Offload (switchdev) or regular SR-IOV
>>> (legacy) mode.
>>> And select the appropriate hypervisors with the requested capability
>>> see [1].
>>>
>>> [1] -
>>> https://specs.openstack.org/openstack/nova-specs/specs/pike/approved/enable-sriov-nic-features.html
>>>
>>> ---
>>>  configure.ac                                      |  13 ++
>>>  docs/formatnode.html.in                           |   1 +
>>>  src/util/virnetdev.c                              | 187
>>> +++++++++++++++++++++-
>>>  src/util/virnetdev.h                              |   1 +
>>>  tests/nodedevschemadata/net_00_13_02_b9_f9_d3.xml |   1 +
>>>  tests/nodedevschemadata/net_00_15_58_2f_e9_55.xml |   1 +
>>>  6 files changed, 203 insertions(+), 1 deletion(-)
>>>
> 
> This patch fails to compile on a Gentoo machine with
> sys-kernel/linux-headers-4.8:
> 
> util/virnetdev.c:3248:18: error: use of undeclared identifier
> 'DEVLINK_CMD_ESWITCH_GET'; did you mean 'DEVLINK_CMD_ESWITCH_MODE_GET'?
>    gmsgh->cmd = DEVLINK_CMD_ESWITCH_GET;
>                 ^~~~~~~~~~~~~~~~~~~~~~~
>                 DEVLINK_CMD_ESWITCH_MODE_GET
> /usr/include/linux/devlink.h:60:2: note: 'DEVLINK_CMD_ESWITCH_MODE_GET'
> declared here
>        DEVLINK_CMD_ESWITCH_MODE_GET,
>        ^
> 1 error generated.
> 
>>> diff --git a/configure.ac b/configure.ac
>>> index b12b7fa..c089798 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -627,6 +627,19 @@ if test "$with_linux" = "yes"; then
>>>      AC_CHECK_HEADERS([linux/btrfs.h])
>>>  fi
>>>
>>> +dnl
>>> +dnl check for kernel headers required by devlink
>>> +dnl
>>> +if test "$with_linux" = "yes"; then
>>> +    AC_CHECK_HEADERS([linux/devlink.h])
>>> +    AC_CHECK_DECLS([DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME,
>>> DEVLINK_ATTR_MAX, DEVLINK_CMD_ESWITCH_GET, DEVLINK_ATTR_BUS_NAME,
>>> DEVLINK_ATTR_DEV_NAME, DEVLINK_ATTR_ESWITCH_MODE,
>>> DEVLINK_ESWITCH_MODE_SWITCHDEV],
>>
>> That's very ..... thorough, and potentially misleading if someone ever
>> wanted to use devlink to check for something other than switchdev (e.g.
>> [mythical feature X]) on some system that didn't have the proper stuff
>> defined for switchdev, but did have it for [other mythical feature X].
>> But that's all just hypothetical, so this is fine with me.
>>
>>
>>> +                   [AC_DEFINE([HAVE_DECL_DEVLINK],
>>> +                              [1],
>>> +                              [whether devlink declarations is
>>> available])],
>>
>> [**]
>> s/is/are/
>>
> 
> It seems the [action-if-found] is executed for every found symbol:
> 
> #define HAVE_DECL_DEVLINK_GENL_VERSION 1
> #define HAVE_DECL_DEVLINK 1
> #define HAVE_DECL_DEVLINK_GENL_NAME 1
> #define HAVE_DECL_DEVLINK 1
> #define HAVE_DECL_DEVLINK_ATTR_MAX 1
> #define HAVE_DECL_DEVLINK 1
> #define HAVE_DECL_DEVLINK_CMD_ESWITCH_GET 0
> #define HAVE_DECL_DEVLINK_ATTR_BUS_NAME 1
> #define HAVE_DECL_DEVLINK 1
> #define HAVE_DECL_DEVLINK_ATTR_DEV_NAME 1
> #define HAVE_DECL_DEVLINK 1
> #define HAVE_DECL_DEVLINK_ATTR_ESWITCH_MODE 1
> #define HAVE_DECL_DEVLINK 1
> #define HAVE_DECL_DEVLINK_ESWITCH_MODE_SWITCHDEV 1
> #define HAVE_DECL_DEVLINK 1
> 
> so this usage of AC_CHECK_DECLS is bogus.
> 
> Jan
> 

So then the question becomes is it "safe enough" to only look for
HAVE_DECL_DEVLINK_CMD_ESWITCH_GET and assume the others are present or
does there have to be some sort of AC_CHECK_DECLS for every one of the
symbols that are going to be used and then some way to only allow that
code to be compiled if all are true?

I'd say the former - just check for CMD_ESWITCH_GET and assume the
others, but you know what can be said about assumptions... And this is
why I didn't want to push this on Saturday!  bad enough that I've been
in and out of the office today ;-)

John

>>> +                   [],
>>> +                   [[#include <linux/devlink.h>]])
>>> +fi
>>> +
>>>  dnl Allow perl/python overrides
>>>  AC_PATH_PROGS([PYTHON], [python2 python])
>>>  if test -z "$PYTHON"; then
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>