On 04.07.2019 14:06, David Marchand wrote:
On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maxim...@samsung.com
<mailto:i.maxim...@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 <ktray...@redhat.com
<mailto:ktray...@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.
>> + 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
