[v3,1/2] metrics: new API to deinitialise metrics library

Message ID 1562755934-13358-1-git-send-email-hkalra@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v3,1/2] metrics: new API to deinitialise metrics library |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation fail apply issues

Commit Message

Harman Kalra July 10, 2019, 10:52 a.m. UTC
  Once the library usage is over, it must be deinitialized which
will free the shared memory reserved during initialization.

Observed an issue while running 'metrics_autotest' continuously
without quiting. For the first run 'metrics_autotest' passes
all test cases but second run onwards first test case fails
because metrics library is already initialized during first run.

Cc: stable@dpdk.org

Signed-off-by: Harman Kalra <hkalra@marvell.com>
---
v2:
* Adding stable@dpdk.org into cc as this patch falls between bug fix
and new feature.
v3:
* Fixed check-git-log.sh and make doc-guides-html issues.

 doc/guides/prog_guide/metrics_lib.rst      | 14 ++++++++++++++
 lib/librte_metrics/rte_metrics.c           | 20 ++++++++++++++++++++
 lib/librte_metrics/rte_metrics.h           | 18 ++++++++++++++++++
 lib/librte_metrics/rte_metrics_version.map |  6 ++++++
 4 files changed, 58 insertions(+)
  

Comments

Thomas Monjalon July 10, 2019, 10:08 p.m. UTC | #1
10/07/2019 12:52, Harman Kalra:
> Once the library usage is over, it must be deinitialized which
> will free the shared memory reserved during initialization.
> 
> Observed an issue while running 'metrics_autotest' continuously
> without quiting. For the first run 'metrics_autotest' passes
> all test cases but second run onwards first test case fails
> because metrics library is already initialized during first run.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>
> ---
> v2:
> * Adding stable@dpdk.org into cc as this patch falls between bug fix
> and new feature.
> v3:
> * Fixed check-git-log.sh and make doc-guides-html issues.

Why don't you take our comments into account?

Reminder 1:
"
I was waiting for an ack on this patch,
and realized that there was one already on v1.
When sending v2, you should have reported the Ack.
"

Reminder 2:
"
Tested-by : Reshma Pattan <reshma.pattan@intel.com>
Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Please keep these tags for next versions.
"

Reminder 3:
"
I would vote for not backporting this new API.
"

In case it is not clear, this comment means you should not Cc stable@dpdk.org
  
Harman Kalra July 11, 2019, 8:12 a.m. UTC | #2
On Thu, Jul 11, 2019 at 12:08:09AM +0200, Thomas Monjalon wrote:
> 10/07/2019 12:52, Harman Kalra:
> > Once the library usage is over, it must be deinitialized which
> > will free the shared memory reserved during initialization.
> > 
> > Observed an issue while running 'metrics_autotest' continuously
> > without quiting. For the first run 'metrics_autotest' passes
> > all test cases but second run onwards first test case fails
> > because metrics library is already initialized during first run.
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Harman Kalra <hkalra@marvell.com>
> > ---
> > v2:
> > * Adding stable@dpdk.org into cc as this patch falls between bug fix
> > and new feature.
> > v3:
> > * Fixed check-git-log.sh and make doc-guides-html issues.
> 
> Why don't you take our comments into account?

Sorry, I did not know that I have to copy ACKs and other tags.
Will surely take care from next time onwards.

> 
> Reminder 1:
> "
> I was waiting for an ack on this patch,
> and realized that there was one already on v1.
> When sending v2, you should have reported the Ack.
> "
> 
> Reminder 2:
> "
> Tested-by : Reshma Pattan <reshma.pattan@intel.com>
> Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
> Acked-by: Reshma Pattan <reshma.pattan@intel.com>
> 
> Please keep these tags for next versions.
> "

Will include these in version 4.

> 
> Reminder 3:
> "
> I would vote for not backporting this new API.
> "
> 
> In case it is not clear, this comment means you should not Cc stable@dpdk.org

I added CC because of the following comment from Remy:
"
On Mon, Feb 25, 2019 at 12:21:21PM +0000, Remy Horton wrote:
> External Email
> 
> ----------------------------------------------------------------------
> This patchset is in that grey area between new feature and bugfix so
> it
> might need to be CC'd to stable@dpdk.org
"

