[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 15:15:41 UTC 2019
On Thu, Jul 4, 2019 at 5:13 PM Ian Stokes <ian.stokes at intel.com> 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?
>
>
I meant:
ovs_strlcpy(custom_stats->counters[0].name, VHOST_STAT_TX_RETRIES,
NETDEV_CUSTOM_STATS_NAME_SIZE);
custom_stats->counters[0].value = dev->tx_retries;
--
David Marchand
More information about the dev
mailing list