From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 3C658A046B for ; Tue, 25 Jun 2019 18:12:08 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A94DA1BB29; Tue, 25 Jun 2019 18:11:52 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id AB7511B9FD for ; Tue, 25 Jun 2019 18:11:46 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jun 2019 09:11:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,416,1557212400"; d="scan'208";a="245120811" Received: from silpixa00399498.ir.intel.com (HELO silpixa00399498.ger.corp.intel.com) ([10.237.223.125]) by orsmga001.jf.intel.com with ESMTP; 25 Jun 2019 09:11:45 -0700 From: Anatoly Burakov To: dev@dpdk.org Cc: Robert Sanford , Erik Gabriel Carrillo Date: Tue, 25 Jun 2019 17:11:42 +0100 Message-Id: <5c7403e06efccd2c8210ce811fa16c8e56e084b0.1561478924.git.anatoly.burakov@intel.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: In-Reply-To: References: <1557354906-2500-1-git-send-email-erik.g.carrillo@intel.com> Subject: [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 because the data itself must live in shared memory. However, there is now a timer library lock in the shared memory config, which can be used to synchronize access to the timer library shared memory. Use it to initialize/deinitialize timer library shared data in a safe way. There is still a way to leak the memory (by killing one of the processes), but we can't do anything about that. 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. Signed-off-by: Erik Gabriel Carrillo Signed-off-by: Anatoly Burakov --- lib/librte_timer/rte_timer.c | 41 ++++++++++++++++++++++++------------ lib/librte_timer/rte_timer.h | 5 +++-- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c index dd7953922..08517c120 100644 --- a/lib/librte_timer/rte_timer.c +++ b/lib/librte_timer/rte_timer.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -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 2196934b2..a7af10176 100644 --- a/lib/librte_timer/rte_timer.h +++ b/lib/librte_timer/rte_timer.h @@ -170,10 +170,11 @@ int __rte_experimental 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