Re: [dpdk-dev] [PATCH 2/2] timer: fix resource leak in finalize

[David Marchand <david.marchand@redhat.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.


-- 
David Marchand