[ovs-dev] [RFC PATCH v2 1/1] netdev-dpdk: Add support for DPDK 16.07

[Daniele Di Proietto]

[...]

> @@ -1776,7 +1764,8 @@ netdev_dpdk_get_stats(const struct netdev
> > *netdev, struct netdev_stats *stats)
> >      netdev_dpdk_get_carrier(netdev, &gg);
> >      ovs_mutex_lock(&dev->mutex);
> >
> > -    struct rte_eth_xstats *rte_xstats;
> > +    struct rte_eth_xstat *rte_xstats;
> > +    struct rte_eth_xstat_name *rte_xstats_names;
> >      int rte_xstats_len, rte_xstats_ret;
> >
> >      if (rte_eth_stats_get(dev->port_id, &rte_stats)) {
> > @@ -1785,20 +1774,51 @@ netdev_dpdk_get_stats(const struct netdev
> > *netdev, struct netdev_stats *stats)
> >          return EPROTO;
> >      }
> >
> > -    rte_xstats_len = rte_eth_xstats_get(dev->port_id, NULL, 0);
> > -    if (rte_xstats_len > 0) {
> > -        rte_xstats = dpdk_rte_mzalloc(sizeof(*rte_xstats) *
> rte_xstats_len);
> > -        memset(rte_xstats, 0xff, sizeof(*rte_xstats) * rte_xstats_len);
> > -        rte_xstats_ret = rte_eth_xstats_get(dev->port_id, rte_xstats,
> > -                                            rte_xstats_len);
> > -        if (rte_xstats_ret > 0 && rte_xstats_ret <= rte_xstats_len) {
> > -            netdev_dpdk_convert_xstats(stats, rte_xstats,
> rte_xstats_ret);
> > -        }
> > +    /* Get length of statistics */
> > +    rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
> > +    if (rte_xstats_len < 0) {
> > +        VLOG_WARN("Cannot get XSTATS values for port: %i",
> dev->port_id);
> > +        goto out;
> > +    }
> > +    /* Reserve memory for xstats names */
> > +    rte_xstats_names = dpdk_rte_mzalloc(sizeof(*rte_xstats_names)
> > +                                        * rte_xstats_len);
> > +    if (rte_xstats_names == NULL) {
> >
> > Minor: how about !rte_xstats_names?
> >
> > +        VLOG_WARN("Cannot allocate memory for XSTATS names for port %i",
> > +                  dev->port_id);
> > +        rte_free(rte_xstats_names);
> >
> > This rte_free seems unnecessary
> >
> > +        goto out;
> > +    }
> > +    /* Reserve memory for xstats values */
> > +    rte_xstats = dpdk_rte_mzalloc(sizeof(*rte_xstats) * rte_xstats_len);
> > +    if (rte_xstats == NULL) {
> >
> > Minor: how about !rte_xstats?
> >
> > +        VLOG_WARN("Cannot allocate memory for XSTATS values for port
> %i",
> > +                  dev->port_id);
> >          rte_free(rte_xstats);
> >
> > This rte_free seems unnecessary.
> >
> > +        goto out;
> >
> > I think here we would leak rte_xstats_names.
> >
> > +    }
> > +    /* Retreive xstats names */
> > +    if (rte_xstats_len != rte_eth_xstats_get_names(dev->port_id,
> > +                                       rte_xstats_names,
> rte_xstats_len)) {
> > +        VLOG_WARN("Cannot get XSTATS names for port: %i.",
> dev->port_id);
> > +        rte_free(rte_xstats);
> > +        rte_free(rte_xstats_names);
> > +        goto out;
> > +    }
> > +    /* Retreive xstats values */
> > +    memset(rte_xstats, 0xff, sizeof(*rte_xstats) * rte_xstats_len);
> > +    rte_xstats_ret = rte_eth_xstats_get(dev->port_id, rte_xstats,
> > +                                        rte_xstats_len);
> > +    if (rte_xstats_ret > 0 && rte_xstats_ret <= rte_xstats_len) {
> > +        netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
> > +                                   rte_xstats_len);
> >
> > Who frees rte_xstats_names and rte_xstats?
> >
> >      } else {
> > -        VLOG_WARN("Can't get XSTATS counters for port: %i.",
> dev->port_id);
> > +        VLOG_WARN("Cannot get XSTATS values for port: %i.",
> dev->port_id);
> > +        rte_free(rte_xstats);
> > +        rte_free(rte_xstats_names);
> >      }
> >
> > +out:
> >
> > Perhaps it's easier to always free rte_xstats and rte_xstats_names here.
> I've fixed this in the v3 and moved the free as suggested.
>
> > Also, is there a reason not to use xcalloc() and free(), instead?
> > This is not the fastpath, as far as I understand there's no reason for
> it to be in
> > hugepages.
> Just following what was already there. But good point - I've changed this
> to use xcalloc and free.
>
>
Ok, thanks


> >
> >      stats->rx_packets = rte_stats.ipackets;
> >      stats->tx_packets = rte_stats.opackets;
> >      stats->rx_bytes = rte_stats.ibytes;
>

[...]


> > @@ -2233,26 +2250,27 @@ netdev_dpdk_remap_txqs(struct netdev_dpdk
> > *dev)
> >   * A new virtio-net device is added to a vhost port.
> >   */
> >  static int
> > -new_device(struct virtio_net *virtio_dev)
> > +new_device(int vid)
> >  {
> >      struct netdev_dpdk *dev;
> >      bool exists = false;
> >      int newnode = 0;
> > -    long err = 0;
> > +    char ifname[PATH_MAX];
> > +
> > +    rte_vhost_get_ifname(vid, ifname, sizeof(ifname));
> >
> >      ovs_mutex_lock(&dpdk_mutex);
> >      /* Add device to the vhost port with the same name as that passed
> down.
> > */
> >      LIST_FOR_EACH(dev, list_node, &dpdk_list) {
> > -        if (strncmp(virtio_dev->ifname, dev->vhost_id, IF_NAME_SZ) ==
> 0) {
> > -            uint32_t qp_num = virtio_dev->virt_qp_nb;
> > +        if (strncmp(ifname, dev->vhost_id, PATH_MAX) == 0) {
> > +            uint32_t qp_num = rte_vhost_get_queue_num(vid);
> >
> >              ovs_mutex_lock(&dev->mutex);
> >              /* Get NUMA information */
> > -            err = get_mempolicy(&newnode, NULL, 0, virtio_dev,
> > -                                MPOL_F_NODE | MPOL_F_ADDR);
> > -            if (err) {
> > -                VLOG_INFO("Error getting NUMA info for vHost Device
> '%s'",
> > -                        virtio_dev->ifname);
> > +            newnode = rte_vhost_get_numa_node(vid);
> > +            if ((newnode != -1) && (newnode != dev->socket_id)) {
> > +                dev->requested_socket_id = newnode;
> > +            } else {
> >
> > Great, it looks like we don't need libnuma anymore.
> > I guess we can remove #include <numaif.h>.
> Correct - gone in v3.
>
> > I'm not sure if we can remove the build dependency, since if DPDK depends
> > on it we have to pass it to the linker.
> > Perhaps we can make it optional? In any case, this can be addressed by a
> > separate patch
> It's possible to make it optional by detecting if it's enabled in the DPDK
> build during configure.
> I've a rough patch for this, plan to submit soon.
>

Agreed, we can address this separately