[libvirt] [PATCH] nodedev: add RDMA and tx-udp_tnl-segmentation NIC capabilities
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='tx-udp_tnl-segmentation'/> <capability type='80203'/> </capability> </device> --- docs/formatnode.html.in | 2 + src/conf/device_conf.c | 4 +- src/conf/device_conf.h | 2 + src/util/virnetdev.c | 97 ++++++++++++++++++++++++++++++++++++++-------- src/util/virnetdev.h | 1 + 5 files changed, 88 insertions(+), 18 deletions(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 3ff1bef..9b32dd1 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>tx-udp_tnl-segmentation</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..8e8d557 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", + "tx-udp_tnl-segmentation") int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) { diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 7ea90f6..07298c9 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_TX_UDP_TNL_SEGMENTATION, VIR_NET_DEV_FEAT_LAST } virNetDevFeature; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index e4fcd81..3086616 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, @@ -2943,6 +2951,58 @@ 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; + struct dirent *dp; + + DIR *dirp = opendir(SYSFS_INFINIBAND_DIR); + if (dirp == NULL) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + SYSFS_INFINIBAND_DIR); + return -1; + } + + if (virAsprintf(ð_devpath, SYSFS_NET_DIR "%s/device/resource", ifname) < 0) + return -1; + if (!virFileExists(eth_devpath)) + return 0; + if (virFileReadAll(eth_devpath, RESOURCE_FILE_LEN, ð_res_buf) < 0) + return -1; + while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) { + if (STREQ(dp->d_name, ".") || + STREQ(dp->d_name, "..")) + 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) + continue; + if (STREQ(eth_res_buf, ib_res_buf)) { + ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA)); + break; + } + } + return 0; +} + #if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ) /** @@ -2952,12 +3012,10 @@ int virNetDevGetRxFilter(const char *ifname, * @ifname: name of the interface * @cmd: reference to an ethtool command structure * - * Returns 0 on success, -1 on failure. */ -static int -virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) +static void +virNetDevFeatureAvailable(const char *ifname, void *cmd) { - int ret = -1; int sock = -1; virIfreq ifr; @@ -2969,8 +3027,7 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) memset(&ifr, 0, sizeof(ifr)); strcpy(ifr.ifr_name, ifname); - ifr.ifr_data = (void*) cmd; - + ifr.ifr_data = cmd; if (ioctl(sock, SIOCETHTOOL, &ifr) != 0) { switch (errno) { case EPERM: @@ -2988,12 +3045,9 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) } } - ret = cmd->data > 0 ? 1: 0; cleanup: if (sock) VIR_FORCE_CLOSE(sock); - - return ret; } @@ -3013,7 +3067,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; @@ -3037,7 +3091,8 @@ virNetDevGetFeatures(const char *ifname, for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) { cmd.cmd = cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd)) + virNetDevFeatureAvailable(ifname, &cmd); + if (cmd.data > 0) ignore_value(virBitmapSetBit(*out, cmds[i].feat)); } @@ -3061,14 +3116,22 @@ virNetDevGetFeatures(const char *ifname, }; cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd)) { - for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { - if (cmd.data & flags[j].cmd) - ignore_value(virBitmapSetBit(*out, flags[j].feat)); + virNetDevFeatureAvailable(ifname, &cmd); + if (cmd.data > 0) { + for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { + if (cmd.data & flags[j].cmd) + ignore_value(virBitmapSetBit(*out, flags[j].feat)); + } } - } -# endif + g_cmd.cmd = ETHTOOL_GFEATURES; + g_cmd.size = GFEATURES_SIZE; + virNetDevFeatureAvailable(ifname, &g_cmd); + if FEATURE_BIT_IS_SET(g_cmd.features, TX_UDP_TNL, active) + ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TX_UDP_TNL_SEGMENTATION)); +# 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) -- 1.7.1
On 06/18/2015 04:45 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
Any chance for a shorter name? Eg txudptnl (
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='tx-udp_tnl-segmentation'/> <capability type='80203'/> </capability> </device> --- docs/formatnode.html.in | 2 + src/conf/device_conf.c | 4 +- src/conf/device_conf.h | 2 + src/util/virnetdev.c | 97 ++++++++++++++++++++++++++++++++++++++-------- src/util/virnetdev.h | 1 + 5 files changed, 88 insertions(+), 18 deletions(-)
Any chance to have some sort of test? Check out commit id 'c9027d8f4' for an example when other features were added
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 3ff1bef..9b32dd1 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>tx-udp_tnl-segmentation</code></dt><dd>tx-udp-tunnel-segmentation</dd>
Here's where the shorter name would be used inside the <code>, but keep the longer name inside the <dd>
</dl> </dd> <dt><code>capability</code></dt> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 98808e2..8e8d557 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", + "tx-udp_tnl-segmentation")
shorter name
int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) { diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 7ea90f6..07298c9 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_TX_UDP_TNL_SEGMENTATION,
shorter name
VIR_NET_DEV_FEAT_LAST } virNetDevFeature;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index e4fcd81..3086616 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, @@ -2943,6 +2951,58 @@ 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; + struct dirent *dp; + + DIR *dirp = opendir(SYSFS_INFINIBAND_DIR);
The extra space should be between dirp and if (!dirp) not between dp and dirp... Also leak dirp. Add an "int ret = -1" and use standard "goto cleanup;" type logic on failures in order to clean up anything allocated. So the construct/code would be: DIR *dirp = NULL; if (!(dirp = opendir(SYSFS_INFINIBAND_DIR))) {
+ if (dirp == NULL) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + SYSFS_INFINIBAND_DIR); + return -1; goto cleanup; + } + + if (virAsprintf(ð_devpath, SYSFS_NET_DIR "%s/device/resource", ifname) < 0)
eth_devpath would be leaked
+ return -1; + if (!virFileExists(eth_devpath)) + return 0;
ret = 0 ??? goto cleanup; It's a success to not have the file? Wouldn't that make it difficult for the caller to determine if something existed and/or whether the bitmap was updated? If that doesn't matter then fine, but still have proper exit syntax
+ if (virFileReadAll(eth_devpath, RESOURCE_FILE_LEN, ð_res_buf) < 0)
eth_res_buf would be leaked
+ return -1;
goto cleanup;
+ while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) { + if (STREQ(dp->d_name, ".") || + STREQ(dp->d_name, "..")) + continue;
Consider instead: if (dp->d_name[0] == '.') continue;
+ + if (virAsprintf(&ib_devpath, SYSFS_INFINIBAND_DIR "%s/device/resource", dp->d_name) < 0) + continue;
ib_devpath is leaked
+ if (virFileReadAll(ib_devpath, RESOURCE_FILE_LEN, &ib_res_buf) < 0)
ib_res_buf is leaked multiple times VIR_FREE(ib_devpath);
+ continue; + if (STREQ(eth_res_buf, ib_res_buf)) {
^ Extra space
+ ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA)); + break; + } VIR_FREE(ib_res_buf); + } + return 0;
here you would have: 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)
/** @@ -2952,12 +3012,10 @@ int virNetDevGetRxFilter(const char *ifname, * @ifname: name of the interface * @cmd: reference to an ethtool command structure
Would this parameter need an adjustment to indicate either command or feature structure?
* - * Returns 0 on success, -1 on failure.
hrmph - looks like this was wrong ... 0 if not found, 1 on success, and -1 on failure... I'm 50/50 on the switch from int to void... Of course the name of the function to me implies a value return
*/ -static int -virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) +static void +virNetDevFeatureAvailable(const char *ifname, void *cmd) { - int ret = -1; int sock = -1; virIfreq ifr;
@@ -2969,8 +3027,7 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
memset(&ifr, 0, sizeof(ifr)); strcpy(ifr.ifr_name, ifname); - ifr.ifr_data = (void*) cmd; - + ifr.ifr_data = cmd; if (ioctl(sock, SIOCETHTOOL, &ifr) != 0) { switch (errno) { case EPERM: @@ -2988,12 +3045,9 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) } }
- ret = cmd->data > 0 ? 1: 0; cleanup: if (sock) VIR_FORCE_CLOSE(sock); - - return ret; }
@@ -3013,7 +3067,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; @@ -3037,7 +3091,8 @@ virNetDevGetFeatures(const char *ifname,
for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) { cmd.cmd = cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd)) + virNetDevFeatureAvailable(ifname, &cmd); + if (cmd.data > 0)
Could just go with cmd.data unless it could be negative...
ignore_value(virBitmapSetBit(*out, cmds[i].feat)); }
@@ -3061,14 +3116,22 @@ virNetDevGetFeatures(const char *ifname, };
cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd)) { - for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { - if (cmd.data & flags[j].cmd) - ignore_value(virBitmapSetBit(*out, flags[j].feat)); + virNetDevFeatureAvailable(ifname, &cmd); + if (cmd.data > 0) { + for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { + if (cmd.data & flags[j].cmd) + ignore_value(virBitmapSetBit(*out, flags[j].feat)); + }
There are 4 extra spaces for each of the previous 5 lines... Also I suppose you could just use "if (cmd.data)", unless of course it could be negative...
} - } -# endif
What would happen if some arch didn't have ETHTOOL_GFEATURES? or is that assumed due to opening "#if"? #if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ)
+ g_cmd.cmd = ETHTOOL_GFEATURES; + g_cmd.size = GFEATURES_SIZE; + virNetDevFeatureAvailable(ifname, &g_cmd); + if FEATURE_BIT_IS_SET(g_cmd.features, TX_UDP_TNL, active)
This looks odd... should be "if (FEATURE_BIT...))
+ ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TX_UDP_TNL_SEGMENTATION));
Long name... Looking forward to a V2 John
+# 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)
Hi John, I responded to some of your comments inline. Thank you for reviewing my patch. I just posted V2 of it, hopefully if will be better :) Thanks, Moshe Levi.
-----Original Message----- From: John Ferlan [mailto:jferlan@redhat.com] Sent: Tuesday, July 14, 2015 5:36 PM To: Moshe Levi; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH] nodedev: add RDMA and tx-udp_tnl- segmentation NIC capabilities
On 06/18/2015 04:45 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
Any chance for a shorter name? Eg txudptnl ( Yes will do
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='tx-udp_tnl-segmentation'/> <capability type='80203'/> </capability> </device> --- docs/formatnode.html.in | 2 + src/conf/device_conf.c | 4 +- src/conf/device_conf.h | 2 + src/util/virnetdev.c | 97 ++++++++++++++++++++++++++++++++++++++-------- src/util/virnetdev.h | 1 + 5 files changed, 88 insertions(+), 18 deletions(-)
Any chance to have some sort of test? Check out commit id 'c9027d8f4' for an example when other features were added Yes will do
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 3ff1bef..9b32dd1 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>tx-udp_tnl-segmentation</code></dt><dd>tx-udp-tunnel- segme + ntation</dd>
Here's where the shorter name would be used inside the <code>, but keep the longer name inside the <dd>
</dl> </dd> <dt><code>capability</code></dt> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 98808e2..8e8d557 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", + "tx-udp_tnl-segmentation")
shorter name
int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) { diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 7ea90f6..07298c9 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_TX_UDP_TNL_SEGMENTATION,
shorter name
VIR_NET_DEV_FEAT_LAST } virNetDevFeature;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index e4fcd81..3086616 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, @@ -2943,6 +2951,58 @@ 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; + struct dirent *dp; + + DIR *dirp = opendir(SYSFS_INFINIBAND_DIR);
The extra space should be between dirp and if (!dirp) not between dp and dirp...
Also leak dirp.
Add an "int ret = -1" and use standard "goto cleanup;" type logic on failures in order to clean up anything allocated.
Yes will do
So the construct/code would be:
DIR *dirp = NULL;
if (!(dirp = opendir(SYSFS_INFINIBAND_DIR))) {
+ if (dirp == NULL) { + virReportSystemError(errno, + _("Failed to opendir path '%s'"), + SYSFS_INFINIBAND_DIR); + return -1; goto cleanup; + } + + if (virAsprintf(ð_devpath, SYSFS_NET_DIR "%s/device/resource", + ifname) < 0)
eth_devpath would be leaked
+ return -1; + if (!virFileExists(eth_devpath)) + return 0;
ret = 0 ??? goto cleanup;
It's a success to not have the file? Wouldn't that make it difficult for the caller to determine if something existed and/or whether the bitmap was updated?
If that doesn't matter then fine, but still have proper exit syntax Yes you are right
+ if (virFileReadAll(eth_devpath, RESOURCE_FILE_LEN, ð_res_buf) + < 0)
eth_res_buf would be leaked
+ return -1;
goto cleanup;
+ while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) { + if (STREQ(dp->d_name, ".") || + STREQ(dp->d_name, "..")) + continue;
Consider instead:
if (dp->d_name[0] == '.') continue;
+ + if (virAsprintf(&ib_devpath, SYSFS_INFINIBAND_DIR "%s/device/resource", dp->d_name) < 0) + continue;
ib_devpath is leaked
+ if (virFileReadAll(ib_devpath, RESOURCE_FILE_LEN, + &ib_res_buf) < 0)
ib_res_buf is leaked multiple times
VIR_FREE(ib_devpath);
+ continue; + if (STREQ(eth_res_buf, ib_res_buf)) {
^ Extra space
+ ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_RDMA)); + break; + } VIR_FREE(ib_res_buf); + } + return 0;
here you would have: 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)
/** @@ -2952,12 +3012,10 @@ int virNetDevGetRxFilter(const char *ifname, * @ifname: name of the interface * @cmd: reference to an ethtool command structure
Would this parameter need an adjustment to indicate either command or feature structure?
Looking on the code this the correct comment for virNetDevGetRxFilter * virNetDevGetRxFilter: * This function supplies the RX filter list for a given device interface * * @ifname: Name of the interface * @filter: The RX filter list So there is no change to do here, unless I am missing something.
* - * Returns 0 on success, -1 on failure.
hrmph - looks like this was wrong ... 0 if not found, 1 on success, and -1 on failure...
Updated
I'm 50/50 on the switch from int to void... Of course the name of the function to me implies a value return
Ok I split it to 3 functions virNetDevSendEthtoolIoctl - just send the ioctl command virNetDevFeatureAvailable - will be same as before virNetDevGFeatureAvailable - check if GFeature is Available
*/ -static int -virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) +static void +virNetDevFeatureAvailable(const char *ifname, void *cmd) { - int ret = -1; int sock = -1; virIfreq ifr;
@@ -2969,8 +3027,7 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
memset(&ifr, 0, sizeof(ifr)); strcpy(ifr.ifr_name, ifname); - ifr.ifr_data = (void*) cmd; - + ifr.ifr_data = cmd; if (ioctl(sock, SIOCETHTOOL, &ifr) != 0) { switch (errno) { case EPERM: @@ -2988,12 +3045,9 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) } }
- ret = cmd->data > 0 ? 1: 0; cleanup: if (sock) VIR_FORCE_CLOSE(sock); - - return ret; }
@@ -3013,7 +3067,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; @@ -3037,7 +3091,8 @@ virNetDevGetFeatures(const char *ifname,
for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) { cmd.cmd = cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd)) + virNetDevFeatureAvailable(ifname, &cmd); + if (cmd.data > 0)
Could just go with cmd.data unless it could be negative...
ignore_value(virBitmapSetBit(*out, cmds[i].feat)); }
@@ -3061,14 +3116,22 @@ virNetDevGetFeatures(const char *ifname, };
cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd)) { - for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { - if (cmd.data & flags[j].cmd) - ignore_value(virBitmapSetBit(*out, flags[j].feat)); + virNetDevFeatureAvailable(ifname, &cmd); + if (cmd.data > 0) { + for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { + if (cmd.data & flags[j].cmd) + ignore_value(virBitmapSetBit(*out, flags[j].feat)); + }
There are 4 extra spaces for each of the previous 5 lines...
Also I suppose you could just use "if (cmd.data)", unless of course it could be negative...
} - } -# endif
What would happen if some arch didn't have ETHTOOL_GFEATURES? or is that assumed due to opening "#if"?
I update the configure to check if ETHTOOL_GFEATURES exist and used
#if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ)
+ g_cmd.cmd = ETHTOOL_GFEATURES; + g_cmd.size = GFEATURES_SIZE; + virNetDevFeatureAvailable(ifname, &g_cmd); + if FEATURE_BIT_IS_SET(g_cmd.features, TX_UDP_TNL, active)
This looks odd... should be "if (FEATURE_BIT...))
+ ignore_value(virBitmapSetBit(*out, + VIR_NET_DEV_FEAT_TX_UDP_TNL_SEGMENTATION));
Long name...
Looking forward to a V2
John
+# 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)
participants (2)
-
John Ferlan -
Moshe Levi