On 15/10/2019 16:42, Ilya Maximets wrote: > On 15.10.2019 17:32, Sriram Vatala wrote: >> -----Original Message----- >> From: Kevin Traynor <[email protected]> >> Sent: 15 October 2019 19:21 >> To: Ilya Maximets <[email protected]>; Sriram Vatala >> <[email protected]>; [email protected] >> Subject: Re: [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH >> custom stats. >> >> On 15/10/2019 14:43, Ilya Maximets wrote: >>> On 15.10.2019 15:35, Kevin Traynor wrote: >>>> With corrected email for Ilya. >>>> >>>> On 15/10/2019 14:33, Kevin Traynor wrote: >>>>> On 21/09/2019 03:40, Sriram Vatala wrote: >>>>>> From: Ilya Maximets <[email protected]> >>>>>> >>>>>> This is yet another refactoring for upcoming detailed drop stats. >>>>>> It allowes to use single function for all the software calculated >>>>>> statistics in netdev-dpdk for both vhost and ETH ports. >>>>>> >>>>>> UINT64_MAX used as a marker for non-supported statistics in a same >>>>>> way as it's done in bridge.c for common netdev stats. >>>>>> >>>>> >>>>> Hi Ilya, one comment below, >>>>> >>>>> thanks, >>>>> Kevin. >>>>> >>>>>> Cc: Sriram Vatala <[email protected]> >>>>>> Signed-off-by: Ilya Maximets <[email protected]> >>>>>> Signed-off-by: Sriram Vatala <[email protected]> >>>>>> --- >>>>>> lib/netdev-dpdk.c | 67 +++++++++++++++++++++++++++-------------------- >>>>>> 1 file changed, 39 insertions(+), 28 deletions(-) >>>>>> >>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >>>>>> bc20d6843..652b57e3b 100644 >>>>>> --- a/lib/netdev-dpdk.c >>>>>> +++ b/lib/netdev-dpdk.c >>>>>> @@ -471,6 +471,8 @@ struct netdev_rxq_dpdk { >>>>>> static void netdev_dpdk_destruct(struct netdev *netdev); >>>>>> static void netdev_dpdk_vhost_destruct(struct netdev *netdev); >>>>>> >>>>>> +static int netdev_dpdk_get_sw_custom_stats(const struct netdev *, >>>>>> + struct >>>>>> +netdev_custom_stats *); >>>>>> static void netdev_dpdk_clear_xstats(struct netdev_dpdk *dev); >>>>>> >>>>>> int netdev_dpdk_get_vid(const struct netdev_dpdk *dev); @@ >>>>>> -1171,7 +1173,7 @@ common_construct(struct netdev *netdev, dpdk_port_t >>>>>> port_no, >>>>>> dev->rte_xstats_ids = NULL; >>>>>> dev->rte_xstats_ids_size = 0; >>>>>> >>>>>> - dev->tx_retries = 0; >>>>>> + dev->tx_retries = (dev->type == DPDK_DEV_VHOST) ? 0 : >>>>>> + UINT64_MAX; >>>>>> >>>>>> return 0; >>>>>> } >>>>>> @@ -2771,7 +2773,9 @@ netdev_dpdk_get_custom_stats(const struct >>>>>> netdev *netdev, >>>>>> >>>>>> uint32_t i; >>>>>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >>>>>> - int rte_xstats_ret; >>>>>> + int rte_xstats_ret, sw_stats_size; >>>>>> + >>>>>> + netdev_dpdk_get_sw_custom_stats(netdev, custom_stats); >>>>>> >>>>>> ovs_mutex_lock(&dev->mutex); >>>>>> >>>>>> @@ -2786,12 +2790,13 @@ netdev_dpdk_get_custom_stats(const struct netdev >>>>>> *netdev, >>>>>> if (rte_xstats_ret > 0 && >>>>>> rte_xstats_ret <= dev->rte_xstats_ids_size) { >>>>>> >>>>>> - custom_stats->size = rte_xstats_ret; >>>>>> - custom_stats->counters = >>>>>> - (struct netdev_custom_counter *) >>>>>> xcalloc(rte_xstats_ret, >>>>>> - sizeof(struct netdev_custom_counter)); >>>>>> + sw_stats_size = custom_stats->size; >>>>>> + custom_stats->size += rte_xstats_ret; >>>>>> + custom_stats->counters = xrealloc(custom_stats->counters, >>>>>> + custom_stats->size * >>>>>> + sizeof >>>>>> + *custom_stats->counters); >>>>>> >>>>>> - for (i = 0; i < rte_xstats_ret; i++) { >>>>>> + for (i = sw_stats_size; i < sw_stats_size + >>>>>> + rte_xstats_ret; i++) { >>>>>> ovs_strlcpy(custom_stats->counters[i].name, >>>>>> netdev_dpdk_get_xstat_name(dev, >>>>>> >>>>>> dev->rte_xstats_ids[i]), >>>>> >>>>> I think you need to add another array index counter for >>>>> ret_xstats_ids[] and values[] as they are still using i, but i is >>>>> now starting with sw_stats_size and not 0 anymore. >>> >>> Good catch. >>> I didn't actually test this code hoping that it'll be tested along >>> with the second patch. >>> Sorry I missed this. I checked vhost port stats and missed to check dpdk >>> port stats in my testing. It's my mistake. >>> For this part we could just move the 'sw_stats_size' from the loop >>> counter to the counters[i]. Like this: >>> >>> for (i = 0; i < rte_xstats_ret; i++) { >>> ovs_strlcpy(custom_stats->counters[sw_stats_size + i].name, >>> netdev_dpdk_get_xstat_name(dev, >>> dev->rte_xstats_ids[i]), >>> NETDEV_CUSTOM_STATS_NAME_SIZE); >>> custom_stats->counters[sw_stats_size + i].value = values[i]; } >>> >> >> Yep, that would be good. With that fix, you can add >> Acked-by: Kevin Traynor <[email protected]> >> >> @Ilya Maximets with your approval, I can fix this with the suggested >> approach >> and send the updated patch. What do you say? > > It's OK. But, I think, that it might be better to wait for review comments > for patch #2 first. >
I'll review patch #2 and send comments tomorrow. (btw, it doesn't apply cleanly on master) > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
