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

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/enab... >> >> --- >>  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

>> +                   [], >> +                   [[#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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list