Re: [PATCH] nodedev: Fix gfeature size to be according to running kernel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: sendmail [mailto:justsendmailnothingelse@xxxxxxxxx] On Behalf Of
> Laine Stump
> Sent: Tuesday, August 11, 2015 9:27 AM
> To: libvir-list@xxxxxxxxxx
> Cc: Moshe Levi
> Subject: Re:  [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)


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]