On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <[email protected]> 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 <[email protected]> > >> --- > >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