Shall I remove it now?

> 
> 
>
  
Thomas Monjalon July 11, 2019, 8:34 a.m. UTC | #3
11/07/2019 10:12, Harman Kalra:
> On Thu, Jul 11, 2019 at 12:08:09AM +0200, Thomas Monjalon wrote:
> > "
> > I would vote for not backporting this new API.
> > "
> > 
> > In case it is not clear, this comment means you should not Cc stable@dpdk.org
> 
> I added CC because of the following comment from Remy:
> "
> On Mon, Feb 25, 2019 at 12:21:21PM +0000, Remy Horton wrote:
> > This patchset is in that grey area between new feature and bugfix so
> > it might need to be CC'd to stable@dpdk.org
> "
> 
> Shall I remove it now?

Yes, this is my opinion and we did not get any other opinion.
I don't think the issue is critical enough to backport a new API.
  

Patch

diff --git a/doc/guides/prog_guide/metrics_lib.rst b/doc/guides/prog_guide/metrics_lib.rst
index 89bc7d68f..eca855d60 100644
--- a/doc/guides/prog_guide/metrics_lib.rst
+++ b/doc/guides/prog_guide/metrics_lib.rst
@@ -154,6 +154,20 @@  print out all metrics for a given port:
     }
 
 
+Deinitialising the library
+--------------------------
+
+Once the library usage is done, it must be deinitialized by calling
+``rte_metrics_deinit()`` which will free the shared memory reserved
+during initialization.
+
+.. code-block:: c
+
+    err = rte_metrics_deinit(void);
+
+If the return value is negative, it means deinitialization failed.
+This function **must** be called from a primary process.
+
 Bit-rate statistics library
 ---------------------------
 
diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
index 99a96b651..df5e32c59 100644
--- a/lib/librte_metrics/rte_metrics.c
+++ b/lib/librte_metrics/rte_metrics.c
@@ -76,6 +76,26 @@  rte_metrics_init(int socket_id)
 	rte_spinlock_init(&stats->lock);
 }
 
+int
+rte_metrics_deinit(void)
+{
+	struct rte_metrics_data_s *stats;
+	const struct rte_memzone *memzone;
+
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -EINVAL;
+
+	memzone = rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME);
+	if (memzone == NULL)
+		return -EIO;
+
+	stats = memzone->addr;
+	memset(stats, 0, sizeof(struct rte_metrics_data_s));
+
+	return rte_memzone_free(memzone);
+
+}
+
 int
 rte_metrics_reg_name(const char *name)
 {
diff --git a/lib/librte_metrics/rte_metrics.h b/lib/librte_metrics/rte_metrics.h
index 67a60fadd..77bffe08e 100644
--- a/lib/librte_metrics/rte_metrics.h
+++ b/lib/librte_metrics/rte_metrics.h
@@ -24,6 +24,7 @@ 
 #define _RTE_METRICS_H_
 
 #include <stdint.h>
+#include <rte_compat.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -80,6 +81,23 @@  struct rte_metric_value {
  */
 void rte_metrics_init(int socket_id);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Deinitialize metric module. This function must be called from
+ * a primary process after all the metrics usage is over, to
+ *  release the shared memory.
+ *
+ * @return
+ *  -EINVAL - invalid parameter.
+ *  -EIO: Error, unable to access metrics shared memory
+ *    (rte_metrics_init() not called)
+ *  0 - success
+ */
+__rte_experimental
+int rte_metrics_deinit(void);
+
 /**
  * Register a metric, making it available as a reporting parameter.
  *
diff --git a/lib/librte_metrics/rte_metrics_version.map b/lib/librte_metrics/rte_metrics_version.map
index 4c5234cd1..6ac99a44a 100644
--- a/lib/librte_metrics/rte_metrics_version.map
+++ b/lib/librte_metrics/rte_metrics_version.map
@@ -11,3 +11,9 @@  DPDK_17.05 {
 
 	local: *;
 };
+
+EXPERIMENTAL {
+	global:
+
+	rte_metrics_deinit;
+};