From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id A2C7AA0487 for ; Fri, 5 Jul 2019 15:20:44 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3EFFB1BECA; Fri, 5 Jul 2019 15:20:40 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id A37411B9B5 for ; Fri, 5 Jul 2019 15:20:37 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jul 2019 06:20:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,455,1557212400"; d="scan'208";a="363178543" Received: from silpixa00399498.ir.intel.com (HELO silpixa00399498.ger.corp.intel.com) ([10.237.223.125]) by fmsmga005.fm.intel.com with ESMTP; 05 Jul 2019 06:20:36 -0700 From: Anatoly Burakov To: dev@dpdk.org Cc: Bruce Richardson , Robert Sanford , Erik Gabriel Carrillo Date: Fri, 5 Jul 2019 14:20:34 +0100 Message-Id: <640592892955e359e64d8c0263dd99182f96f4b1.1562332788.git.anatoly.burakov@intel.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: References: In-Reply-To: References: Subject: [dpdk-dev] [PATCH v2 1/1] 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 [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 Signed-off-by: Anatoly Burakov Acked-by: Erik Gabriel Carrillo Reviewed-by: David Marchand --- lib/librte_eal/common/eal_common_mcfg.c | 29 +++++++++++++ lib/librte_eal/common/eal_memcfg.h | 8 ++++ .../common/include/rte_eal_memconfig.h | 22 ++++++++++ lib/librte_eal/freebsd/eal/eal.c | 5 ++- lib/librte_eal/linux/eal/eal.c | 5 ++- lib/librte_eal/rte_eal_version.map | 2 + lib/librte_timer/rte_timer.c | 41 +++++++++++++------ lib/librte_timer/rte_timer.h | 5 ++- 8 files changed, 100 insertions(+), 17 deletions(-) diff --git a/lib/librte_eal/common/eal_common_mcfg.c b/lib/librte_eal/common/eal_common_mcfg.c index fe8d2b726..066549432 100644 --- a/lib/librte_eal/common/eal_common_mcfg.c +++ b/lib/librte_eal/common/eal_common_mcfg.c @@ -4,6 +4,7 @@ #include #include +#include #include "eal_internal_cfg.h" #include "eal_memcfg.h" @@ -31,6 +32,18 @@ eal_mcfg_wait_complete(void) rte_pause(); } +int +eal_mcfg_check_version(void) +{ + struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config; + + /* check if version from memconfig matches compiled in macro */ + if (mcfg->version != RTE_VERSION) + return -1; + + return 0; +} + void eal_mcfg_update_internal(void) { @@ -47,6 +60,8 @@ eal_mcfg_update_from_internal(void) mcfg->legacy_mem = internal_config.legacy_mem; mcfg->single_file_segments = internal_config.single_file_segments; + /* record current DPDK version */ + mcfg->version = RTE_VERSION; } void @@ -132,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 573233896..359beb216 100644 --- a/lib/librte_eal/common/eal_memcfg.h +++ b/lib/librte_eal/common/eal_memcfg.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -19,6 +20,8 @@ */ struct rte_mem_config { volatile uint32_t magic; /**< Magic number - sanity check. */ + uint32_t version; + /**< Prevent secondary processes using different DPDK versions. */ /* memory topology */ uint32_t nchannel; /**< Number of channels (0 if unknown). */ @@ -34,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. */ @@ -81,6 +85,10 @@ eal_mcfg_update_from_internal(void); void eal_mcfg_wait_complete(void); +/* check if DPDK version of current process matches one stored in the config */ +int +eal_mcfg_check_version(void); + /* set mem config as complete */ void eal_mcfg_complete(void); 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 + /** * @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/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c index ec1650c43..1d3d7e80f 100644 --- a/lib/librte_eal/freebsd/eal/eal.c +++ b/lib/librte_eal/freebsd/eal/eal.c @@ -385,6 +385,10 @@ rte_config_init(void) if (rte_eal_config_attach() < 0) return -1; eal_mcfg_wait_complete(); + if (eal_mcfg_check_version() < 0) { + RTE_LOG(ERR, EAL, "Primary and secondary process DPDK version mismatch\n"); + return -1; + } if (rte_eal_config_reattach() < 0) return -1; eal_mcfg_update_internal(); @@ -395,7 +399,6 @@ rte_config_init(void) rte_config.process_type); return -1; } - return 0; } diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c index 445d72f0c..a5e0d1e75 100644 --- a/lib/librte_eal/linux/eal/eal.c +++ b/lib/librte_eal/linux/eal/eal.c @@ -489,6 +489,10 @@ rte_config_init(void) if (rte_eal_config_attach() < 0) return -1; eal_mcfg_wait_complete(); + if (eal_mcfg_check_version() < 0) { + RTE_LOG(ERR, EAL, "Primary and secondary process DPDK version mismatch\n"); + return -1; + } if (rte_eal_config_reattach() < 0) return -1; eal_mcfg_update_internal(); @@ -499,7 +503,6 @@ rte_config_init(void) rte_config.process_type); return -1; } - return 0; } 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 #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 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