diff mbox series

[ovs-dev,v4,2/5] netdev-dummy: Introduce per rxq/txq statistics.

Message ID 20211217151018.436874-3-maxime.coquelin@redhat.com
State Superseded
Headers show
Series dpif-netdev: Hash-based Tx packet steering | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Maxime Coquelin Dec. 17, 2021, 3:10 p.m. UTC 
This patch adds Rx and Tx per-queue statistics. It will be
used to test hash-based Tx packet steering. Only "bytes",
and "packets" per-queue custom statistics are added, as
there are no global "errors" counters in netdev-dummy.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/netdev-dummy.c | 82 ++++++++++++++++++++++++++++++++++++++++++----
 tests/ofproto.at   |  3 +-
 2 files changed, 77 insertions(+), 8 deletions(-)

Comments

David Marchand Jan. 3, 2022, 6:43 p.m. UTC | #1 
On Fri, Dec 17, 2021 at 4:10 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch adds Rx and Tx per-queue statistics. It will be
> used to test hash-based Tx packet steering. Only "bytes",
> and "packets" per-queue custom statistics are added, as
> there are no global "errors" counters in netdev-dummy.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Similar to previous patch, I only have some nits.
The patch lgtm.
Reviewed-by: David Marchand <david.marchand@redhat.com>


> @@ -954,6 +966,8 @@ static int
>  netdev_dummy_reconfigure(struct netdev *netdev_)
>  {
>      struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
> +    int old_n_txq = netdev_->n_txq;
> +    int old_n_rxq = netdev_->n_rxq;
>
>      ovs_mutex_lock(&netdev->mutex);
>
> @@ -961,6 +975,24 @@ netdev_dummy_reconfigure(struct netdev *netdev_)
>      netdev_->n_rxq = netdev->requested_n_rxq;
>      netdev->numa_id = netdev->requested_numa_id;
>
> +    if (netdev_->n_txq != old_n_txq || netdev_->n_rxq != old_n_rxq) {
> +        /* Resize the per queue stats arrays */

Nit: comments must end with a .

> +        netdev->txq_stats = xrealloc(netdev->txq_stats,
> +                netdev_->n_txq * sizeof *netdev->txq_stats);
> +        netdev->rxq_stats = xrealloc(netdev->rxq_stats,
> +                netdev_->n_rxq * sizeof *netdev->rxq_stats);

Nit: Indent is off.

> +
> +        /* Reset all stats for consistency between per-queue and global
> +         * counters */

Nit: comments must end with a .

> +        memset(&netdev->stats, 0, sizeof netdev->stats);
> +        netdev->custom_stats[0].value = 0;
> +        netdev->custom_stats[1].value = 0;
> +        memset(netdev->txq_stats, 0,
> +                netdev_->n_txq * sizeof *netdev->txq_stats);
> +        memset(netdev->rxq_stats, 0,
> +                netdev_->n_rxq * sizeof *netdev->rxq_stats);

Nit: Indent is off.


> +    }
> +
>      ovs_mutex_unlock(&netdev->mutex);
>      return 0;
>  }

