[ovs-dev] [PATCH v4 2/3] netdev-dpdk: Add custom stat for vhost tx retries.

David Marchand david.marchand at redhat.com
Thu Jul 4 11:06:28 UTC 2019


On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets at samsung.com>
wrote:

> On 03.07.2019 18:03, Ian Stokes wrote:
> > On 7/2/2019 1:32 AM, Kevin Traynor wrote:
> >> vhost tx retries may occur, and it can be a sign that
> >> the guest is not optimally configured.
> >>
> >> Add a custom stat so a user will know if vhost tx retries are
> >> occurring and hence give a hint that guest config should be
> >> examined.
> >>
> >
> > Thanks Kevin, tests ok for me.
> >
> > Just a general comment on the design. In comparison to the previous
> approach proposed there seems to be more required with the custom stat
> approach below.
> >
> > This may be ok as the retry is very OVS DPDK specific and doesn;t come
> dor lets say DPDK (unlike the XTATS).
> >
> > @Ilya, whats your thoughts? Is this approach preferable for you rather
> than adding it to the general stats for all devices? (in which case I
> agree, they will not be used for dpdk types so will not be of use).
>
> I think, It's better to keep this in custom stats section since
> no other port types are going to use it in a near future.
>
> Have a few style/naming comments inline.
>
> >
> > One other minor comment below.
> >
> >> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> >> ---
> >>   Documentation/topics/dpdk/vhost-user.rst |  5 ++++
> >>   lib/netdev-dpdk.c                        | 38 +++++++++++++++++++++++-
> >>   2 files changed, 42 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst
> b/Documentation/topics/dpdk/vhost-user.rst
> >> index 5f393aced..368f44520 100644
> >> --- a/Documentation/topics/dpdk/vhost-user.rst
> >> +++ b/Documentation/topics/dpdk/vhost-user.rst
> >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores
> dedicated for consuming and
> >>   processing packets at the required rate.
> >>   +The amount of Tx retries on a vhost-user or vhost-user-client
> interface can be
> >> +shown with::
> >> +
> >> +  $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries
> >> +
> >>   vhost-user Dequeue Zero Copy (experimental)
> >>   -------------------------------------------
> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> >> index a380b6ffe..d3e02d389 100644
> >> --- a/lib/netdev-dpdk.c
> >> +++ b/lib/netdev-dpdk.c
> >> @@ -139,4 +139,10 @@ 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_STAT_TX_RETRIES_SIZE    1
>
> I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE.
>

+1


>
> >> +
> >> +/* Name of vHost custom stats. */
>
> s/Name/Names/ ?
>
> >> +#define VHOST_STAT_TX_RETRIES    "tx_retries" > +
> >
> > The alignment of the defines above look odd as they are not aligned with
> the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on
> leave I think this will be ok to change on commit if there are no
> objections.
>
> Sure.
>
> >
> > Regards
> > Ian
> >>   #define SOCKET0              0
> >>   @@ -434,7 +440,9 @@ struct netdev_dpdk {
> >>       PADDED_MEMBERS(CACHE_LINE_SIZE,
> >>           struct netdev_stats stats;
> >> +        /* Custom stat for retries when unable to transmit. */
> >> +        uint64_t tx_retries;
> >>           /* Protects stats */
> >>           rte_spinlock_t stats_lock;
> >> -        /* 44 pad bytes here. */
> >> +        /* 4 pad bytes here. */
> >>       );
> >>   @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev,
> dpdk_port_t port_no,
> >>       dev->rte_xstats_ids_size = 0;
> >>   +    dev->tx_retries = 0;
> >> +
> >>       return 0;
> >>   }
> >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev,
> int qid,
> >>       netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts,
> total_pkts,
> >>                                            cnt + dropped);
> >> +    dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM);
> >>       rte_spinlock_unlock(&dev->stats_lock);
> >>   @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct
> netdev *netdev,
> >>   }
> >>   +static int
> >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
> >> +                                   struct netdev_custom_stats
> *custom_stats)
> >> +{
> >> +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> >> +
> >> +    ovs_mutex_lock(&dev->mutex);
> >> +
> >> +    custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE;
> >> +    custom_stats->counters = (struct netdev_custom_counter *)
> >> +                                 xzalloc(sizeof(struct
> netdev_custom_counter));
>
> Since we have a _SIZE, we, probably, need to use it while allocation.
> With this and a few coding-style fixes, this line should look like:
>
>     custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE,
>                                      sizeof *custom_stats->counters);
>
>
Can I suggest?
    custom_stats->counters = xcalloc(custom_stats->size,
                                     sizeof *custom_stats->counters);

>> +    ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES,
> >> +                NETDEV_CUSTOM_STATS_NAME_SIZE);
> >> +
> >> +    rte_spinlock_lock(&dev->stats_lock);
> >> +    custom_stats->counters->value = dev->tx_retries;
>


I have a patch after this that will add another stats, so I'd rather see
custom_stats->counters[0]. than custom_stats->counters->



> >> +    rte_spinlock_unlock(&dev->stats_lock);
> >> +
> >> +    ovs_mutex_unlock(&dev->mutex);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>   static int
> >>   netdev_dpdk_get_features(const struct netdev *netdev,
> >> @@ -4398,4 +4432,5 @@ static const struct netdev_class dpdk_vhost_class
> = {
> >>       .get_carrier = netdev_dpdk_vhost_get_carrier,
> >>       .get_stats = netdev_dpdk_vhost_get_stats,
> >> +    .get_custom_stats = netdev_dpdk_vhost_get_custom_stats,
> >>       .get_status = netdev_dpdk_vhost_user_get_status,
> >>       .reconfigure = netdev_dpdk_vhost_reconfigure,
> >> @@ -4413,4 +4448,5 @@ static const struct netdev_class
> dpdk_vhost_client_class = {
> >>       .get_carrier = netdev_dpdk_vhost_get_carrier,
> >>       .get_stats = netdev_dpdk_vhost_get_stats,
> >> +    .get_custom_stats = netdev_dpdk_vhost_get_custom_stats,
> >>       .get_status = netdev_dpdk_vhost_user_get_status,
> >>       .reconfigure = netdev_dpdk_vhost_client_reconfigure,
> >>
>



-- 
David Marchand


More information about the dev mailing list