DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
To: David Marchand <david.marchand@redhat.com>
Cc: dev <dev@dpdk.org>, Robert Sanford <rsanford@akamai.com>,
	Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize
Date: Thu, 4 Jul 2019 11:45:26 +0100	[thread overview]
Message-ID: <d6645e97-d577-0169-8ecf-cb2671ffaf07@intel.com> (raw)
In-Reply-To: <CAJFAV8yooQOON8qas9WuCPTExO+0jMk-KVQn8AstoJhKsMQGaw@mail.gmail.com>

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.

-- 
Thanks,
Anatoly

  reply	other threads:[~2019-07-04 10:45 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-01 19:00 [dpdk-dev] [PATCH] " Erik Gabriel Carrillo
2019-05-01 19:00 ` Erik Gabriel Carrillo
2019-05-02  9:18 ` Burakov, Anatoly
2019-05-02  9:18   ` Burakov, Anatoly
2019-05-02 12:19   ` Carrillo, Erik G
2019-05-02 12:19     ` Carrillo, Erik G
2019-05-02 13:03     ` Burakov, Anatoly
2019-05-02 13:03       ` Burakov, Anatoly
2019-05-02 13:48       ` Carrillo, Erik G
2019-05-02 13:48         ` Carrillo, Erik G
2019-05-03 22:54 ` [dpdk-dev] [PATCH v2] " Erik Gabriel Carrillo
2019-05-03 22:54   ` Erik Gabriel Carrillo
2019-05-07 11:03   ` Burakov, Anatoly
2019-05-07 11:03     ` Burakov, Anatoly
2019-05-07 22:04     ` Carrillo, Erik G
2019-05-07 22:04       ` Carrillo, Erik G
2019-05-08  8:49       ` Burakov, Anatoly
2019-05-08  8:49         ` Burakov, Anatoly
2019-05-08 23:01         ` Carrillo, Erik G
2019-05-08 23:01           ` Carrillo, Erik G
2019-05-09  7:44           ` Thomas Monjalon
2019-05-09  7:44             ` Thomas Monjalon
2019-05-08 22:35   ` [dpdk-dev] [PATCH v3] " Erik Gabriel Carrillo
2019-05-08 22:35     ` Erik Gabriel Carrillo
2019-05-09  8:29     ` Burakov, Anatoly
2019-05-09  8:29       ` Burakov, Anatoly
2019-06-05  9:33       ` Thomas Monjalon
2019-06-05  9:47         ` Burakov, Anatoly
2019-06-25 16:11     ` [dpdk-dev] [PATCH 0/2] Fix timer resource leak Anatoly Burakov
2019-07-05 13:20       ` [dpdk-dev] [PATCH v2 0/1] " Anatoly Burakov
2019-07-05 17:22         ` [dpdk-dev] [PATCH v3 " Anatoly Burakov
2019-07-05 17:22         ` [dpdk-dev] [PATCH v3 1/1] timer: fix resource leak in finalize Anatoly Burakov
2019-07-05 22:06           ` Thomas Monjalon
2019-07-05 13:20       ` [dpdk-dev] [PATCH v2 " Anatoly Burakov
2019-06-25 16:11     ` [dpdk-dev] [PATCH 1/2] eal: add internal locks for timer lib into EAL Anatoly Burakov
2019-06-27 18:41       ` Carrillo, Erik G
2019-07-04  9:09       ` David Marchand
2019-07-04 10:44         ` Burakov, Anatoly
2019-06-25 16:11     ` [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize Anatoly Burakov
2019-06-27 18:48       ` Carrillo, Erik G
2019-07-04  9:10       ` David Marchand
2019-07-04 10:45         ` Burakov, Anatoly [this message]
2019-07-04 10:50           ` David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d6645e97-d577-0169-8ecf-cb2671ffaf07@intel.com \
    --to=anatoly.burakov@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=erik.g.carrillo@intel.com \
    --cc=rsanford@akamai.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).