On 11/23/22 11:39, David Marchand wrote: > I have a few nits on my own patch. > Noting them here for now. > > In case there is no further comment on the patch, I will send a new revision.
I didn't actually review the patch yet, but if you can address comments below and re-post for a master branch (since 22.11 support is now accepted), that would be great. Thanks! Best regards, Ilya Maximets. > > > On Wed, Nov 9, 2022 at 9:39 PM David Marchand <[email protected]> > wrote: >> @@ -2845,8 +2779,7 @@ netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >> stats.tx_retries = MIN(retries, max_retries); >> >> rte_spinlock_lock(&dev->stats_lock); >> - netdev_dpdk_vhost_update_tx_counters(dev, batch->packets, batch_cnt, >> - &stats); >> + netdev_dpdk_vhost_update_tx_counters(dev, &stats); >> rte_spinlock_unlock(&dev->stats_lock); >> >> pkts = (struct rte_mbuf **) batch->packets; >> @@ -3001,41 +2934,304 @@ netdev_dpdk_set_mtu(struct netdev *netdev, int mtu) >> return 0; >> } >> >> -static int >> -netdev_dpdk_get_carrier(const struct netdev *netdev, bool *carrier); >> - >> static int >> netdev_dpdk_vhost_get_stats(const struct netdev *netdev, >> struct netdev_stats *stats) >> { >> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> + struct rte_vhost_stat_name *vhost_stats_names = NULL; >> + struct rte_vhost_stat *vhost_stats = NULL; >> + int vhost_stats_count; >> + int err = -1; > > Rather than set to -1 ... > > >> + int qid; >> + int vid; >> + > > (useless empty line) > > >> >> ovs_mutex_lock(&dev->mutex); >> >> - rte_spinlock_lock(&dev->stats_lock); >> - /* Supported Stats */ >> - stats->rx_packets = dev->stats.rx_packets; >> - stats->tx_packets = dev->stats.tx_packets; >> + if (!is_vhost_running(dev)) { > > ... it is more consistent to set err to EPROTO here. > > >> + goto out; >> + } >> + >> + vid = netdev_dpdk_get_vid(dev); >> + >> + /* We expect all rxqs have the same number of stats, only query rxq0. */ >> + qid = 0 * VIRTIO_QNUM + VIRTIO_TXQ; >> + err = rte_vhost_vring_stats_get_names(vid, qid, NULL, 0); >> + if (err < 0) { >> + err = EPROTO; >> + goto out; >> + } >> + >> + vhost_stats_count = err; >> + vhost_stats_names = xcalloc(vhost_stats_count, sizeof >> *vhost_stats_names); >> + vhost_stats = xcalloc(vhost_stats_count, sizeof *vhost_stats); >> + >> + err = rte_vhost_vring_stats_get_names(vid, qid, vhost_stats_names, >> + vhost_stats_count); >> + if (err != vhost_stats_count) { >> + err = EPROTO; >> + goto out; >> + } >> + >> +#define VHOST_PER_RXQ_STATS \ > > VHOST_RXQ_STATS is shorter and enough. > > >> + VHOST_PER_RXQ_STAT(rx_packets, "good_packets") \ >> + VHOST_PER_RXQ_STAT(rx_bytes, "good_bytes") \ >> + VHOST_PER_RXQ_STAT(rx_broadcast_packets, "broadcast_packets") \ >> + VHOST_PER_RXQ_STAT(multicast, "multicast_packets") \ >> + VHOST_PER_RXQ_STAT(rx_undersized_errors, "undersize_packets") \ >> + VHOST_PER_RXQ_STAT(rx_1_to_64_packets, "size_64_packets") \ >> + VHOST_PER_RXQ_STAT(rx_65_to_127_packets, "size_65_127_packets") \ >> + VHOST_PER_RXQ_STAT(rx_128_to_255_packets, "size_128_255_packets") \ >> + VHOST_PER_RXQ_STAT(rx_256_to_511_packets, "size_256_511_packets") \ >> + VHOST_PER_RXQ_STAT(rx_512_to_1023_packets, "size_512_1023_packets") \ >> + VHOST_PER_RXQ_STAT(rx_1024_to_1522_packets, "size_1024_1518_packets") \ >> + VHOST_PER_RXQ_STAT(rx_1523_to_max_packets, "size_1519_max_packets") > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
