[ovs-dev] [PATCH] netdev-dpdk: Add a statistic on vhost tx contention.

David Marchand david.marchand at redhat.com
Mon Jul 8 12:23:28 UTC 2019


On Mon, Jul 8, 2019 at 1:41 PM Eelco Chaudron <echaudro at redhat.com> wrote:

>
>
> On 7 Jul 2019, at 18:13, David Marchand wrote:
>
> > Add a statistic to help diagnose contention on the vhost txqs.
> > This is usually seen as dropped packets on the physical ports for
> > rates
> > that are usually handled fine by OVS.
> >
> > Signed-off-by: David Marchand <david.marchand at redhat.com>
> > ---
> > This patch applies on top of Kevin "vhost-retry" series [1].
> >
> > 1:
> > https://patchwork.ozlabs.org/project/openvswitch/list/?submitter=70482
> > ---
> >  Documentation/topics/dpdk/vhost-user.rst | 25
> > +++++++++++++++++++++++++
> >  lib/netdev-dpdk.c                        | 20 ++++++++++++++++++--
> >  2 files changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/topics/dpdk/vhost-user.rst
> > b/Documentation/topics/dpdk/vhost-user.rst
> > index 724aa62..e276b21 100644
> > --- a/Documentation/topics/dpdk/vhost-user.rst
> > +++ b/Documentation/topics/dpdk/vhost-user.rst
> > @@ -553,6 +553,31 @@ shown with::
> >
> >    $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries
> >
> > +vhost tx contention
> > +-------------------
> > +
> > +Contrary to physical ports where the number of Transmit queues is
> > decided at
> > +the configuration of the port and does not change once set up, the
> > number of
> > +Transmit queues for a vhost-user or vhost-user-client interface is
> > negociated
> > +with the guest when its driver enables a virtio interface and OVS has
> > no
> > +control over it.
> > +
> > +Each PMD threads needs a Transmit queue to send packet on such a
> > port.
> > +To accomodate with the cases where the number of PMD threads differs
> > from the
>
> Maybe make it more explicit, i.e. where the number of PMD threads is
> higher than the number of tx queues.
>

Ok, I will try to re-phrase this in v2.



> > +number of Transmit queues enabled by a Virtual Machine, the Transmit
> > queues are
> > +distributed among the PMD threads and their accesses are protected by
> > a
> > +spinlock.
> > +
> > +When contention occurs (because two PMD threads are mapped to the
> > same Transmit
> > +queue), it can be hard to understand this phenomenon.
> > +It usually will be seen as OVS taking more cycles to handle the
> > packet and can
> > +be seen as packets getting dropped on the receive side (rx_dropped
> > statistic
> > +on the physical ports).
> > +
> > +A statistic for the port is available to catch those cases::
> > +
> > +  ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_contentions
> > +
> >  vhost-user Dequeue Zero Copy (experimental)
> >  -------------------------------------------
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 0e70e44..844fb3c 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -139,10 +139,11 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF /
> > ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF))
> >  #define XSTAT_RX_JABBER_ERRORS           "rx_jabber_errors"
> >
> >  /* Size of vHost custom stats. */
> > -#define VHOST_CUSTOM_STATS_SIZE          1
> > +#define VHOST_CUSTOM_STATS_SIZE          2
> >
> >  /* Names of vHost custom stats. */
> >  #define VHOST_STAT_TX_RETRIES            "tx_retries"
> > +#define VHOST_STAT_TX_CONTENTIONS        "tx_contentions"
> >
> >  #define SOCKET0              0
> >
> > @@ -340,6 +341,8 @@ struct dpdk_tx_queue {
> >                                      * pmd threads (see
> > 'concurrent_txq'). */
> >      int map;                       /* Mapping of configured
> > vhost-user queues
> >                                      * to enabled by guest. */
> > +    uint64_t tx_contentions;       /* Number of times a pmd has been
> > waiting
> > +                                    * for another to xmit on this
> > queue. */
> >  };
> >
> >  /* dpdk has no way to remove dpdk ring ethernet devices
> > @@ -2387,7 +2390,10 @@ __netdev_dpdk_vhost_send(struct netdev *netdev,
> > int qid,
> >          goto out;
> >      }
> >
> > -    rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +    if (unlikely(!rte_spinlock_trylock(&dev->tx_q[qid].tx_lock))) {
> > +        rte_spinlock_lock(&dev->tx_q[qid].tx_lock);
> > +        atomic_count_inc64(&dev->tx_q[qid].tx_contentions);
>
> I would swap the two lines above, as the atomic will probably finish
> before the lock is free’ed.
>

Indeed, better.


> > +    }
> >
> >      cnt = netdev_dpdk_filter_packet_len(dev, cur_pkts, cnt);
> >      /* Check has QoS has been configured for the netdev */
> > @@ -2878,6 +2884,16 @@ netdev_dpdk_vhost_get_custom_stats(const struct
> > netdev *netdev,
> >      custom_stats->counters[0].value = dev->tx_retries;
> >      rte_spinlock_unlock(&dev->stats_lock);
> >
> > +    ovs_strlcpy(custom_stats->counters[1].name,
> > VHOST_STAT_TX_CONTENTIONS,
>
> Can we maybe do something smart with the index, [1], used below in each
> case (and above [0]), as it seems prone to error.
>

I had in mind a little index variable that is incremented each time a
statistic is filled in this function.
This could be done in Kevin patch (that Ian is currently looking into
merging).
Or I will introduce it with my addition.



> > +                NETDEV_CUSTOM_STATS_NAME_SIZE);
> > +    custom_stats->counters[1].value = 0;
> > +    for (unsigned int i = 0; i < netdev->n_txq; i++) {
> > +        uint64_t tx_contentions;
> > +
> > +        atomic_read_relaxed(&dev->tx_q[i].tx_contentions,
> > &tx_contentions);
> > +        custom_stats->counters[1].value += tx_contentions;
> > +    }
> > +
> >      ovs_mutex_unlock(&dev->mutex);
> >
> >      return 0;
> > --
> > 1.8.3.1
>


-- 
David Marchand


More information about the dev mailing list