[...] > @@ -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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev