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

[David Marchand <david.marchand@redhat.com>]

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