-----Original Message----- From: sendmail [mailto:justsendmailnothingelse@gmail.com] On Behalf Of Laine Stump Sent: Tuesday, August 11, 2015 9:27 AM To: libvir-list@redhat.com Cc: Moshe Levi Subject: Re: [libvirt] [PATCH] nodedev: Fix gfeature size to be according to running kernel

On 08/08/2015 05:34 AM, Moshe Levi wrote:

...

This patch add virNetDevGetGFeaturesSize to get the supported gfeature size from the kernel ---

This is interesting/possibly useful, but it doesn't fix the crash that users are experiencing. Here is a patch that should fix the crash:

https://www.redhat.com/archives/libvir-list/2015-August/msg00382.html

I would rather have that patch pushed before this one (which will mean rebasing and resolving some merge conflicts).

Ok I will rebase once you patch is merged.

...

src/util/virnetdev.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 1e20789..3fdf4bb 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -89,8 +89,10 @@ VIR_LOG_INIT("util.netdev");

#define RESOURCE_FILE_LEN 4096 #if HAVE_DECL_ETHTOOL_GFEATURES +#define ETH_GSTRING_LEN 32

Where does the "32" come from? Is it from ETH_GSTRING_LEN (defined in ethtool.h)? If so, why not just use that directly instead of #defining a new one?

It is define in ethtool-copy.h

...

# define TX_UDP_TNL 25

I'll keep asking this every time I encounter it until I get a response - when I look at the ethtool source files in the kernel, it looks to me like the index for this capability should be 26 and not 25. Where/how did you arrive at the value of 25?

I printed the value when I run ethtool command moshe index 25 tx-udp_tnl-segmentation: on moshe index 26 tx-mpls-segmentation: off [fixed] ethtool modified code static void dump_one_feature(const char *indent, const char *name, const struct feature_state *state, const struct feature_state *ref_state, u32 index) { if (ref_state && !(FEATURE_BIT_IS_SET(state->features.features, index, active) ^ FEATURE_BIT_IS_SET(ref_state->features.features, index, active))) return; printf("moshe index %d\n",index); printf("%s%s: %s%s\n", indent, name, FEATURE_BIT_IS_SET(state->features.features, index, active) ? "on" : "off", (!FEATURE_BIT_IS_SET(state->features.features, index, available) || FEATURE_BIT_IS_SET(state->features.features, index, never_changed)) ? " [fixed]" : (FEATURE_BIT_IS_SET(state->features.features, index, requested) ^ FEATURE_BIT_IS_SET(state->features.features, index, active)) ? (FEATURE_BIT_IS_SET(state->features.features, index, requested) ? " [requested on]" : " [requested off]") : ""); } Also this how I got the GFEATURES_SIZE 2

...

-# define GFEATURES_SIZE 2 +# define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) +# define FEATURE_BITS_TO_BLOCKS(n_bits) DIV_ROUND_UP(n_bits, 32U) # 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) \ @@ -3110,6 +3112,53 @@ virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd) ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); return ret; } +/** + * virNetDevGetGFeaturesSize + * This function return the number of gfeatures supported in + * the kernel + * + * @ifname: name of the interface + * + * Returns gfeature size on success, and 0 on failure or not supported. + */ + static int +virNetDevGetGFeaturesSize(const char *ifname) { + struct { + struct ethtool_sset_info hdr; + uint32_t buf[1]; + } sset_info; + struct ethtool_gstrings *strings; + uint32_t size = 0; + + sset_info.hdr.cmd = ETHTOOL_GSSET_INFO; + sset_info.hdr.reserved = 0; + sset_info.hdr.sset_mask = 1ULL << ETH_SS_FEATURES; + + if (virNetDevSendEthtoolIoctl(ifname, &sset_info) == -1) + return size;

virNetDevSendEthtoolIoctl() logs an error message, but it looks like you want for an error to be swallowed here (just returning size = 0). If that's the case then virNetDevSendEthtoolIoctl() needs to be reworked to not log errors, then every caller to it needs to log the error.

This was comment by John Ferlan he preferred the method will return size 0 if not supported or error. My previous patch which I send to him directly and not the ML return -1 on failure. But thinking about this again I don't want to swallow if error occur. What do you think?

...

+ size = sset_info.hdr.sset_mask ? sset_info.hdr.data[0] : 0; + + if (VIR_ALLOC_VAR(strings, struct ethtool_gstrings, sizeof(*strings) + size * ETH_GSTRING_LEN)) { + size = 0; + return size; + } + + strings->cmd = ETHTOOL_GSTRINGS; + strings->string_set = ETH_SS_FEATURES; + strings->len = size; + if (size != 0 && virNetDevSendEthtoolIoctl(ifname, strings) == -1) { + size = 0; + goto cleanup; + } + + size = FEATURE_BITS_TO_BLOCKS(size); + + cleanup: + VIR_FREE(strings); + return size; +} + # endif

@@ -3189,9 +3238,12 @@ virNetDevGetFeatures(const char *ifname,

# if HAVE_DECL_ETHTOOL_GFEATURES g_cmd.cmd = ETHTOOL_GFEATURES; - g_cmd.size = GFEATURES_SIZE; - if (virNetDevGFeatureAvailable(ifname, &g_cmd)) - ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL)); + g_cmd.size = virNetDevGetGFeaturesSize(ifname); + if (g_cmd.size == 0) + VIR_DEBUG("GFeatures unsupported or failure in getting + GFeatures size on ifname %s", ifname);

Should this failure really be ignored? Or should it lead to failure of the entire operation?

So I guess only if I will return -1 on failure we should not ignore it (like I mention in the comment above ) and the unsupported will be size 0 And that can be ignored.

...

+ else + if (virNetDevGFeatureAvailable(ifname, &g_cmd)) + ignore_value(virBitmapSetBit(*out, + VIR_NET_DEV_FEAT_TXUDPTNL)); # endif

if (virNetDevRDMAFeature(ifname, out) < 0)