[dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize
Burakov, Anatoly
anatoly.burakov at intel.com
Thu Jul 4 12:45:26 CEST 2019
On 04-Jul-19 10:10 AM, David Marchand wrote:
> On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov <anatoly.burakov at 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 at intel.com>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov at 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
More information about the dev
mailing list