[ovs-dev] [PATCH v8] netdev-dpdk:Detailed packet drop statistics

Ilya Maximets i.maximets at samsung.com
Thu Sep 12 12:36:42 UTC 2019


On 10.09.2019 20:31, Sriram Vatala wrote:
> -----Original Message-----
> From: Ilya Maximets <i.maximets at samsung.com>
> Sent: 10 September 2019 19:29
> To: Sriram Vatala <sriram.v at altencalsoftlabs.com>; ovs-dev at openvswitch.org
> Cc: ktraynor at redhat.com; ian.stokes at intel.com
> Subject: Re: [PATCH v8] netdev-dpdk:Detailed packet drop statistics
> 
> On 08.09.2019 19:01, Sriram Vatala wrote:
>> OVS may be unable to transmit packets for multiple reasons and today
>> there is a single counter to track packets dropped due to any of those
>> reasons. The most common reason is that a VM is unable to read packets
>> fast enough causing the vhostuser port transmit queue on the OVS side
>> to become full. This manifests as a problem with VNFs not receiving
>> all packets. Having a separate drop counter to track packets dropped
>> because the transmit queue is full will clearly indicate that the
>> problem is on the VM side and not in OVS. Similarly maintaining
>> separate counters for all possible drops helps in indicating sensible
>> cause for packet drops.
>>
>> This patch adds custom software stats counters to track packets
>> dropped at port level and these counters are displayed along with
>> other stats in "ovs-vsctl get interface <iface> statistics"
>> command. The detailed stats will be available for both dpdk and
>> vhostuser ports.
>>
>> Signed-off-by: Sriram Vatala <sriram.v at altencalsoftlabs.com>
>> ---
>> Changes since v7 :
>> Defined structure netdev_dpdk_sw_stats and moved all custom stats into
>> it.
>> Placed a pointer to netdev_dpdk_sw_stats structure in netdev_dpdk
>> strucure.
>> stats are reported with prefix with netdev_dpdk
>> ---
>>  include/openvswitch/netdev.h                  |  14 +++
>>  lib/netdev-dpdk.c                             | 109 +++++++++++++++---
>>  utilities/bugtool/automake.mk                 |   3 +-
>>  utilities/bugtool/ovs-bugtool-get-port-stats  |  25 ++++
>>  .../plugins/network-status/openvswitch.xml    |   1 +
>>  vswitchd/vswitch.xml                          |  24 ++++
>>  6 files changed, 156 insertions(+), 20 deletions(-)  create mode
>> 100755 utilities/bugtool/ovs-bugtool-get-port-stats
>>
>> diff --git a/include/openvswitch/netdev.h
>> b/include/openvswitch/netdev.h index 0c10f7b48..c17e6a97d 100644
>> --- a/include/openvswitch/netdev.h
>> +++ b/include/openvswitch/netdev.h
>> @@ -89,6 +89,20 @@ struct netdev_stats {
>>      uint64_t rx_jabber_errors;
>>  };
>>
>> +/* Custom software stats for dpdk ports */ struct
>> +netdev_dpdk_sw_stats {
>> +    /* No. of retries when unable to transmit. */
>> +    uint64_t tx_retries;
>> +    /* Pkt drops when unable to transmit; Probably Tx queue is full */
>> +    uint64_t tx_failure_drops;
>> +    /* Pkt len greater than max dev MTU */
>> +    uint64_t tx_mtu_exceeded_drops;
>> +    /* Pkt drops in egress policier processing */
>> +    uint64_t tx_qos_drops;
>> +    /* Pkt drops in ingress policier processing */
>> +    uint64_t rx_qos_drops;
>> +};
> 
> This should not be in a common header since the only user is netdev-dpdk.c.
> Regarding this code itself:
> 1. s/policier/policer/
> 2. s/Pkt/Packet/, s/len/length/
> 3. s/max dev MTU/device MTU/ (MTU already has 'maximum' word).
> 4. All comments should end with a period.
> 
> Sorry I missed to check spell. I will fix this in my next patch v9
> I will move this structure definition to netdev-dpdk.c  and make it static.
> 
>> +
>>  /* Structure representation of custom statistics counter */  struct
>> netdev_custom_counter {
>>      uint64_t value;
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>> bc20d6843..39aab4672 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -413,11 +413,10 @@ struct netdev_dpdk {
>>
>>      PADDED_MEMBERS(CACHE_LINE_SIZE,
>>          struct netdev_stats stats;
>> -        /* Custom stat for retries when unable to transmit. */
>> -        uint64_t tx_retries;
>> +        struct netdev_dpdk_sw_stats *sw_stats;
>>          /* Protects stats */
>>          rte_spinlock_t stats_lock;
>> -        /* 4 pad bytes here. */
>> +        /* 36 pad bytes here. */
>>      );
>>
>>      PADDED_MEMBERS(CACHE_LINE_SIZE,
>> @@ -1171,7 +1170,8 @@ common_construct(struct netdev *netdev, dpdk_port_t 
>> port_no,
>>      dev->rte_xstats_ids = NULL;
>>      dev->rte_xstats_ids_size = 0;
>>
>> -    dev->tx_retries = 0;
>> +    dev->sw_stats = (struct netdev_dpdk_sw_stats *)
>> +                        xcalloc(1,sizeof(struct
>> + netdev_dpdk_sw_stats));
> 
> This should be:
>        dev->sw_stats = xzalloc(sizeof *dev->sw_stats); Or
>        dev->sw_stats = dpdk_rte_mzalloc(sizeof *dev->sw_stats);
> 
> The later variant will require proper error handling as it could return NULL.
> I will change this in next patch v9.
> 
> See the Documentation/internals/contributing/coding-style.rst for details.
> 
>>
>>      return 0;
>>  }
>> @@ -1357,6 +1357,7 @@ common_destruct(struct netdev_dpdk *dev)
>>      ovs_list_remove(&dev->list_node);
>>      free(ovsrcu_get_protected(struct ingress_policer *,
>>                                &dev->ingress_policer));
>> +    free(dev->sw_stats);
>>      ovs_mutex_destroy(&dev->mutex);
>>  }
>>
>> @@ -2171,6 +2172,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
>>      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);
>>
>> @@ -2202,11 +2204,13 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq *rxq,
>>                                      (struct rte_mbuf **) batch->packets,
>>                                      nb_rx, true);
>>          dropped -= nb_rx;
>> +        qos_drops = dropped;
> 
> 'dropped' here already counts 'qos_drops' only.  I don't think we need an 
> extra variable here.
> I used separate variable for readability purpose. I can remove this.

