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 0204CA0487 for ; Thu, 4 Jul 2019 12:45:31 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CDB8C1BDFB; Thu, 4 Jul 2019 12:45:30 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 574D41BDE9 for ; Thu, 4 Jul 2019 12:45:29 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Jul 2019 03:45:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,450,1557212400"; d="scan'208";a="172398509" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.126]) ([10.237.220.126]) by FMSMGA003.fm.intel.com with ESMTP; 04 Jul 2019 03:45:27 -0700 To: David Marchand Cc: dev , Robert Sanford , Erik Gabriel Carrillo References: <1557354906-2500-1-git-send-email-erik.g.carrillo@intel.com> <5c7403e06efccd2c8210ce811fa16c8e56e084b0.1561478924.git.anatoly.burakov@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Thu, 4 Jul 2019 11:45:26 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [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" On 04-Jul-19 10:10 AM, David Marchand wrote: > On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov > wrote: > >> 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 >> > > Can be squashed with the first patch. > This is not just search-and-replace - this is also a bugfix. I can squash the search-and-replace part with the first patch, but this commit will have to stay IMO. -- Thanks, Anatoly