| Message ID | 20190921024008.11355-1-sriram.v@altencalsoftlabs.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v9,1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats. | expand |
On 21/09/2019 03:40, Sriram Vatala wrote: > From: Ilya Maximets <i.maximets@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 <sriram.v@altencalsoftlabs.com> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: Sriram Vatala <sriram.v@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. > @@ -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, >
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.maximets@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 <sriram.v@altencalsoftlabs.com> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> Signed-off-by: Sriram Vatala <sriram.v@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. > >> @@ -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, >> >
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.maximets@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 <sriram.v@altencalsoftlabs.com> >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>> Signed-off-by: Sriram Vatala <sriram.v@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. 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]; } >> >>> @@ -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, >>> >> >
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.maximets@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 <sriram.v@altencalsoftlabs.com> >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>> Signed-off-by: Sriram Vatala <sriram.v@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. > > 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 <ktraynor@redhat.com> > >>> >>>> @@ -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, >>>> >>> >>
-----Original Message----- From: Kevin Traynor <ktraynor@redhat.com> Sent: 15 October 2019 19:21 To: Ilya Maximets <i.maximets@ovn.org>; Sriram Vatala <sriram.v@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.maximets@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 <sriram.v@altencalsoftlabs.com> >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>> Signed-off-by: Sriram Vatala <sriram.v@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 <ktraynor@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, >>>> >>> >>
On 15.10.2019 17:32, Sriram Vatala wrote: > -----Original Message----- > From: Kevin Traynor <ktraynor@redhat.com> > Sent: 15 October 2019 19:21 > To: Ilya Maximets <i.maximets@ovn.org>; Sriram Vatala > <sriram.v@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.maximets@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 <sriram.v@altencalsoftlabs.com> >>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>>> Signed-off-by: Sriram Vatala <sriram.v@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 <ktraynor@redhat.com> > > @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. Best regards, Ilya Maximets.
On 15/10/2019 16:42, Ilya Maximets wrote: > On 15.10.2019 17:32, Sriram Vatala wrote: >> -----Original Message----- >> From: Kevin Traynor <ktraynor@redhat.com> >> Sent: 15 October 2019 19:21 >> To: Ilya Maximets <i.maximets@ovn.org>; Sriram Vatala >> <sriram.v@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.maximets@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 <sriram.v@altencalsoftlabs.com> >>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>>>> Signed-off-by: Sriram Vatala <sriram.v@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 <ktraynor@redhat.com> >> >> @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. >
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]), @@ -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,