[dpdk-dev] [PATCH 1/2] lib/metrics: fix to reset the init flag

[dpdk-dev] [PATCH 1/2] lib/metrics: fix to reset the init flag

[Hemant Agrawal]

metrics_initialized shall be reset in deinit function
This is currently causing issue in running
metrics_autotest mulutiple times

Fixes: 07c1b6925b65 ("telemetry: invert dependency on metrics library")

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 lib/librte_metrics/rte_metrics.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
index e07670219..f570cf226 100644
--- a/lib/librte_metrics/rte_metrics.c
+++ b/lib/librte_metrics/rte_metrics.c
@@ -96,6 +96,8 @@ rte_metrics_deinit(void)
 	stats = memzone->addr;
 	memset(stats, 0, sizeof(struct rte_metrics_data_s));
 
+	metrics_initialized = 0;
+
 	return rte_memzone_free(memzone);
 
 }
-- 
2.17.1

[dpdk-dev] [PATCH 2/2] test: support cleanup in bitrate and latency test

[Hemant Agrawal]

both bitratestats_autotest latency test initializes
the metrics library. It should be cleaned during exit.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 app/test/test_bitratestats.c | 14 ++++++++++++++
 app/test/test_latencystats.c |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/app/test/test_bitratestats.c b/app/test/test_bitratestats.c
index 3a7d9c037..39d7f734d 100644
--- a/app/test/test_bitratestats.c
+++ b/app/test/test_bitratestats.c
@@ -32,6 +32,18 @@ test_stats_bitrate_create(void)
 	return TEST_SUCCESS;
 }
 
+/* To test free the resources from bitrate_reg test */
+static int
+test_stats_bitrate_free(void)
+{
+	int ret = 0;
+
+	ret = rte_metrics_deinit();
+	TEST_ASSERT(ret >= 0, "Test Failed: rte_metrics_deinit failed");
+
+	return TEST_SUCCESS;
+}
+
 /* To test bit rate registration */
 static int
 test_stats_bitrate_reg(void)
@@ -214,6 +226,8 @@ unit_test_suite bitratestats_testsuite  = {
 		 */
 		TEST_CASE_ST(test_bit_packet_forward, NULL,
 				test_stats_bitrate_calc),
+		/* TEST CASE 9: Test to do the cleanup w.r.t create */
+		TEST_CASE(test_stats_bitrate_free),
 		TEST_CASES_END()
 	}
 };
diff --git a/app/test/test_latencystats.c b/app/test/test_latencystats.c
index 968e0bc47..427339904 100644
--- a/app/test/test_latencystats.c
+++ b/app/test/test_latencystats.c
@@ -60,6 +60,9 @@ static int test_latency_uninit(void)
 	ret = rte_latencystats_uninit();
 	TEST_ASSERT(ret >= 0, "Test Failed: rte_latencystats_uninit failed");
 
+	ret = rte_metrics_deinit();
+	TEST_ASSERT(ret >= 0, "Test Failed: rte_metrics_deinit failed");
+
 	return TEST_SUCCESS;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/2] lib/metrics: fix to reset the init flag

[David Marchand]

On Wed, May 13, 2020 at 12:39 PM Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>
> metrics_initialized shall be reset in deinit function
> This is currently causing issue in running
> metrics_autotest mulutiple times
>
> Fixes: 07c1b6925b65 ("telemetry: invert dependency on metrics library")
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
>  lib/librte_metrics/rte_metrics.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
> index e07670219..f570cf226 100644
> --- a/lib/librte_metrics/rte_metrics.c
> +++ b/lib/librte_metrics/rte_metrics.c
> @@ -96,6 +96,8 @@ rte_metrics_deinit(void)
>         stats = memzone->addr;
>         memset(stats, 0, sizeof(struct rte_metrics_data_s));
>
> +       metrics_initialized = 0;
> +
>         return rte_memzone_free(memzone);

Should this flag be reset only if rte_memzone_free succeeds?


-- 
David Marchand

Re: [dpdk-dev] [PATCH 1/2] lib/metrics: fix to reset the init flag

[Hemant Agrawal]

