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 <[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?
custom_stats->counters[0].value = dev->tx_retries; _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