You could remove this and, optionally, rename the current counter, if you want.

> 
>>      }
>>
>>      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 += qos_drops;
>>      rte_spinlock_unlock(&dev->stats_lock);
>>
>>      batch->count = nb_rx;
>> @@ -2232,6 +2236,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct 
>> dp_packet_batch *batch,
>>      struct ingress_policer *policer = netdev_dpdk_get_ingress_policer(dev);
>>      int nb_rx;
>>      int dropped = 0;
>> +    int qos_drops = 0;
>>
>>      if (OVS_UNLIKELY(!(dev->flags & NETDEV_UP))) {
>>          return EAGAIN;
>> @@ -2250,12 +2255,14 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct 
>> dp_packet_batch *batch,
>>                                      (struct rte_mbuf **) batch->packets,
>>                                      nb_rx, true);
>>          dropped -= nb_rx;
>> +        qos_drops = dropped;
> 
> Same here.
> 
>>      }
>>
>>      /* Update stats to reflect dropped packets */
>>      if (OVS_UNLIKELY(dropped)) {
>>          rte_spinlock_lock(&dev->stats_lock);
>>          dev->stats.rx_dropped += dropped;
>> +        dev->sw_stats->rx_qos_drops += qos_drops;
>>          rte_spinlock_unlock(&dev->stats_lock);
>>      }
>>
>> @@ -2339,6 +2346,10 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int 
>> qid,
>>      struct rte_mbuf **cur_pkts = (struct rte_mbuf **) pkts;
>>      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 = dev->sw_stats;
>>      int i, retries = 0;
>>      int max_retries = VHOST_ENQ_RETRY_MIN;
>>      int vid = netdev_dpdk_get_vid(dev); @@ -2356,9 +2367,12 @@
>> __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>>      rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
>>
>>      cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
>> +    mtu_drops = total_pkts - cnt;
>> +    qos_drops = cnt;
>>      /* Check has QoS has been configured for the netdev */
>>      cnt = netdev_dpdk_qos_run(dev, cur_pkts, cnt, true);
>> -    dropped = total_pkts - cnt;
>> +    qos_drops -= cnt;
>> +    dropped = qos_drops + mtu_drops;
>>
>>      do {
>>          int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ; @@ -2383,12
>> +2397,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;
>>      rte_spinlock_unlock(&dev->stats_lock);
>>
>>  out:
>> @@ -2413,12 +2431,16 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, 
>> struct dp_packet_batch *batch)
>>      struct rte_mbuf *pkts[PKT_ARRAY_SIZE];
>>      uint32_t cnt = batch_cnt;
>>      uint32_t dropped = 0;
>> +    uint32_t tx_failure = 0;
>> +    uint32_t mtu_drops = 0;
>> +    uint32_t qos_drops = 0;
>> +    struct netdev_dpdk_sw_stats *sw_stats = dev->sw_stats;
>>
>>      if (dev->type != DPDK_DEV_VHOST) {
>>          /* Check if QoS has been configured for this netdev. */
>>          cnt = netdev_dpdk_qos_run(dev, (struct rte_mbuf **) batch->packets,
>>                                    batch_cnt, false);
>> -        dropped += batch_cnt - cnt;
>> +        qos_drops = batch_cnt - cnt;
>>      }
>>
>>      uint32_t txcnt = 0;
>> @@ -2431,7 +2453,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct 
>> dp_packet_batch *batch)
>>              VLOG_WARN_RL(&rl, "Too big size %u max_packet_len %d",
>>                           size, dev->max_packet_len);
>>
>> -            dropped++;
>> +            mtu_drops++;
>>              continue;
>>          }
>>
>> @@ -2454,13 +2476,17 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, 
>> struct dp_packet_batch *batch)
>>              __netdev_dpdk_vhost_send(netdev, qid, (struct dp_packet **) 
>> pkts,
>>                                       txcnt);
>>          } else {
>> -            dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, txcnt);
>> +            tx_failure = netdev_dpdk_eth_tx_burst(dev, qid, pkts,
>> + txcnt);
>>          }
>>      }
>>
>> +    dropped += qos_drops + mtu_drops + tx_failure;
>>      if (OVS_UNLIKELY(dropped)) {
>>          rte_spinlock_lock(&dev->stats_lock);
>>          dev->stats.tx_dropped += dropped;
>> +        sw_stats->tx_failure_drops += tx_failure;
>> +        sw_stats->tx_mtu_exceeded_drops += mtu_drops;
>> +        sw_stats->tx_qos_drops += qos_drops;
>>          rte_spinlock_unlock(&dev->stats_lock);
>>      }
>>  }
>> @@ -2485,6 +2511,8 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,
>>                     struct dp_packet_batch *batch,
>>                     bool concurrent_txq)  {
>> +    struct netdev_dpdk_sw_stats *sw_stats = dev->sw_stats;
> 
> This should be defined under the 'else' branch.
> Sorry, I missed this. I will move this.
>> +
>>      if (OVS_UNLIKELY(!(dev->flags & NETDEV_UP))) {
>>          dp_packet_delete_batch(batch, true);
>>          return;
>> @@ -2502,18 +2530,25 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,
>>          dp_packet_delete_batch(batch, true);
>>      } else {
>>          int tx_cnt, dropped;
>> +        int tx_failure, mtu_drops, qos_drops;
>>          int batch_cnt = dp_packet_batch_size(batch);
>>          struct rte_mbuf **pkts = (struct rte_mbuf **) batch->packets;
>>
>>          tx_cnt = netdev_dpdk_filter_packet_len(dev, pkts, batch_cnt);
>> +        mtu_drops = batch_cnt - tx_cnt;
>> +        qos_drops = tx_cnt;
>>          tx_cnt = netdev_dpdk_qos_run(dev, pkts, tx_cnt, true);
>> -        dropped = batch_cnt - tx_cnt;
>> +        qos_drops -= tx_cnt;
>>
>> -        dropped += netdev_dpdk_eth_tx_burst(dev, qid, pkts, tx_cnt);
>> +        tx_failure = netdev_dpdk_eth_tx_burst(dev, qid, pkts,
>> + tx_cnt);
>>
>> +        dropped = tx_failure + mtu_drops + qos_drops;
>>          if (OVS_UNLIKELY(dropped)) {
>>              rte_spinlock_lock(&dev->stats_lock);
>>              dev->stats.tx_dropped += dropped;
>> +            sw_stats->tx_failure_drops += tx_failure;
>> +            sw_stats->tx_mtu_exceeded_drops += mtu_drops;
>> +            sw_stats->tx_qos_drops += qos_drops;
>>              rte_spinlock_unlock(&dev->stats_lock);
>>          }
>>      }
>> @@ -2772,6 +2807,17 @@ netdev_dpdk_get_custom_stats(const struct netdev 
>> *netdev,
>>      uint32_t i;
>>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>      int rte_xstats_ret;
>> +    uint16_t index = 0;
>> +
>> +#define DPDK_CSTATS                         \
>> +    DPDK_CSTAT(tx_failure_drops)            \
>> +    DPDK_CSTAT(tx_mtu_exceeded_drops)       \
>> +    DPDK_CSTAT(tx_qos_drops)                \
>> +    DPDK_CSTAT(rx_qos_drops)
>> +
>> +#define DPDK_CSTAT(NAME) +1
>> +    custom_stats->size = DPDK_CSTATS; #undef DPDK_CSTAT
>>
>>      ovs_mutex_lock(&dev->mutex);
>>
>> @@ -2786,9 +2832,10 @@ netdev_dpdk_get_custom_stats(const struct netdev 
>> *netdev,
>>          if (rte_xstats_ret > 0 &&
>>              rte_xstats_ret <= dev->rte_xstats_ids_size) {
>>
>> -            custom_stats->size = rte_xstats_ret;
>> +            index = rte_xstats_ret;
>> +            custom_stats->size += rte_xstats_ret;
>>              custom_stats->counters =
>> -                    (struct netdev_custom_counter *) 
>> xcalloc(rte_xstats_ret,
>> +                   (struct netdev_custom_counter *)
>> + xcalloc(custom_stats->size,
>>                              sizeof(struct netdev_custom_counter));
>>
>>              for (i = 0; i < rte_xstats_ret; i++) { @@ -2802,7 +2849,6
>> @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev,
>>              VLOG_WARN("Cannot get XSTATS values for port: 
>> "DPDK_PORT_ID_FMT,
>>                        dev->port_id);
>>              custom_stats->counters = NULL;
>> -            custom_stats->size = 0;
>>              /* Let's clear statistics cache, so it will be
>>               * reconfigured */
>>              netdev_dpdk_clear_xstats(dev); @@ -2811,6 +2857,27 @@
>> netdev_dpdk_get_custom_stats(const struct netdev *netdev,
>>          free(values);
>>      }
>>
>> +    if (custom_stats->counters == NULL) {
>> +        custom_stats->counters =
>> +            (struct netdev_custom_counter *) xcalloc(custom_stats->size,
>> +                                  sizeof(struct netdev_custom_counter));
>> +    }
>> +
>> +    rte_spinlock_lock(&dev->stats_lock);
>> +    i = index;
>> +#define DPDK_CSTAT(NAME)                                                \
>> +    ovs_strlcpy(custom_stats->counters[i++].name, "netdev_dpdk_"#NAME,  \
>> +                NETDEV_CUSTOM_STATS_NAME_SIZE);
>> +    DPDK_CSTATS;
>> +#undef DPDK_CSTAT
>> +
>> +    i = index;
>> +#define DPDK_CSTAT(NAME)                                     \
>> +    custom_stats->counters[i++].value = dev->sw_stats->NAME;
>> +    DPDK_CSTATS;
>> +#undef DPDK_CSTAT
>> +    rte_spinlock_unlock(&dev->stats_lock);
>> +
> 
> 
> Above is still a big code duplication. I think, it's better to rename
> netdev_dpdk_vhost_get_custom_stats() to something neutral like
> netdev_dpdk_get_sw_custom_stats() and re-use it here. DPDK xstats could be 
> added by calling xrealloc() on resulted sw custom_stats.
> For tracking unsupported statistics (tx_retries), we can use same trick as 
> bridge.c, i.e. setting UINT64_MAX for unsupported and skip them while 
> reporting.
> 
> What do you think?
> I could draft another refactoring patch for this if needed.
> 
> Yes. This can be done.  I can do this change in my next patch v9. You can 
> draft patch if you want this particular change to go as a separate patch. Let 
> me know your decision.

I've sent a patch with this, so you could rebase on it and send both as
a single patch-set.

> 
>>      ovs_mutex_unlock(&dev->mutex);
>>
>>      return 0;
>> @@ -2823,8 +2890,12 @@ netdev_dpdk_vhost_get_custom_stats(const struct 
>> netdev *netdev,
>>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>      int i;
>>
>> -#define VHOST_CSTATS \
>> -    VHOST_CSTAT(tx_retries)
>> +#define VHOST_CSTATS                        \
>> +    VHOST_CSTAT(tx_retries)                 \
>> +    VHOST_CSTAT(tx_failure_drops)           \
>> +    VHOST_CSTAT(tx_mtu_exceeded_drops)      \
>> +    VHOST_CSTAT(tx_qos_drops)               \
>> +    VHOST_CSTAT(rx_qos_drops)
>>
>>  #define VHOST_CSTAT(NAME) + 1
>>      custom_stats->size = VHOST_CSTATS; @@ -2832,8 +2903,8 @@
>> netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
>>      custom_stats->counters = xcalloc(custom_stats->size,
>>                                       sizeof *custom_stats->counters);
>>      i = 0;
>> -#define VHOST_CSTAT(NAME) \
>> -    ovs_strlcpy(custom_stats->counters[i++].name, #NAME, \
>> +#define VHOST_CSTAT(NAME)                                                \
>> +    ovs_strlcpy(custom_stats->counters[i++].name, "netdev_dpdk_"#NAME,   \
>>                  NETDEV_CUSTOM_STATS_NAME_SIZE);
>>      VHOST_CSTATS;
>>  #undef VHOST_CSTAT
>> @@ -2843,7 +2914,7 @@ netdev_dpdk_vhost_get_custom_stats(const struct netdev 
>> *netdev,
>>      rte_spinlock_lock(&dev->stats_lock);
>>      i = 0;
>>  #define VHOST_CSTAT(NAME) \
>> -    custom_stats->counters[i++].value = dev->NAME;
>> +    custom_stats->counters[i++].value = dev->sw_stats->NAME;
>>      VHOST_CSTATS;
>>  #undef VHOST_CSTAT
>>      rte_spinlock_unlock(&dev->stats_lock);
>> diff --git a/utilities/bugtool/automake.mk
>> b/utilities/bugtool/automake.mk index 40980b367..dda58e0a1 100644
>> --- a/utilities/bugtool/automake.mk
>> +++ b/utilities/bugtool/automake.mk
>> @@ -22,7 +22,8 @@ bugtool_scripts = \
>>  	utilities/bugtool/ovs-bugtool-ovs-bridge-datapath-type \
>>  	utilities/bugtool/ovs-bugtool-ovs-vswitchd-threads-affinity \
>>  	utilities/bugtool/ovs-bugtool-qos-configs \
>> -	utilities/bugtool/ovs-bugtool-get-dpdk-nic-numa
>> +	utilities/bugtool/ovs-bugtool-get-dpdk-nic-numa \
>> +	utilities/bugtool/ovs-bugtool-get-port-stats
>>
>>  scripts_SCRIPTS += $(bugtool_scripts)
>>
>> diff --git a/utilities/bugtool/ovs-bugtool-get-port-stats
>> b/utilities/bugtool/ovs-bugtool-get-port-stats
>> new file mode 100755
>> index 000000000..0fe175e6b
>> --- /dev/null
>> +++ b/utilities/bugtool/ovs-bugtool-get-port-stats
>> @@ -0,0 +1,25 @@
>> +#! /bin/bash
>> +
>> +# This library is free software; you can redistribute it and/or #
>> +modify it under the terms of version 2.1 of the GNU Lesser General #
>> +Public License as published by the Free Software Foundation.
>> +#
>> +# This library is distributed in the hope that it will be useful, #
>> +but WITHOUT ANY WARRANTY; without even the implied warranty of #
>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU #
>> +Lesser General Public License for more details.
>> +#
>> +# Copyright (C) 2019 Ericsson AB
>> +
>> +for bridge in `ovs-vsctl -- --real list-br` do
>> +    echo -e "\nBridge : ${bridge}\n"
>> +    for iface in `ovs-vsctl list-ifaces ${bridge}`
>> +    do
>> +        echo -e "iface : ${iface}"
>> +        ovs-vsctl get interface ${iface} statistics
>> +        echo -e "\n"
>> +    done
>> +    echo -e "iface : ${bridge}"
>> +    ovs-vsctl get interface ${bridge} statistics done
>> diff --git a/utilities/bugtool/plugins/network-status/openvswitch.xml
>> b/utilities/bugtool/plugins/network-status/openvswitch.xml
>> index d39867c6e..8c1ec643e 100644
>> --- a/utilities/bugtool/plugins/network-status/openvswitch.xml
>> +++ b/utilities/bugtool/plugins/network-status/openvswitch.xml
>> @@ -40,4 +40,5 @@
>>      <command label="ovs-ofctl-dump-groups" 
>> filters="ovs">/usr/share/openvswitch/scripts/ovs-bugtool-ovs-ofctl-loop-over-bridges 
>> "dump-groups"</command>
>>      <command label="ovs-ofctl-dump-group-stats" filters="ovs" 
>> repeat="2">/usr/share/openvswitch/scripts/ovs-bugtool-ovs-ofctl-loop-over-bridges 
>> "dump-group-stats"</command>
>>      <command label="get_dpdk_nic_numa"
>> filters="ovs">/usr/share/openvswitch/scripts/ovs-bugtool-get-dpdk-nic-
>> numa</command>
>> +    <command label="get_port_statistics"
>> + filters="ovs">/usr/share/openvswitch/scripts/ovs-bugtool-get-port-st
>> + ats</command>
>>  </collect>
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index
>> 9a743c05b..402f3c8ec 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -3486,6 +3486,30 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 
>> type=patch options:peer=p1 \
>>            the above.
>>          </column>
>>        </group>
>> +      <group title="Statistics: Custom Software stats for dpdk ports">
>> +        <column name="statistics" key="tx_retries">
>> +          Total number of transmit retries on a vhost-user or 
>> vhost-user-client
>> +          interface.
>> +        </column>
>> +        <column name="statistics" key="tx_failure_drops">
>> +          Total number of packets dropped because DPDP transmit API for
>> +          physical/vhost ports fails to transmit the packets. This happens
>> +          most likely because the transmit queue is full or has been filled
>> +          up. There are other reasons as well which are unlikely to happen.
>> +        </column>
>> +        <column name="statistics" key="tx_mtu_exceeded_drops">
>> +          Number of packets dropped due to packet length exceeding the max
>> +          device MTU.
>> +        </column>
>> +        <column name="statistics" key="tx_qos_drops">
>> +          Total number of packets dropped due to transmission rate 
>> exceeding
>> +          the configured egress policer rate.
>> +        </column>
>> +        <column name="statistics" key="rx_qos_drops">
>> +          Total number of packets dropped due to reception rate exceeding
>> +          the configured ingress policer rate.
>> +        </column>
>> +      </group>
> 
> I still think that we should not document these stats here.
> 
> I am bit unclear on where to document the stats. Initially I documented this 
> stats under Documentation/topics/dpdk/bridge.rst.  Later on Ben's suggestion I 
> moved this to vswitchd.xml

I'd drop the documentation entierly. The stats names should be self-describing.
The only one that could need some description is 'tx_failure_drops'. You may
mention it in vhost-user.rst close to tx_retries. It's unlikely to have tx
queue overflow on physical NIC and you're not describing other cases anyway,
so vhost-user manual could be a better place. Also, you need to rename
these stats in docs, since you're adding the 'netdev_dpdk_' prefix.

> 
> Thanks for your comments. I will fix the comments in my next patch v9.
> 
> Thanks & Regards,
> Sriram.
> 
>>      </group>
>>
>>      <group title="Ingress Policing">
>>


More information about the dev mailing list