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 F3AA3A0487 for ; Thu, 4 Jul 2019 11:10:16 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B61831B998; Thu, 4 Jul 2019 11:10:16 +0200 (CEST) Received: from mail-vk1-f194.google.com (mail-vk1-f194.google.com [209.85.221.194]) by dpdk.org (Postfix) with ESMTP id DFC311B96E for ; Thu, 4 Jul 2019 11:10:15 +0200 (CEST) Received: by mail-vk1-f194.google.com with SMTP id m17so492553vkl.2 for ; Thu, 04 Jul 2019 02:10:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Eli4//rYjrGnOiszU5osoAm8nJjZyhh6/dULVoQdfP4=; b=PTsoyIcpC8P2IvDjvvfDaCmWBjplbhgP6cWIcocfm4LPQVSXEPaXJBwDJe+lruWsVi Vvhs8nruHZZ+ZZAiCjpF9J0eqiWLP3DpCOA9mWh+KEBJTC1ViD84UrJhG+trGAL52Z0W RXMNt5L2jVGTL7EQ8KvvtoYS6HG7cklLqO0dY2ymP3mY9l11fO0/HuhbHW2j2kiasMt7 /NrGZTUzVS62ax6Kt1nMWufBgPAJ82OMjYdyL5gg3/ZZFohhdtrtDHwSRvucOjr4iHWk rWUnHSR18AEfbA+1zMIvyYc3KvBIds5LSx6X1KKtNrXrSPTqq1z+xFounq09q49uF+NT QU0Q== X-Gm-Message-State: APjAAAU2weCyktqEx6fiFNn5dpUfi73p8mnRXO0sVxKIAnFXqM32fa1l hRF9T2s3t34XzErtVO6pELPcErlQUu4xR6iGg482gQ== X-Google-Smtp-Source: APXvYqyAxJ/XU7flTRRWKt64DSBPztr9HNi3V2BYc8zyE6BmQLamGOU84WkUqFoEtweIMcHAwzEkkehyBX49/Tz6J50= X-Received: by 2002:a1f:144:: with SMTP id 65mr1877801vkb.53.1562231415256; Thu, 04 Jul 2019 02:10:15 -0700 (PDT) MIME-Version: 1.0 References: <1557354906-2500-1-git-send-email-erik.g.carrillo@intel.com> <5c7403e06efccd2c8210ce811fa16c8e56e084b0.1561478924.git.anatoly.burakov@intel.com> In-Reply-To: <5c7403e06efccd2c8210ce811fa16c8e56e084b0.1561478924.git.anatoly.burakov@intel.com> From: David Marchand Date: Thu, 4 Jul 2019 11:10:04 +0200 Message-ID: To: Anatoly Burakov Cc: dev , Robert Sanford , Erik Gabriel Carrillo Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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 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. -- David Marchand