From: Anatoly Burakov <anatoly.burakov@intel.com>
To: dev@dpdk.org
Cc: Robert Sanford <rsanford@akamai.com>,
Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Subject: [dpdk-dev] [PATCH v3 1/1] timer: fix resource leak in finalize
Date: Fri, 5 Jul 2019 18:22:44 +0100 [thread overview]
Message-ID: <c4df88d88459bb157656e18f1bdf5549ce541527.1562347316.git.anatoly.burakov@intel.com> (raw)
In-Reply-To: <cover.1562347316.git.anatoly.burakov@intel.com>
In-Reply-To: <cover.1562347316.git.anatoly.burakov@intel.com>
Currently, whenever timer library is initialized, the memory
is leaked because there is no telling when primary or secondary
processes get to use the state, and there is no way to
initialize/deinitialize timer library state without race
conditions [1] because the data itself must live in shared memory.
Add a spinlock to the shared mem config to have a way to
exclusively initialize/deinitialize the timer library without
any races, and implement the synchronization mechanism based
on this lock in the timer library.
Also, update the API doc. Note that the behavior of the API
itself did not change - the requirement to call init in every
process was simply not documented explicitly.
Fixes: c0749f7096c7 ("timer: allow management in shared memory")
[1] See the following email thread:
https://mails.dpdk.org/archives/dev/2019-May/131498.html
Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
---
lib/librte_eal/common/eal_common_mcfg.c | 14 +++++++
lib/librte_eal/common/eal_memcfg.h | 2 +
.../common/include/rte_eal_memconfig.h | 22 ++++++++++
lib/librte_eal/rte_eal_version.map | 2 +
lib/librte_timer/rte_timer.c | 41 +++++++++++++------
lib/librte_timer/rte_timer.h | 5 ++-
6 files changed, 71 insertions(+), 15 deletions(-)
diff --git a/lib/librte_eal/common/eal_common_mcfg.c b/lib/librte_eal/common/eal_common_mcfg.c
index 1825d9083..066549432 100644
--- a/lib/librte_eal/common/eal_common_mcfg.c
+++ b/lib/librte_eal/common/eal_common_mcfg.c
@@ -147,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void)
struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
rte_rwlock_write_unlock(&mcfg->mplock);
}
+
+void
+rte_mcfg_timer_lock(void)
+{
+ struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+ rte_spinlock_lock(&mcfg->tlock);
+}
+
+void
+rte_mcfg_timer_unlock(void)
+{
+ struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
+ rte_spinlock_unlock(&mcfg->tlock);
+}
diff --git a/lib/librte_eal/common/eal_memcfg.h b/lib/librte_eal/common/eal_memcfg.h
index 030f903ad..359beb216 100644
--- a/lib/librte_eal/common/eal_memcfg.h
+++ b/lib/librte_eal/common/eal_memcfg.h
@@ -11,6 +11,7 @@
#include <rte_memory.h>
#include <rte_memzone.h>
#include <rte_pause.h>
+#include <rte_spinlock.h>
#include <rte_rwlock.h>
#include <rte_tailq.h>
@@ -36,6 +37,7 @@ struct rte_mem_config {
rte_rwlock_t mlock; /**< used by memzones for thread safety. */
rte_rwlock_t qlock; /**< used by tailqs for thread safety. */
rte_rwlock_t mplock; /**< used by mempool library for thread safety. */
+ rte_spinlock_t tlock; /**< used by timer library for thread safety. */
rte_rwlock_t memory_hotplug_lock;
/**< Indicates whether memory hotplug request is in progress. */
diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
index dc61a6fed..34b0e44a0 100644
--- a/lib/librte_eal/common/include/rte_eal_memconfig.h
+++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
@@ -5,6 +5,8 @@
#ifndef _RTE_EAL_MEMCONFIG_H_
#define _RTE_EAL_MEMCONFIG_H_
+#include <rte_compat.h>
+
/**
* @file
*
@@ -87,6 +89,26 @@ rte_mcfg_mempool_write_lock(void);
void
rte_mcfg_mempool_write_unlock(void);
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Lock the internal EAL Timer Library lock for exclusive access.
+ */
+__rte_experimental
+void
+rte_mcfg_timer_lock(void);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ *
+ * Unlock the internal EAL Timer Library lock for exclusive access.
+ */
+__rte_experimental
+void
+rte_mcfg_timer_unlock(void);
+
#ifdef __cplusplus
}
#endif
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index cc4ef04a0..d0ee67dbf 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -405,4 +405,6 @@ EXPERIMENTAL {
# added in 19.08
rte_lcore_cpuset;
rte_lcore_to_cpu_id;
+ rte_mcfg_timer_lock;
+ rte_mcfg_timer_unlock;
};
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index eaeafd74f..71dffd23b 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -13,6 +13,7 @@
#include <rte_atomic.h>
#include <rte_common.h>
#include <rte_cycles.h>
+#include <rte_eal_memconfig.h>
#include <rte_per_lcore.h>
#include <rte_memory.h>
#include <rte_launch.h>
@@ -61,6 +62,8 @@ struct rte_timer_data {
};
#define RTE_MAX_DATA_ELS 64
+static const struct rte_memzone *rte_timer_data_mz;
+static int *volatile rte_timer_mz_refcnt;
static struct rte_timer_data *rte_timer_data_arr;
static const uint32_t default_data_id;
static uint32_t rte_timer_subsystem_initialized;
@@ -157,28 +160,30 @@ rte_timer_subsystem_init_v1905(void)
int i, lcore_id;
static const char *mz_name = "rte_timer_mz";
const size_t data_arr_size =
- RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+ RTE_MAX_DATA_ELS * sizeof(*rte_timer_data_arr);
+ const size_t mem_size = data_arr_size + sizeof(*rte_timer_mz_refcnt);
bool do_full_init = true;
if (rte_timer_subsystem_initialized)
return -EALREADY;
-reserve:
- rte_errno = 0;
- mz = rte_memzone_reserve_aligned(mz_name, data_arr_size, SOCKET_ID_ANY,
- 0, RTE_CACHE_LINE_SIZE);
+ rte_mcfg_timer_lock();
+
+ mz = rte_memzone_lookup(mz_name);
if (mz == NULL) {
- if (rte_errno == EEXIST) {
- mz = rte_memzone_lookup(mz_name);
- if (mz == NULL)
- goto reserve;
-
- do_full_init = false;
- } else
+ mz = rte_memzone_reserve_aligned(mz_name, mem_size,
+ SOCKET_ID_ANY, 0, RTE_CACHE_LINE_SIZE);
+ if (mz == NULL) {
+ rte_mcfg_timer_unlock();
return -ENOMEM;
- }
+ }
+ do_full_init = true;
+ } else
+ do_full_init = false;
+ rte_timer_data_mz = mz;
rte_timer_data_arr = mz->addr;
+ rte_timer_mz_refcnt = (void *)((char *)mz->addr + data_arr_size);
if (do_full_init) {
for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
@@ -195,6 +200,9 @@ rte_timer_subsystem_init_v1905(void)
}
rte_timer_data_arr[default_data_id].internal_flags |= FL_ALLOCATED;
+ (*rte_timer_mz_refcnt)++;
+
+ rte_mcfg_timer_unlock();
rte_timer_subsystem_initialized = 1;
@@ -210,6 +218,13 @@ rte_timer_subsystem_finalize(void)
if (!rte_timer_subsystem_initialized)
return;
+ rte_mcfg_timer_lock();
+
+ if (--(*rte_timer_mz_refcnt) == 0)
+ rte_memzone_free(rte_timer_data_mz);
+
+ rte_mcfg_timer_unlock();
+
rte_timer_subsystem_initialized = 0;
}
diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
index 1b0d5f6a5..05d287d8f 100644
--- a/lib/librte_timer/rte_timer.h
+++ b/lib/librte_timer/rte_timer.h
@@ -172,10 +172,11 @@ int rte_timer_data_dealloc(uint32_t id);
* Initializes internal variables (list, locks and so on) for the RTE
* timer library.
*
+ * @note
+ * This function must be called in every process before using the library.
+ *
* @return
* - 0: Success
- * - -EEXIST: Returned in secondary process when primary process has not
- * yet initialized the timer subsystem
* - -ENOMEM: Unable to allocate memory needed to initialize timer
* subsystem
*/
--
2.17.1
next prev parent reply other threads:[~2019-07-05 17:22 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-01 19:00 [dpdk-dev] [PATCH] " Erik Gabriel Carrillo
2019-05-01 19:00 ` Erik Gabriel Carrillo
2019-05-02 9:18 ` Burakov, Anatoly
2019-05-02 9:18 ` Burakov, Anatoly
2019-05-02 12:19 ` Carrillo, Erik G
2019-05-02 12:19 ` Carrillo, Erik G
2019-05-02 13:03 ` Burakov, Anatoly
2019-05-02 13:03 ` Burakov, Anatoly
2019-05-02 13:48 ` Carrillo, Erik G
2019-05-02 13:48 ` Carrillo, Erik G
2019-05-03 22:54 ` [dpdk-dev] [PATCH v2] " Erik Gabriel Carrillo
2019-05-03 22:54 ` Erik Gabriel Carrillo
2019-05-07 11:03 ` Burakov, Anatoly
2019-05-07 11:03 ` Burakov, Anatoly
2019-05-07 22:04 ` Carrillo, Erik G
2019-05-07 22:04 ` Carrillo, Erik G
2019-05-08 8:49 ` Burakov, Anatoly
2019-05-08 8:49 ` Burakov, Anatoly
2019-05-08 23:01 ` Carrillo, Erik G
2019-05-08 23:01 ` Carrillo, Erik G
2019-05-09 7:44 ` Thomas Monjalon
2019-05-09 7:44 ` Thomas Monjalon
2019-05-08 22:35 ` [dpdk-dev] [PATCH v3] " Erik Gabriel Carrillo
2019-05-08 22:35 ` Erik Gabriel Carrillo
2019-05-09 8:29 ` Burakov, Anatoly
2019-05-09 8:29 ` Burakov, Anatoly
2019-06-05 9:33 ` Thomas Monjalon
2019-06-05 9:47 ` Burakov, Anatoly
2019-06-25 16:11 ` [dpdk-dev] [PATCH 0/2] Fix timer resource leak Anatoly Burakov
2019-07-05 13:20 ` [dpdk-dev] [PATCH v2 0/1] " Anatoly Burakov
2019-07-05 17:22 ` [dpdk-dev] [PATCH v3 " Anatoly Burakov
2019-07-05 17:22 ` Anatoly Burakov [this message]
2019-07-05 22:06 ` [dpdk-dev] [PATCH v3 1/1] timer: fix resource leak in finalize Thomas Monjalon
2019-07-05 13:20 ` [dpdk-dev] [PATCH v2 " Anatoly Burakov
2019-06-25 16:11 ` [dpdk-dev] [PATCH 1/2] eal: add internal locks for timer lib into EAL Anatoly Burakov
2019-06-27 18:41 ` Carrillo, Erik G
2019-07-04 9:09 ` David Marchand
2019-07-04 10:44 ` Burakov, Anatoly
2019-06-25 16:11 ` [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize Anatoly Burakov
2019-06-27 18:48 ` Carrillo, Erik G
2019-07-04 9:10 ` David Marchand
2019-07-04 10:45 ` Burakov, Anatoly
2019-07-04 10:50 ` David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c4df88d88459bb157656e18f1bdf5549ce541527.1562347316.git.anatoly.burakov@intel.com \
--to=anatoly.burakov@intel.com \
--cc=dev@dpdk.org \
--cc=erik.g.carrillo@intel.com \
--cc=rsanford@akamai.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).