On Thu, Jul 4, 2019 at 12:45 PM Burakov, Anatoly <anatoly.bura...@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.bura...@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.carri...@intel.com>
> >> Signed-off-by: Anatoly Burakov <anatoly.bura...@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
