[ovs-dev] [PATCH v11 2/3] netdev-dpdk : Detailed packet drop statistics

Ilya Maximets i.maximets at ovn.org
Sun Nov 10 23:20:24 UTC 2019


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.


More information about the dev mailing list