Re: [ovs-dev] [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.
On 15/10/2019 16:42, Ilya Maximets wrote: > On 15.10.2019 17:32, Sriram Vatala wrote: >> -Original Message- >> From: Kevin Traynor >> Sent: 15 October 2019 19:21 >> To: Ilya Maximets ; Sriram Vatala >> ; 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 >> >> 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 >> Signed-off-by: Ilya Maximets >> Signed-off-by: Sriram Vatala >> --- >>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(>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 >> >> @Ilya Maximets with your approval, I can fix this with the suggested
Re: [ovs-dev] [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.
On 15.10.2019 17:32, Sriram Vatala wrote: -Original Message- From: Kevin Traynor Sent: 15 October 2019 19:21 To: Ilya Maximets ; Sriram Vatala ; 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 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 Signed-off-by: Ilya Maximets Signed-off-by: Sriram Vatala --- 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(>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 @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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.
-Original Message- From: Kevin Traynor Sent: 15 October 2019 19:21 To: Ilya Maximets ; Sriram Vatala ; 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 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 Signed-off-by: Ilya Maximets Signed-off-by: Sriram Vatala --- 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(>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 @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;
Re: [ovs-dev] [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 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 Signed-off-by: Ilya Maximets Signed-off-by: Sriram Vatala --- 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(>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 > >>> @@ -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,
Re: [ovs-dev] [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.
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 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 Signed-off-by: Ilya Maximets Signed-off-by: Sriram Vatala --- 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(>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,
Re: [ovs-dev] [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.
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 >> >> 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 >> Signed-off-by: Ilya Maximets >> Signed-off-by: Sriram Vatala >> --- >> 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(>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 >> >>
Re: [ovs-dev] [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.
On 21/09/2019 03:40, Sriram Vatala wrote: > From: Ilya Maximets > > 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 > Signed-off-by: Ilya Maximets > Signed-off-by: Sriram Vatala > --- > 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(>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(>mutex); > > rte_spinlock_lock(>stats_lock); > i = 0; > -#define VHOST_CSTAT(NAME) \ > +#define SW_CSTAT(NAME) \ > custom_stats->counters[i++].value = dev->NAME; > -VHOST_CSTATS; > -#undef
[ovs-dev] [PATCH v9 1/2] netdev-dpdk: Reuse vhost function for dpdk ETH custom stats.
From: Ilya Maximets 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. Cc: Sriram Vatala Signed-off-by: Ilya Maximets Signed-off-by: Sriram Vatala --- 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(>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(>mutex); rte_spinlock_lock(>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(>stats_lock); ovs_mutex_unlock(>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++;