From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id B6B1AA0487
for <public@inbox.dpdk.org>; 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 <dev@dpdk.org>; Thu, 4 Jul 2019 12:50:36 +0200 (CEST)
Received: by mail-ua1-f66.google.com with SMTP id a97so990373uaa.9
for <dev@dpdk.org>; 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>
<cover.1561478924.git.anatoly.burakov@intel.com>
<5c7403e06efccd2c8210ce811fa16c8e56e084b0.1561478924.git.anatoly.burakov@intel.com>
<CAJFAV8yooQOON8qas9WuCPTExO+0jMk-KVQn8AstoJhKsMQGaw@mail.gmail.com>
<d6645e97-d577-0169-8ecf-cb2671ffaf07@intel.com>
In-Reply-To: <d6645e97-d577-0169-8ecf-cb2671ffaf07@intel.com>
From: David Marchand <david.marchand@redhat.com>
Date: Thu, 4 Jul 2019 12:50:25 +0200
Message-ID: <CAJFAV8zzpVYQHCWX7uiR8xMbe8aPAtdVin66F8aNLvdBkfzpSw@mail.gmail.com>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Cc: dev <dev@dpdk.org>, Robert Sanford <rsanford@akamai.com>,
Erik Gabriel Carrillo <erik.g.carrillo@intel.com>,
Thomas Monjalon <thomas@monjalon.net>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On Thu, Jul 4, 2019 at 12:45 PM Burakov, Anatoly <anatoly.burakov@intel.com>
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 <erik.g.carrillo@intel.com>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >> ---
> >> 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 <rte_atomic.h>
> >> #include <rte_common.h>
> >> #include <rte_cycles.h>
> >> +#include <rte_eal_memconfig.h>
> >> #include <rte_per_lcore.h>
> >> #include <rte_memory.h>
> >> #include <rte_launch.h>
> >> @@ -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