2:28 a.m.
-----Original Message-----
From: sendmail [mailto:justsendmailnothingelse@gmail.com] On Behalf Of
Laine Stump
Sent: Tuesday, August 11, 2015 9:27 AM
To: libvir-list(a)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)