> @@ -1252,22 +1288,54 @@ static int
>  netdev_dummy_get_custom_stats(const struct netdev *netdev,
>                               struct netdev_custom_stats *custom_stats)
>  {
> -    int i;
> +    int i,j;

Nit: missing a space after ,

>
>      struct netdev_dummy *dev = netdev_dummy_cast(netdev);
>
> -    custom_stats->size = 2;
> +    ovs_mutex_lock(&dev->mutex);
> +
> +#define DUMMY_Q_STATS \
> +    DUMMY_Q_STAT(bytes) \
> +    DUMMY_Q_STAT(packets)
> +
> +    custom_stats->size = C_STATS_SIZE;
> +#define DUMMY_Q_STAT(NAME) + netdev->n_rxq
> +    custom_stats->size += DUMMY_Q_STATS;
> +#undef DUMMY_Q_STAT
> +#define DUMMY_Q_STAT(NAME) + netdev->n_txq
> +    custom_stats->size += DUMMY_Q_STATS;
> +#undef DUMMY_Q_STAT
> +
>      custom_stats->counters =
> -            (struct netdev_custom_counter *) xcalloc(C_STATS_SIZE,
> +            (struct netdev_custom_counter *) xcalloc(custom_stats->size,

Nit: it was there before your patch, but I don't think this cast is needed.


>                      sizeof(struct netdev_custom_counter));
>
> -    ovs_mutex_lock(&dev->mutex);
> +    j = 0;
>      for (i = 0 ; i < C_STATS_SIZE ; i++) {
> -        custom_stats->counters[i].value = dev->custom_stats[i].value;
> -        ovs_strlcpy(custom_stats->counters[i].name,
> +        custom_stats->counters[j].value = dev->custom_stats[i].value;
> +        ovs_strlcpy(custom_stats->counters[j++].name,
>                      dev->custom_stats[i].name,
>                      NETDEV_CUSTOM_STATS_NAME_SIZE);
>      }
> +
> +    for (i = 0; i < netdev->n_rxq; i++) {
> +#define DUMMY_Q_STAT(NAME) \
> +        snprintf(custom_stats->counters[j].name, \
> +                NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_"#NAME, i); \

Nit: indent is off.

> +        custom_stats->counters[j++].value = dev->rxq_stats[i].NAME;
> +        DUMMY_Q_STATS
> +#undef DUMMY_Q_STAT
> +    }
> +
> +    for (i = 0; i < netdev->n_txq; i++) {
> +#define DUMMY_Q_STAT(NAME) \
> +        snprintf(custom_stats->counters[j].name, \
> +                NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_"#NAME, i); \

Nit: indent is off.


> +        custom_stats->counters[j++].value = dev->txq_stats[i].NAME;
> +        DUMMY_Q_STATS
> +#undef DUMMY_Q_STAT
> +    }
> +
>      ovs_mutex_unlock(&dev->mutex);
>
>      return 0;
diff mbox series

Patch

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 1f386b81b..59a9efefd 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -101,6 +101,11 @@  struct offloaded_flow {
     uint32_t mark;
 };
 
+struct netdev_dummy_q_stats {
+    uint64_t bytes;
+    uint64_t packets;
+};
+
 /* Protects 'dummy_list'. */
 static struct ovs_mutex dummy_list_mutex = OVS_MUTEX_INITIALIZER;
 
@@ -121,6 +126,8 @@  struct netdev_dummy {
     int mtu OVS_GUARDED;
     struct netdev_stats stats OVS_GUARDED;
     struct netdev_custom_counter custom_stats[C_STATS_SIZE] OVS_GUARDED;
+    struct netdev_dummy_q_stats *rxq_stats OVS_GUARDED;
+    struct netdev_dummy_q_stats *txq_stats OVS_GUARDED;
     enum netdev_flags flags OVS_GUARDED;
     int ifindex OVS_GUARDED;
     int numa_id OVS_GUARDED;
@@ -707,6 +714,9 @@  netdev_dummy_construct(struct netdev *netdev_)
     ovs_strlcpy(netdev->custom_stats[1].name,
                 "rx_custom_packets_2", NETDEV_CUSTOM_STATS_NAME_SIZE);
 
+    netdev->rxq_stats = xcalloc(netdev->up.n_rxq, sizeof *netdev->rxq_stats);
+    netdev->txq_stats = xcalloc(netdev->up.n_rxq, sizeof *netdev->txq_stats);
+
     dummy_packet_conn_init(&netdev->conn);
 
     ovs_list_init(&netdev->rxes);
@@ -731,6 +741,8 @@  netdev_dummy_destruct(struct netdev *netdev_)
     ovs_mutex_unlock(&dummy_list_mutex);
 
     ovs_mutex_lock(&netdev->mutex);
+    free(netdev->rxq_stats);
+    free(netdev->txq_stats);
     if (netdev->rxq_pcap) {
         ovs_pcap_close(netdev->rxq_pcap);
     }
@@ -954,6 +966,8 @@  static int
 netdev_dummy_reconfigure(struct netdev *netdev_)
 {
     struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
+    int old_n_txq = netdev_->n_txq;
+    int old_n_rxq = netdev_->n_rxq;
 
     ovs_mutex_lock(&netdev->mutex);
 
@@ -961,6 +975,24 @@  netdev_dummy_reconfigure(struct netdev *netdev_)
     netdev_->n_rxq = netdev->requested_n_rxq;
     netdev->numa_id = netdev->requested_numa_id;
 
+    if (netdev_->n_txq != old_n_txq || netdev_->n_rxq != old_n_rxq) {
+        /* Resize the per queue stats arrays */
+        netdev->txq_stats = xrealloc(netdev->txq_stats,
+                netdev_->n_txq * sizeof *netdev->txq_stats);
+        netdev->rxq_stats = xrealloc(netdev->rxq_stats,
+                netdev_->n_rxq * sizeof *netdev->rxq_stats);
+
+        /* Reset all stats for consistency between per-queue and global
+         * counters */
+        memset(&netdev->stats, 0, sizeof netdev->stats);
+        netdev->custom_stats[0].value = 0;
+        netdev->custom_stats[1].value = 0;
+        memset(netdev->txq_stats, 0,
+                netdev_->n_txq * sizeof *netdev->txq_stats);
+        memset(netdev->rxq_stats, 0,
+                netdev_->n_rxq * sizeof *netdev->rxq_stats);
+    }
+
     ovs_mutex_unlock(&netdev->mutex);
     return 0;
 }
@@ -1049,7 +1081,9 @@  netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch,
     }
     ovs_mutex_lock(&netdev->mutex);
     netdev->stats.rx_packets++;
+    netdev->rxq_stats[rxq_->queue_id].packets++;
     netdev->stats.rx_bytes += dp_packet_size(packet);
+    netdev->rxq_stats[rxq_->queue_id].bytes += dp_packet_size(packet);
     netdev->custom_stats[0].value++;
     netdev->custom_stats[1].value++;
     ovs_mutex_unlock(&netdev->mutex);
@@ -1096,7 +1130,7 @@  netdev_dummy_rxq_drain(struct netdev_rxq *rxq_)
 }
 
 static int
-netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED,
+netdev_dummy_send(struct netdev *netdev, int qid,
                   struct dp_packet_batch *batch,
                   bool concurrent_txq OVS_UNUSED)
 {
@@ -1135,7 +1169,9 @@  netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED,
 
         ovs_mutex_lock(&dev->mutex);
         dev->stats.tx_packets++;
+        dev->txq_stats[qid].packets++;
         dev->stats.tx_bytes += size;
+        dev->txq_stats[qid].bytes += size;
 
         dummy_packet_conn_send(&dev->conn, buffer, size);
 
@@ -1252,22 +1288,54 @@  static int
 netdev_dummy_get_custom_stats(const struct netdev *netdev,
                              struct netdev_custom_stats *custom_stats)
 {
-    int i;
+    int i,j;
 
     struct netdev_dummy *dev = netdev_dummy_cast(netdev);
 
-    custom_stats->size = 2;
+    ovs_mutex_lock(&dev->mutex);
+
+#define DUMMY_Q_STATS \
+    DUMMY_Q_STAT(bytes) \
+    DUMMY_Q_STAT(packets)
+
+    custom_stats->size = C_STATS_SIZE;
+#define DUMMY_Q_STAT(NAME) + netdev->n_rxq
+    custom_stats->size += DUMMY_Q_STATS;
+#undef DUMMY_Q_STAT
+#define DUMMY_Q_STAT(NAME) + netdev->n_txq
+    custom_stats->size += DUMMY_Q_STATS;
+#undef DUMMY_Q_STAT
+
     custom_stats->counters =
-            (struct netdev_custom_counter *) xcalloc(C_STATS_SIZE,
+            (struct netdev_custom_counter *) xcalloc(custom_stats->size,
                     sizeof(struct netdev_custom_counter));
 
-    ovs_mutex_lock(&dev->mutex);
+    j = 0;
     for (i = 0 ; i < C_STATS_SIZE ; i++) {
-        custom_stats->counters[i].value = dev->custom_stats[i].value;
-        ovs_strlcpy(custom_stats->counters[i].name,
+        custom_stats->counters[j].value = dev->custom_stats[i].value;
+        ovs_strlcpy(custom_stats->counters[j++].name,
                     dev->custom_stats[i].name,
                     NETDEV_CUSTOM_STATS_NAME_SIZE);
     }
+
+    for (i = 0; i < netdev->n_rxq; i++) {
+#define DUMMY_Q_STAT(NAME) \
+        snprintf(custom_stats->counters[j].name, \
+                NETDEV_CUSTOM_STATS_NAME_SIZE, "rx_q%d_"#NAME, i); \
+        custom_stats->counters[j++].value = dev->rxq_stats[i].NAME;
+        DUMMY_Q_STATS
+#undef DUMMY_Q_STAT
+    }
+
+    for (i = 0; i < netdev->n_txq; i++) {
+#define DUMMY_Q_STAT(NAME) \
+        snprintf(custom_stats->counters[j].name, \
+                NETDEV_CUSTOM_STATS_NAME_SIZE, "tx_q%d_"#NAME, i); \
+        custom_stats->counters[j++].value = dev->txq_stats[i].NAME;
+        DUMMY_Q_STATS
+#undef DUMMY_Q_STAT
+    }
+
     ovs_mutex_unlock(&dev->mutex);
 
     return 0;
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 08c0a20b6..156d3e058 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -112,7 +112,8 @@  OFPST_PORT reply (OF1.4): 1 ports
            tx pkts=0, bytes=0, drop=?, errs=?, coll=?
            duration=?s
            CUSTOM Statistics
-                      rx_custom_packets_1=0, rx_custom_packets_2=0,
+                      rx_custom_packets_1=0, rx_custom_packets_2=0, rx_q0_bytes=0,
+                      rx_q0_packets=0, tx_q0_bytes=0, tx_q0_packets=0,
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP