Re: [libvirt] [PATCHv2] nodedev: add RDMA and tx-udp_tnl-segmentation NIC capabilities

-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Tuesday, July 21, 2015 1:06 AM To: Moshe Levi; libvir-list(a)redhat.com Subject: Re: [libvirt] [PATCHv2] nodedev: add RDMA and tx-udp_tnl- segmentation NIC capabilities On 07/19/2015 06:11 AM, Moshe Levi wrote: > Adding functionality to libvirt that will allow it query the interface > for the availability of RDMA and tx-udp_tnl-segmentation Offloading > NIC capabilities > > Here is an example of the feature XML definition: > > <device> > <name>net_eth4_90_e2_ba_5e_a5_45</name> > <path>/sys/devices/pci0000:00/0000:00:03.0/0000:08:00.1/net/eth4</path> > <parent>pci_0000_08_00_1</parent> > <capability type='net'> > <interface>eth4</interface> > <address>90:e2:ba:5e:a5:45</address> > <link speed='10000' state='up'/> > <feature name='rx'/> > <feature name='tx'/> > <feature name='sg'/> > <feature name='tso'/> > <feature name='gso'/> > <feature name='gro'/> > <feature name='rxvlan'/> > <feature name='txvlan'/> > <feature name='rxhash'/> > <feature name='rdma'/> > <feature name='txudptnl'/> > <capability type='80203'/> > </capability> > </device> > --- > configure.ac | 2 +- > docs/formatnode.html.in | 2 + > src/conf/device_conf.c | 4 +- > src/conf/device_conf.h | 2 + > src/util/virnetdev.c | 141 +++++++++++++++++++-- > src/util/virnetdev.h | 1 + > tests/nodedevschemadata/net_00_13_02_b9_f9_d3.xml | 2 + > tests/nodedevschemadata/net_00_15_58_2f_e9_55.xml | 2 + > 8 files changed, 144 insertions(+), 12 deletions(-) > > diff --git a/configure.ac b/configure.ac index a7f38e8..70b3ef3 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -390,7 +390,7 @@ AC_CHECK_TYPE([struct ifreq], > ]]) > > AC_CHECK_DECLS([ETH_FLAG_TXVLAN, ETH_FLAG_NTUPLE, ETH_FLAG_RXHASH, ETH_FLAG_LRO, > - ETHTOOL_GGSO, ETHTOOL_GGRO, ETHTOOL_GFLAGS], > + ETHTOOL_GGSO, ETHTOOL_GGRO, ETHTOOL_GFLAGS, > + ETHTOOL_GFEATURES], > [], [], [[#include <linux/ethtool.h> > ]]) > > diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index > 3ff1bef..ed00af5 100644 > --- a/docs/formatnode.html.in > +++ b/docs/formatnode.html.in > @@ -199,6 +199,8 @@ > <dt><code>txvlan</code></dt><dd>tx-vlan-offload</dd> > <dt><code>ntuple</code></dt><dd>ntuple-filters</dd> > > <dt><code>rxhash</code></dt><dd>receive-hashing</dd> > + <dt><code>rdma</code></dt><dd>remote-direct-memory- access</dd> > + > + <dt><code>txudptnl</code></dt><dd>tx-udp-tunnel- segmentation</dd> > </dl> > </dd> > <dt><code>capability</code></dt> diff --git > a/src/conf/device_conf.c b/src/conf/device_conf.c index > 98808e2..e7b7957 100644 > --- a/src/conf/device_conf.c > +++ b/src/conf/device_conf.c > @@ -51,7 +51,9 @@ VIR_ENUM_IMPL(virNetDevFeature, > "rxvlan", > "txvlan", > "ntuple", > - "rxhash") > + "rxhash", > + "rdma", > + "txudptnl") > > int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) { diff > --git a/src/conf/device_conf.h b/src/conf/device_conf.h index > 7ea90f6..40a2b3d 100644 > --- a/src/conf/device_conf.h > +++ b/src/conf/device_conf.h > @@ -74,6 +74,8 @@ typedef enum { > VIR_NET_DEV_FEAT_TXVLAN, > VIR_NET_DEV_FEAT_NTUPLE, > VIR_NET_DEV_FEAT_RXHASH, > + VIR_NET_DEV_FEAT_RDMA, > + VIR_NET_DEV_FEAT_TXUDPTNL, > VIR_NET_DEV_FEAT_LAST > } virNetDevFeature; > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index > e4fcd81..0dcb42d 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -87,6 +87,14 @@ VIR_LOG_INIT("util.netdev"); # define > VIR_IFF_ALLMULTI 0 #endif > > +#define RESOURCE_FILE_LEN 4096 > +#define TX_UDP_TNL 25 > +#define GFEATURES_SIZE 2 > +#define FEATURE_WORD(blocks, index, field) ((blocks)[(index) / 32U].field) > +#define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U) > +#define FEATURE_BIT_IS_SET(blocks, index, field) \ > + (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index)) > + > typedef enum { > VIR_MCAST_TYPE_INDEX_TOKEN, > VIR_MCAST_TYPE_NAME_TOKEN, > @@ -1868,7 +1876,6 @@ virNetDevReplaceMacAddress(const char *linkdev, > goto cleanup; > > ret = 0; > - > cleanup: > VIR_FREE(path); > return ret; > @@ -2858,9 +2865,9 @@ static int virNetDevGetMulticastTable(const char *ifname, > } > > ret = 0; > + > cleanup: > virNetDevMcastListClear(&mcast); > - > return ret; > } > > @@ -2943,11 +2950,76 @@ int virNetDevGetRxFilter(const char *ifname, > return ret; > } > > + > +/** > + * virNetDevRDMAFeature > + * This function checks for the availability of RDMA feature > + * and add it to bitmap > + * > + * @ifname: name of the interface > + * @out: add RDMA feature if exist to bitmap > + * > + * Returns 0 on success, -1 on failure. > + */ > +static int > +virNetDevRDMAFeature(const char *ifname, > + virBitmapPtr *out) { > + char *eth_devpath = NULL; > + char *ib_devpath = NULL; > + char *eth_res_buf = NULL; > + char *ib_res_buf = NULL; > + DIR *dirp = NULL; > + struct dirent *dp; > + int ret = -1; > + > + if (!(dirp = opendir(SYSFS_INFINIBAND_DIR))) { > + virReportSystemError(errno, > + _("Failed to opendir path '%s'"), > + SYSFS_INFINIBAND_DIR); > + goto cleanup; > + } > + > + if (virAsprintf(&eth_devpath, SYSFS_NET_DIR "%s/device/resource", ifname) < 0) > + goto cleanup; > + if (!virFileExists(eth_devpath)) > + goto cleanup; > + if (virFileReadAll(eth_devpath, RESOURCE_FILE_LEN, &eth_res_buf) < 0) > + goto cleanup; > + > + while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) { > + if (dp->d_name[0] == '.') > + continue; > + if (virAsprintf(&ib_devpath, SYSFS_INFINIBAND_DIR "%s/device/resource", dp->d_name) < 0) { > + VIR_FREE(ib_devpath); If virAsprintf fails, then no need to VIR_FREE here > + continue; > + } > + if (virFileReadAll(ib_devpath, RESOURCE_FILE_LEN, &ib_res_buf) < 0) { > + VIR_FREE(ib_res_buf); if virFileReadAll fails, ib_devpath should be VIR_FREE()'d > + continue; > + } > + if (STREQ(eth_res_buf, ib_res_buf)) { > + ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA)); > + break; > + } > + VIR_FREE(ib_res_buf); > + } How about if I change the while loop to: while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) { if (dp->d_name[0] == '.') continue; if (virAsprintf(&ib_devpath, SYSFS_INFINIBAND_DIR "%s/device/resource", dp->d_name) < 0) continue; if (virFileReadAll(ib_devpath, RESOURCE_FILE_LEN, &ib_res_buf) > 0 && STREQ(eth_res_buf, ib_res_buf)) { ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA)); break; } VIR_FREE(ib_devpath); VIR_FREE(ib_res_buf); }

NOTE: I considered >= 0 to do the STREQ, but that'd be pointless I think > + ret = 0; > + > + cleanup: > + closedir(dirp); > + VIR_FREE(eth_devpath); > + VIR_FREE(ib_devpath); > + VIR_FREE(eth_res_buf); > + VIR_FREE(ib_res_buf); > + return ret; > +} > + > #if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ) > > /** > - * virNetDevFeatureAvailable > - * This function checks for the availability of a network device > feature > + * virNetDevSendEthtoolIoctl > + * This function sends ethtool ioctl request > * > * @ifname: name of the interface > * @cmd: reference to an ethtool command structure @@ -2955,7 +3027,7 > @@ int virNetDevGetRxFilter(const char *ifname, > * Returns 0 on success, -1 on failure. > */ > static int > -virNetDevFeatureAvailable(const char *ifname, struct ethtool_value > *cmd) > +virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) > { > int ret = -1; > int sock = -1; > @@ -2969,9 +3041,9 @@ virNetDevFeatureAvailable(const char *ifname, > struct ethtool_value *cmd) > > memset(&ifr, 0, sizeof(ifr)); > strcpy(ifr.ifr_name, ifname); > - ifr.ifr_data = (void*) cmd; > - > - if (ioctl(sock, SIOCETHTOOL, &ifr) != 0) { > + ifr.ifr_data = cmd; > + ret = ioctl(sock, SIOCETHTOOL, &ifr); > + if (ret != 0) { > switch (errno) { > case EPERM: > VIR_DEBUG("ethtool ioctl: permission denied"); @@ > -2988,11 +3060,51 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) > } > } > > - ret = cmd->data > 0 ? 1: 0; > cleanup: > if (sock) > VIR_FORCE_CLOSE(sock); > + return ret; > +} > > + > +/** > +* virNetDevFeatureAvailable > +* This function checks for the availability of a network device > +feature > +* > +* @ifname: name of the interface > +* @cmd: reference to an ethtool command structure > +* > +* Returns 0 if not found, 1 on success, and -1 on failure. > +*/ > +static int > +virNetDevFeatureAvailable(const char *ifname, struct ethtool_value > +*cmd) { > + int ret = -1; > + > + cmd = (void*)cmd; > + if (!virNetDevSendEthtoolIoctl(ifname, cmd)) > + ret = cmd->data > 0 ? 1: 0; > + return ret; > +} > + > + > +/** > + * virNetDevGFeatureAvailable > + * This function checks for the availability of a network device > +gfeature > + * > + * @ifname: name of the interface > + * @cmd: reference to a gfeatures ethtool command structure > + * > + * Returns 0 if not found, 1 on success, and -1 on failure. > + */ > +static int > +virNetDevGFeatureAvailable(const char *ifname, struct > +ethtool_gfeatures *cmd) { > + int ret = -1; > + > + cmd = (void*)cmd; > + if (!virNetDevSendEthtoolIoctl(ifname, cmd)) > + ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); > return ret; > } > > @@ -3013,7 +3125,7 @@ virNetDevGetFeatures(const char *ifname, { > size_t i = -1; > struct ethtool_value cmd = { 0 }; > - > + struct ethtool_gfeatures g_cmd = { 0 }; > struct elem{ > const int cmd; > const virNetDevFeature feat; > @@ -3069,6 +3181,15 @@ virNetDevGetFeatures(const char *ifname, > } > # endif > > +# if HAVE_DECL_ETHTOOL_GFEATURES make syntax-check would have told you this was off by an extra space.

If you're good with those adjustments, I'll push the patch.

Tks - John > + g_cmd.cmd = ETHTOOL_GFEATURES; > + g_cmd.size = GFEATURES_SIZE; > + if (virNetDevGFeatureAvailable(ifname, &g_cmd)) > + ignore_value(virBitmapSetBit(*out, > +VIR_NET_DEV_FEAT_TXUDPTNL)); # endif > + > + if (virNetDevRDMAFeature(ifname, out)) > + return -1; > return 0; > } > #else > diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index > 190b70e..fff881c 100644 > --- a/src/util/virnetdev.h > +++ b/src/util/virnetdev.h > @@ -210,6 +210,7 @@ int virNetDevGetRcvAllMulti(const char *ifname, bool *receive) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > > # define SYSFS_NET_DIR "/sys/class/net/" > +# define SYSFS_INFINIBAND_DIR "/sys/class/infiniband/" > int virNetDevSysfsFile(char **pf_sysfs_device_link, > const char *ifname, > const char *file) diff --git > a/tests/nodedevschemadata/net_00_13_02_b9_f9_d3.xml > b/tests/nodedevschemadata/net_00_13_02_b9_f9_d3.xml > index 2a34fed..d4c96e8 100644 > --- a/tests/nodedevschemadata/net_00_13_02_b9_f9_d3.xml > +++ b/tests/nodedevschemadata/net_00_13_02_b9_f9_d3.xml > @@ -13,6 +13,8 @@ > <feature name='rxvlan'/> > <feature name='txvlan'/> > <feature name='rxhash'/> > + <feature name='rdma'/> > + <feature name='txudptnl'/> > <capability type='80211'/> > </capability> > </device> > diff --git a/tests/nodedevschemadata/net_00_15_58_2f_e9_55.xml > b/tests/nodedevschemadata/net_00_15_58_2f_e9_55.xml > index 81d398c..71bf90e 100644 > --- a/tests/nodedevschemadata/net_00_15_58_2f_e9_55.xml > +++ b/tests/nodedevschemadata/net_00_15_58_2f_e9_55.xml > @@ -13,6 +13,8 @@ > <feature name='rxvlan'/> > <feature name='txvlan'/> > <feature name='rxhash'/> > + <feature name='rdma'/> > + <feature name='txudptnl'/> > <capability type='80203'/> > </capability> > </device> >