[2/2] timer: fix resource leak in finalize
Checks
Commit Message
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(-)
Comments
> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 25, 2019 11:12 AM
> To: dev@dpdk.org
> Cc: Robert Sanford <rsanford@akamai.com>; Carrillo, Erik G
> <erik.g.carrillo@intel.com>
> Subject: [PATCH 2/2] timer: fix resource leak in finalize
>
> 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>
Acked-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
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.
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.
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.
@@ -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;
}
@@ -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
*/