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 B6B1AA0487 for ; Thu, 4 Jul 2019 12:50:38 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7EB951BDED; Thu, 4 Jul 2019 12:50:38 +0200 (CEST) Received: from mail-ua1-f66.google.com (mail-ua1-f66.google.com [209.85.222.66]) by dpdk.org (Postfix) with ESMTP id ADCE41BDE9 for ; Thu, 4 Jul 2019 12:50:36 +0200 (CEST) Received: by mail-ua1-f66.google.com with SMTP id a97so990373uaa.9 for ; Thu, 04 Jul 2019 03:50:36 -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=wzAq8uyCGejrgeZW0G/upi5yJwAPilKSl7nXQgT8zsc=; b=qJPeeWqsEweEBu+t3/podZ9IPgyk6ceP+fLinyO1p0IhNGSJ7ii9kzIkQZuDmUpWsg f8W027XOsAXXhCdMk3hf38ajk/fh5EVhM3BGUFozYpupVLKODDECIC1nA0cR8dpt32Oy J+wTDWllPDVMSuXv9ynT9lL8eWkELRN3Wu+vnz248y+AA4J0Pt0Hu3Q7FmpRbw8g62kK CpQrwvcgM0453y3arpE+K3jkFlVqDr7rlUHQa/f79eai8SBS998lgrPf7hCvekRV8l4J yKmPGdqIprOg6FO2K4x99sF3B9K3nCrLw/MO33zpqQDNwS1IyMYMGHG0QuKGdv4Y3RT/ +q+A== X-Gm-Message-State: APjAAAWRkIfvRQ7rbTgS5fKE7e2N915tpa1T/GeVagMhL+lUNnvJE4sC FZVzwiXr9S5kTws/Bs/HfJlLwMItta1nMag3djcq5g== X-Google-Smtp-Source: APXvYqwLk8O7bMSMSllH/6fY2d8fjayNmVSiylRWaN1EyqEI33sKj7/gzPK+tMU2w98n1Id8KPCpYAkT4lqGR+yS7gM= X-Received: by 2002:ab0:b99:: with SMTP id c25mr21223521uak.53.1562237436047; Thu, 04 Jul 2019 03:50:36 -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: From: David Marchand Date: Thu, 4 Jul 2019 12:50:25 +0200 Message-ID: To: "Burakov, Anatoly" Cc: dev , Robert Sanford , Erik Gabriel Carrillo , Thomas Monjalon 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 Thu, Jul 4, 2019 at 12:45 PM Burakov, Anatoly wrote: > On 04-Jul-19 10:10 AM, David Marchand wrote: > > On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov < > anatoly.burakov@intel.com> > > 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. > Yes this is not search and replace, but the first patch is there for the bugfix. There are no other uses of the newly introduced API, so the whole is a single change to me. Apart from that, I am ok with the changes, you can add my review tag. -- David Marchand