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

[Thomas Monjalon <thomas@monjalon.net>]

09/05/2019 10:29, Burakov, Anatoly:
> On 08-May-19 11:35 PM, Erik Gabriel Carrillo wrote:
> > By using a lock added to the rte_mem_config (which lives in shared
> > memory), we can synchronize multiple processes in init/finalize and
> > safely free allocations made during init.
> > 
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > ---
> > changes in v3:
> >   - The previous version had race condition.  This version fixes the race
> >     by adding a lock in shared memory outside of the DPDK heap area
> >     that can be used to safely free the memzone reserved in the subsystem
> >     init call. (Anatoly)
> > 
> >     This patch depends on http://patches.dpdk.org/patch/53333/.
> >   
> > changes in v2:
> >   - Handle scenario where primary process exits before secondaries such
> >     that memzone is not freed early (Anatoly)
> > 
> >   lib/librte_eal/common/include/rte_eal_memconfig.h |  3 +++
> >   lib/librte_timer/rte_timer.c                      | 23 ++++++++++++++++++++++-
> >   2 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h
> > index 84aabe3..8cbc09c 100644
> > --- a/lib/librte_eal/common/include/rte_eal_memconfig.h
> > +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h
> > @@ -64,6 +64,9 @@ struct rte_mem_config {
> >   	rte_rwlock_t memory_hotplug_lock;
> >   	/**< indicates whether memory hotplug request is in progress. */
> >   
> > +	rte_spinlock_t timer_subsystem_lock;
> > +	/**< indicates whether timer subsystem init/finalize is in progress. */
> > +
> 
> I believe there's an initialize function somewhere which will initialize 
> all of these locks. This lock should be in there as well.
> 
> Other than that, i'm OK with this change, however i feel like /just/ 
> adding this would be a missed opportunity, because next time we want to 
> add something here it will be an ABI break again.
> 
> We could perhaps use this opportunity to leave some padding for future 
> data. I'm not sure how would that look like, just an idea floating in my 
> head :)

Any update or other opinion?