Hi Ilya, Thanks for the review. I agree with your proposal to move the stats update code to existing special functions. Thanks for the incremental patch, it looks good to me. Will wait for Kevin's taught on this.
Thanks & Regards, Sriram. -----Original Message----- From: Ilya Maximets <[email protected]> Sent: 11 November 2019 04:50 To: Sriram Vatala <[email protected]>; [email protected]; [email protected]; [email protected] Subject: Re: [PATCH v11 2/3] netdev-dpdk : Detailed packet drop statistics On 29.10.2019 15:50, Sriram Vatala wrote:> @@ -2388,12 +2412,16 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, > } > } while (cnt && (retries++ < max_retries)); > > + tx_failure = cnt; > rte_spinlock_unlock(&dev->tx_q[qid].tx_lock); > > rte_spinlock_lock(&dev->stats_lock); > netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, > cnt + dropped); > - dev->tx_retries += MIN(retries, max_retries); > + sw_stats->tx_retries += MIN(retries, max_retries); > + sw_stats->tx_failure_drops += tx_failure; > + sw_stats->tx_mtu_exceeded_drops += mtu_drops; > + sw_stats->tx_qos_drops += qos_drops; Kevin pointed to this part of code in hope that we can move this to a separate function and reuse in his review for v9. This code catches my eyes too. I don't think that we can reuse this part, at least this will be not very efficient in current situation (clearing of the unused fields in a stats structure will be probably needed before calling such a common function, but ETH tx uses only half of the struct). But there is another thing here. We already have special functions for vhost tx/rx counters. And it looks strange when we're not using these functions to update tx/rx failure counters. Suggesting following incremental. Kevin, Sriram, please, share your thoughts. diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 3cb7023a8..02120a379 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2169,16 +2169,18 @@ netdev_dpdk_vhost_update_rx_size_counters(struct netdev_stats *stats, } static inline void -netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats, +netdev_dpdk_vhost_update_rx_counters(struct netdev_dpdk *dev, struct dp_packet **packets, int count, - int dropped) + int qos_drops) { - int i; - unsigned int packet_size; + struct netdev_dpdk_sw_stats *sw_stats = dev->sw_stats; + struct netdev_stats *stats = &dev->stats; struct dp_packet *packet; + unsigned int packet_size; + int i; stats->rx_packets += count; - stats->rx_dropped += dropped; + stats->rx_dropped += qos_drops; for (i = 0; i < count; i++) { packet = packets[i]; packet_size = dp_packet_size(packet); @@ -2201,6 +2203,8 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats, stats->rx_bytes += packet_size; } + + sw_stats->rx_qos_drops += qos_drops; } /* @@ -2213,7 +2217,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq, struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev); struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev); uint16_t nb_rx = 0; - uint16_t dropped = 0; + uint16_t qos_drops = 0; int qid = rxq->queue_id * VIRTIO_QNUM + VIRTIO_TXQ; int vid = netdev_dpdk_get_vid(dev); @@ -2240,17 +2244,16 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq, } if (policer) { - dropped = nb_rx; + qos_drops = nb_rx; nb_rx = ingress_policer_run(policer, (struct rte_mbuf **) batch->packets, nb_rx, true); - dropped -= nb_rx; + qos_drops -= nb_rx; } rte_spinlock_lock(&dev->stats_lock); - netdev_dpdk_vhost_update_rx_counters(&dev->stats, batch->packets, - nb_rx, dropped); - dev->sw_stats->rx_qos_drops += dropped; + netdev_dpdk_vhost_update_rx_counters(dev, batch->packets, + nb_rx, qos_drops); rte_spinlock_unlock(&dev->stats_lock); batch->count = nb_rx; @@ -2360,13 +2363,18 @@ netdev_dpdk_filter_packet_len(struct netdev_dpdk *dev, struct rte_mbuf **pkts, } static inline void -netdev_dpdk_vhost_update_tx_counters(struct netdev_stats *stats, +netdev_dpdk_vhost_update_tx_counters(struct netdev_dpdk *dev, struct dp_packet **packets, int attempted, - int dropped) + struct netdev_dpdk_sw_stats + *sw_stats_add) { - int i; + struct netdev_dpdk_sw_stats *sw_stats = dev->sw_stats; + int dropped = sw_stats_add->tx_mtu_exceeded_drops + + sw_stats_add->tx_qos_drops + + sw_stats_add->tx_failure_drops; + struct netdev_stats *stats = &dev->stats; int sent = attempted - dropped; + int i; stats->tx_packets += sent; stats->tx_dropped += dropped; @@ -2374,6 +2382,11 @@ netdev_dpdk_vhost_update_tx_counters(struct netdev_stats *stats, for (i = 0; i < sent; i++) { stats->tx_bytes += dp_packet_size(packets[i]); } + + sw_stats->tx_retries += sw_stats_add->tx_retries; + sw_stats->tx_failure_drops += sw_stats_add->tx_failure_drops; + sw_stats->tx_mtu_exceeded_drops += sw_stats_add->tx_mtu_exceeded_drops; + sw_stats->tx_qos_drops += sw_stats_add->tx_qos_drops; } static void @@ -2382,12 +2395,9 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, { struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); struct rte_mbuf **cur_pkts = (struct rte_mbuf **) pkts; - struct netdev_dpdk_sw_stats *sw_stats = dev->sw_stats; - unsigned int total_pkts = cnt; - unsigned int dropped = 0; - unsigned int tx_failure; - unsigned int mtu_drops; - unsigned int qos_drops; + struct netdev_dpdk_sw_stats sw_stats_add; + unsigned int n_packets_to_free = cnt; + unsigned int total_packets = cnt; int i, retries = 0; int max_retries = VHOST_ENQ_RETRY_MIN; int vid = netdev_dpdk_get_vid(dev); @@ -2408,12 +2418,14 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, } cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt); - mtu_drops = total_pkts - cnt; - qos_drops = cnt; + sw_stats_add.tx_mtu_exceeded_drops = total_packets - cnt; + /* Check has QoS has been configured for the netdev */ + sw_stats_add.tx_qos_drops = cnt; cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true); - qos_drops -= cnt; - dropped = qos_drops + mtu_drops; + sw_stats_add.tx_qos_drops -= cnt; + + n_packets_to_free = cnt; do { int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ; @@ -2438,20 +2450,18 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, } } while (cnt && (retries++ < max_retries)); - tx_failure = cnt; rte_spinlock_unlock(&dev->tx_q[qid].tx_lock); + sw_stats_add.tx_failure_drops = cnt; + sw_stats_add.tx_retries = MIN(retries, max_retries); + rte_spinlock_lock(&dev->stats_lock); - netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, - cnt + dropped); - sw_stats->tx_retries += MIN(retries, max_retries); - sw_stats->tx_failure_drops += tx_failure; - sw_stats->tx_mtu_exceeded_drops += mtu_drops; - sw_stats->tx_qos_drops += qos_drops; + netdev_dpdk_vhost_update_tx_counters(dev, pkts, total_packets, + &sw_stats_add); rte_spinlock_unlock(&dev->stats_lock); out: - for (i = 0; i < total_pkts - dropped; i++) { + for (i = 0; i < n_packets_to_free; i++) { dp_packet_delete(pkts[i]); } } --- Best regards, Ilya Maximets.
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
