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

Ian Stokes ian.stokes at intel.com
Thu Jul 4 15:17:39 UTC 2019


On 7/4/2019 4:15 PM, Ilya Maximets wrote:
> On 04.07.2019 18:13, Ian Stokes wrote:
>> On 7/4/2019 12:18 PM, Ilya Maximets wrote:
>>> On 04.07.2019 14:06, David Marchand wrote:
>>>>
>>>>
>>>> On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets at samsung.com <mailto: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 <mailto: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);
>>>
>>> Sure. This looks fine for me too.
>>>
>>>>
>>>>       >> +    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->
>>>
>>> +1.
>>> This is also in line with my previous comment about having a _SIZE.
>>
>> Just to clarify above, maybe I misunderstood what you meant, but are you suggesting
>>
>> custom_stats->counters[0] = dev->tx_retries;
>>
>> I don't think that will work as it will be an incorrect type assignment right?
> 
> custom_stats->counters[0].value = dev->tx_retries;
> 

:D, thats fine, what I thought but wanted to confirm.

Ian



More information about the dev mailing list