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. >> + >> +/* 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); >> + 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; >> + 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, >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