> On Wed, May 13, 2020 at 12:39 PM Hemant Agrawal
> <hemant.agrawal@nxp.com> wrote:
> >
> > metrics_initialized shall be reset in deinit function This is
> > currently causing issue in running metrics_autotest mulutiple times
> >
> > Fixes: 07c1b6925b65 ("telemetry: invert dependency on metrics
> > library")
> >
> > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> > ---
> >  lib/librte_metrics/rte_metrics.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/librte_metrics/rte_metrics.c
> > b/lib/librte_metrics/rte_metrics.c
> > index e07670219..f570cf226 100644
> > --- a/lib/librte_metrics/rte_metrics.c
> > +++ b/lib/librte_metrics/rte_metrics.c
> > @@ -96,6 +96,8 @@ rte_metrics_deinit(void)
> >         stats = memzone->addr;
> >         memset(stats, 0, sizeof(struct rte_metrics_data_s));
> >
> > +       metrics_initialized = 0;
> > +
> >         return rte_memzone_free(memzone);
> 
> Should this flag be reset only if rte_memzone_free succeeds?
> 
[Hemant]  I thought about it but I did not do it for following reasons.
1. If the memzone is not freed,  It will not be initialized next time due to following check in init routine.
 	memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
	if (memzone != NULL)
		return;
2. Most applications have very weak error handling in de-init/cleanup parts. 

Having said that. It can be changed to do it only on the success of rte_memzone_free
> 
> --
> David Marchand

[dpdk-dev] [PATCH v2 1/2] lib/metrics: fix to reset the init flag

[Hemant Agrawal]

metrics_initialized shall be reset in deinit function
This is currently causing issue in running
metrics_autotest mulutiple times

Fixes: 07c1b6925b65 ("telemetry: invert dependency on metrics library")

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 lib/librte_metrics/rte_metrics.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
index e07670219f..dba6409c27 100644
--- a/lib/librte_metrics/rte_metrics.c
+++ b/lib/librte_metrics/rte_metrics.c
@@ -85,6 +85,7 @@ rte_metrics_deinit(void)
 {
 	struct rte_metrics_data_s *stats;
 	const struct rte_memzone *memzone;
+	int ret = 0;
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return -EINVAL;
@@ -96,8 +97,10 @@ rte_metrics_deinit(void)
 	stats = memzone->addr;
 	memset(stats, 0, sizeof(struct rte_metrics_data_s));
 
-	return rte_memzone_free(memzone);
-
+	ret  = rte_memzone_free(memzone);
+	if (ret == 0)
+		metrics_initialized = 0;
+	return ret;
 }
 
 int
-- 
2.17.1

[dpdk-dev] [PATCH v2 2/2] test: support cleanup in bitrate and latency test

[Hemant Agrawal]

both bitratestats_autotest latency test initializes
the metrics library. It should be cleaned during exit.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 app/test/test_bitratestats.c | 14 ++++++++++++++
 app/test/test_latencystats.c |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/app/test/test_bitratestats.c b/app/test/test_bitratestats.c
index 3a7d9c037a..39d7f734d4 100644
--- a/app/test/test_bitratestats.c
+++ b/app/test/test_bitratestats.c
@@ -32,6 +32,18 @@ test_stats_bitrate_create(void)
 	return TEST_SUCCESS;
 }
 
+/* To test free the resources from bitrate_reg test */
+static int
+test_stats_bitrate_free(void)
+{
+	int ret = 0;
+
+	ret = rte_metrics_deinit();
+	TEST_ASSERT(ret >= 0, "Test Failed: rte_metrics_deinit failed");
+
+	return TEST_SUCCESS;
+}
+
 /* To test bit rate registration */
 static int
 test_stats_bitrate_reg(void)
@@ -214,6 +226,8 @@ unit_test_suite bitratestats_testsuite  = {
 		 */
 		TEST_CASE_ST(test_bit_packet_forward, NULL,
 				test_stats_bitrate_calc),
+		/* TEST CASE 9: Test to do the cleanup w.r.t create */
+		TEST_CASE(test_stats_bitrate_free),
 		TEST_CASES_END()
 	}
 };
diff --git a/app/test/test_latencystats.c b/app/test/test_latencystats.c
index 968e0bc470..427339904d 100644
--- a/app/test/test_latencystats.c
+++ b/app/test/test_latencystats.c
@@ -60,6 +60,9 @@ static int test_latency_uninit(void)
 	ret = rte_latencystats_uninit();
 	TEST_ASSERT(ret >= 0, "Test Failed: rte_latencystats_uninit failed");
 
+	ret = rte_metrics_deinit();
+	TEST_ASSERT(ret >= 0, "Test Failed: rte_metrics_deinit failed");
+
 	return TEST_SUCCESS;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2 2/2] test: support cleanup in bitrate and latency test

[Stephen Hemminger]

On Tue, 19 May 2020 16:22:58 +0530
Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

> +/* To test free the resources from bitrate_reg test */
> +static int
> +test_stats_bitrate_free(void)
> +{
> +	int ret = 0;
> +
> +	ret = rte_metrics_deinit();

Coverity will complain about extra initialization like this

Re: [dpdk-dev] [PATCH v2 1/2] lib/metrics: fix to reset the init flag

[David Marchand]

On Tue, May 19, 2020 at 12:55 PM Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>
> metrics_initialized shall be reset in deinit function
> This is currently causing issue in running
> metrics_autotest mulutiple times
>
> Fixes: 07c1b6925b65 ("telemetry: invert dependency on metrics library")
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

For the series,
Acked-by: David Marchand <david.marchand@redhat.com>


Applied, thanks.

-- 
David Marchand

Re: [dpdk-dev] [PATCH v2 1/2] lib/metrics: fix to reset the init flag

[Stephen Hemminger]

On Tue, 19 May 2020 16:22:57 +0530
Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

> diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
> index e07670219f..dba6409c27 100644
> --- a/lib/librte_metrics/rte_metrics.c
> +++ b/lib/librte_metrics/rte_metrics.c
> @@ -85,6 +85,7 @@ rte_metrics_deinit(void)
>  {
>  	struct rte_metrics_data_s *stats;
>  	const struct rte_memzone *memzone;
> +	int ret = 0;
>  

Why do gratuitous initialization? It blocks compiler from finding buggy
code.