-----Original Message----- From: Kevin Traynor <ktray...@redhat.com> Sent: 15 October 2019 19:21 To: Ilya Maximets <i.maxim...@ovn.org>; Sriram Vatala <srira...@altencalsoftlabs.com>; ovs-dev@openvswitch.org 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 <i.maxim...@samsung.com> >>>> >>>> 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 <srira...@altencalsoftlabs.com> >>>> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com> >>>> Signed-off-by: Sriram Vatala <srira...@altencalsoftlabs.com> >>>> --- >>>> 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 <ktray...@redhat.com> @Ilya Maximets with your approval, I can fix this with the suggested approach and send the updated patch. What do you say? > >>> >>>> @@ -2801,8 +2806,6 @@ netdev_dpdk_get_custom_stats(const struct netdev >>>> *netdev, >>>> } else { >>>> VLOG_WARN("Cannot get XSTATS values for port: >>>> "DPDK_PORT_ID_FMT, >>>> dev->port_id); >>>> - custom_stats->counters = NULL; >>>> - custom_stats->size = 0; >>>> /* Let's clear statistics cache, so it will be >>>> * reconfigured */ >>>> netdev_dpdk_clear_xstats(dev); @@ -2817,39 +2820,47 >>>> @@ 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) >>>> +netdev_dpdk_get_sw_custom_stats(const struct netdev *netdev, >>>> + struct netdev_custom_stats >>>> +*custom_stats) >>>> { >>>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >>>> - int i; >>>> + int i, n; >>>> >>>> -#define VHOST_CSTATS \ >>>> - VHOST_CSTAT(tx_retries) >>>> +#define SW_CSTATS \ >>>> + SW_CSTAT(tx_retries) >>>> >>>> -#define VHOST_CSTAT(NAME) + 1 >>>> - custom_stats->size = VHOST_CSTATS; >>>> -#undef VHOST_CSTAT >>>> +#define SW_CSTAT(NAME) + 1 >>>> + custom_stats->size = SW_CSTATS; #undef SW_CSTAT >>>> custom_stats->counters = xcalloc(custom_stats->size, >>>> sizeof *custom_stats->counters); >>>> - i = 0; >>>> -#define VHOST_CSTAT(NAME) \ >>>> - ovs_strlcpy(custom_stats->counters[i++].name, #NAME, \ >>>> - NETDEV_CUSTOM_STATS_NAME_SIZE); >>>> - VHOST_CSTATS; >>>> -#undef VHOST_CSTAT >>>> >>>> ovs_mutex_lock(&dev->mutex); >>>> >>>> rte_spinlock_lock(&dev->stats_lock); >>>> i = 0; >>>> -#define VHOST_CSTAT(NAME) \ >>>> +#define SW_CSTAT(NAME) \ >>>> custom_stats->counters[i++].value = dev->NAME; >>>> - VHOST_CSTATS; >>>> -#undef VHOST_CSTAT >>>> + SW_CSTATS; >>>> +#undef SW_CSTAT >>>> rte_spinlock_unlock(&dev->stats_lock); >>>> >>>> ovs_mutex_unlock(&dev->mutex); >>>> >>>> + i = 0; >>>> + n = 0; >>>> +#define SW_CSTAT(NAME) \ >>>> + if (custom_stats->counters[i].value != UINT64_MAX) >>>> { \ >>>> + ovs_strlcpy(custom_stats->counters[n].name, #NAME, >>>> \ >>>> + NETDEV_CUSTOM_STATS_NAME_SIZE); >>>> \ >>>> + custom_stats->counters[n].value = >>>> custom_stats->counters[i].value; \ >>>> + n++; >>>> \ >>>> + } >>>> \ >>>> + i++; >>>> + SW_CSTATS; >>>> +#undef SW_CSTAT >>>> + >>>> + custom_stats->size = n; >>>> return 0; >>>> } >>>> >>>> @@ -4433,7 +4444,7 @@ static const struct netdev_class dpdk_vhost_class = >>>> { >>>> .send = netdev_dpdk_vhost_send, >>>> .get_carrier = netdev_dpdk_vhost_get_carrier, >>>> .get_stats = netdev_dpdk_vhost_get_stats, >>>> - .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, >>>> + .get_custom_stats = netdev_dpdk_get_sw_custom_stats, >>>> .get_status = netdev_dpdk_vhost_user_get_status, >>>> .reconfigure = netdev_dpdk_vhost_reconfigure, >>>> .rxq_recv = netdev_dpdk_vhost_rxq_recv, @@ -4449,7 +4460,7 @@ >>>> static const struct netdev_class dpdk_vhost_client_class = { >>>> .send = netdev_dpdk_vhost_send, >>>> .get_carrier = netdev_dpdk_vhost_get_carrier, >>>> .get_stats = netdev_dpdk_vhost_get_stats, >>>> - .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, >>>> + .get_custom_stats = netdev_dpdk_get_sw_custom_stats, >>>> .get_status = netdev_dpdk_vhost_user_get_status, >>>> .reconfigure = netdev_dpdk_vhost_client_reconfigure, >>>> .rxq_recv = netdev_dpdk_vhost_rxq_recv, >>>> >>> >>
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev