On Thu, Jul 4, 2019 at 5:13 PM Ian Stokes <[email protected]> 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 <[email protected] > <mailto:[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] <mailto: > [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); > > > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
