Re: [PATCH] SRIOV NIC offload feature discovery

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

 



On Mon, Feb 16, 2015 at 10:18:32AM +0000, James Chapman wrote:
> Adding functionality to libvirt that will allow it
> query the ethtool interface for the availability
> of certain NIC HW offload features
> ---
>  src/conf/device_conf.h             |   6 ++
>  src/conf/node_device_conf.c        |   7 ++
>  src/conf/node_device_conf.h        |   2 +
>  src/libvirt_private.syms           |   1 +
>  src/node_device/node_device_udev.c |   4 ++
>  src/util/virnetdev.c               | 134 +++++++++++++++++++++++++++++++++++++
>  src/util/virnetdev.h               |  12 +++-
>  7 files changed, 165 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index 7256cdc..091f2f0 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -62,6 +62,12 @@ struct _virInterfaceLink {
>      unsigned int speed;      /* link speed in Mbits per second */
>  };
>  
> +typedef struct _virDevFeature virDevFeature;
> +typedef virDevFeature *virDevFeaturePtr;
> +struct _virDevFeature {
> +   char *name;             /* device feature */
> +};
> +
>  int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr);
>  
>  int virDevicePCIAddressParseXML(xmlNodePtr node,
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index a728a00..7f4dbfe 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -437,6 +437,12 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def)
>                  virBufferEscapeString(&buf, "<address>%s</address>\n",
>                                    data->net.address);
>              virInterfaceLinkFormat(&buf, &data->net.lnk);
> +            if (data->net.features) {
> +                for (i = 0; i < data->net.nfeatures; i++) {
> +                    virBufferAsprintf(&buf, "<feature name='%s'/>\n",
> +                            data->net.features[i].name);

Storing these as an enum (for example virNodeNetDevFeature) in a
virBitmap would be nicer, in my opinion.

> +                }
> +            }
>              if (data->net.subtype != VIR_NODE_DEV_CAP_NET_LAST) {
>                  const char *subtyp =
>                      virNodeDevNetCapTypeToString(data->net.subtype);
> @@ -1679,6 +1685,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
>      case VIR_NODE_DEV_CAP_NET:
>          VIR_FREE(data->net.ifname);
>          VIR_FREE(data->net.address);
> +        VIR_FREE(data->net.features);
>          break;
>      case VIR_NODE_DEV_CAP_SCSI_HOST:
>          VIR_FREE(data->scsi_host.wwnn);
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index fd5d179..918523a 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -141,6 +141,8 @@ struct _virNodeDevCapsDef {
>              char *ifname;
>              virInterfaceLink lnk;
>              virNodeDevNetCapType subtype;  /* LAST -> no subtype */
> +            size_t nfeatures;
> +            virDevFeaturePtr features;
>          } net;
>          struct {
>              unsigned int host;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c07a561..1d165a9 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1659,6 +1659,7 @@ virNetDevAddRoute;
>  virNetDevClearIPAddress;
>  virNetDevDelMulti;
>  virNetDevExists;
> +virNetDevGetFeatures;
>  virNetDevGetIndex;
>  virNetDevGetIPv4Address;
>  virNetDevGetLinkInfo;
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 03c7a0b..349733f 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -719,6 +719,10 @@ static int udevProcessNetworkInterface(struct udev_device *device,
>      if (virNetDevGetLinkInfo(data->net.ifname, &data->net.lnk) < 0)
>          goto out;
>  
> +    if (virNetDevGetFeatures(data->net.ifname, &data->net.features,
> +                &data->net.nfeatures) < 0)
> +        goto out;
> +
>      ret = 0;
>  
>   out:
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 2a0eb43..e513c85 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -2728,3 +2728,137 @@ int virNetDevGetRxFilter(const char *ifname,
>      *filter = fil;
>      return ret;
>  }
> +
> +#if defined(SIOCETHTOOL) && defined(HAVE_STRUCT_IFREQ)
> +
> +/**
> + * _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 on success, -1 on failure.
> + */
> +int
> +_virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)

We don't use a special prefix for internal function, we just define them
as static.

> +{
> +    int ret = -1;
> +    int sock = -1;
> +    virIfreq ifr;
> +
> +    sock = socket(AF_LOCAL, SOCK_DGRAM, 0);

Can 'sock' be reused just like cmd?

> +    if (sock < 0) {
> +        virReportSystemError(errno, "%s", _("Cannot open control socket"));
> +        goto cleanup;
> +    }
> +
> +    strcpy(ifr.ifr_name, ifname);
> +    ifr.ifr_data = (void*) cmd;
> +
> +    if (ioctl(sock, SIOCETHTOOL, &ifr) != 0) {
> +        /* Privileged command, no error */
> +        if (errno == EPERM || errno == EINVAL) {
> +            virReportSystemError(errno, "%s", _("ioctl"));
> +            /* Some kernels dont support named feature, no error */

If the failure is ignored, reporting an error spams the log, a VIR_DEBUG
would be nicer.

Also, the messages aren't very descriptive.

> +        } else if (errno == EOPNOTSUPP) {
> +            virReportSystemError(errno, "%s", _("Warning"));
> +        } else {
> +            virReportSystemError(errno, "%s", _("Error"));
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = cmd->data > 0 ? 1: 0;
> + cleanup:
> +    if (sock)
> +        VIR_FORCE_CLOSE(sock);
> +
> +    return ret;
> +}
> +
> +
> +/**
> + * virNetDevGetFeatures:
> + * This function gets the nic offloads features available for ifname
> + *
> + * @ifname: name of the interface
> + * @features: network device feature structures
> + * @nfeatures: number of features available
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +int
> +virNetDevGetFeatures(const char *ifname,
> +                     virDevFeaturePtr *features,
> +                     size_t *nfeatures)
> +{
> +    int ret = -1;
> +    size_t i = -1;
> +    size_t j = -1;
> +    struct ethtool_value cmd;
> +
> +    struct elem{
> +        const char *name;
> +        const int cmd;
> +    };
> +    /* legacy ethtool getters */
> +    struct elem cmds[] = {
> +        {"rx",     ETHTOOL_GRXCSUM},
> +        {"tx",     ETHTOOL_GTXCSUM },
> +        {"sg",     ETHTOOL_GSG},
> +        {"tso",    ETHTOOL_GTSO},
> +        {"gso",    ETHTOOL_GGSO},
> +        {"gro",    ETHTOOL_GGRO},
> +    };
> +    /* ethtool masks */
> +    struct elem flags[] = {
> +        {"lro",    ETH_FLAG_LRO},
> +        {"rxvlan", ETH_FLAG_RXVLAN},
> +        {"txvlan", ETH_FLAG_TXVLAN},
> +        {"ntuple", ETH_FLAG_NTUPLE},
> +        {"rxhash", ETH_FLAG_RXHASH},
> +    };
> +
> +    for (i = 0; i < (sizeof(cmds)/sizeof(struct elem)); i++) {
> +        cmd.cmd = cmds[i].cmd;
> +        if (_virNetDevFeatureAvailable(ifname, &cmd)) {
> +            if (VIR_EXPAND_N(*features, *nfeatures, 1) < 0)
> +                goto cleanup;
> +            if ((ret = VIR_STRDUP((*features)[i].name, cmds[i].name)) != 1)

Here, the value of i is unrelated to the size of the allocated array.
The daemon crashes on startup on an interface that only has the "gro"
feature.

The VIR_APPEND_ELEMENT can be used here. (Or virBitmapSetBit to match
the suggestion I made above)

> +                goto cleanup;
> +        }
> +    }
> +
> +    cmd.cmd = ETHTOOL_GFLAGS;
> +    for (j = 0; j < (sizeof(flags)/sizeof(struct elem)); j++) {

We have the ARRAY_CARDINALITY macro for this.

Jan

Attachment: signature.asc
Description: Digital signature

--
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]