| Message ID | 0743551eba840752223a6447ab4dcaf0731add39.1561478924.git.anatoly.burakov@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | Fix timer resource leak | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | fail | Compilation issues |
Hi Anatoly, > -----Original Message----- > From: Burakov, Anatoly > Sent: Tuesday, June 25, 2019 11:12 AM > To: dev@dpdk.org > Cc: Carrillo, Erik G <erik.g.carrillo@intel.com> > Subject: [PATCH 1/2] eal: add internal locks for timer lib into EAL > > Currently, timer library has a memory leak because there is no way to > concurrently initialize/deinitialize shared memory because of race conditions > [1]. > > Add a spinlock to the shared mem config to have a way to exclusively > initialize/deinitialize the timer library without any races. > > [1] See the following email thread: > https://mails.dpdk.org/archives/dev/2019-May/131498.html > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> <... snipped ...> > --- a/lib/librte_eal/common/include/rte_eal_memconfig.h > +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h > @@ -109,6 +109,24 @@ rte_mcfg_mempool_write_lock(void); > void > rte_mcfg_mempool_write_unlock(void); > > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * Lock the internal EAL Timer Library lock for exclusive access. > + */ > +void __rte_experimental Depending on the decision made for the following thread, the "__rte_experimental" tag location may move: https://mails.dpdk.org/archives/dev/2019-June/136050.html > +rte_mcfg_timer_lock(void); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * Unlock the internal EAL Timer Library lock for exclusive access. > + */ > +void __rte_experimental > +rte_mcfg_timer_unlock(void); > + > #ifdef __cplusplus > } > #endif <... snipped ...> Other than that: 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, timer library has a memory leak because there is no > way to concurrently initialize/deinitialize shared memory because > of race conditions [1]. > > Add a spinlock to the shared mem config to have a way to > exclusively initialize/deinitialize the timer library without > any races. > > [1] See the following email thread: > https://mails.dpdk.org/archives/dev/2019-May/131498.html > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> > --- > lib/librte_eal/common/eal_common_mcfg.c | 14 ++++++++++++++ > lib/librte_eal/common/eal_memcfg.h | 2 ++ > .../common/include/rte_eal_memconfig.h | 18 ++++++++++++++++++ > lib/librte_eal/rte_eal_version.map | 2 ++ > 4 files changed, 36 insertions(+) > > diff --git a/lib/librte_eal/common/eal_common_mcfg.c > b/lib/librte_eal/common/eal_common_mcfg.c > index 1825d9083..066549432 100644 > --- a/lib/librte_eal/common/eal_common_mcfg.c > +++ b/lib/librte_eal/common/eal_common_mcfg.c > @@ -147,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void) > struct rte_mem_config *mcfg = > rte_eal_get_configuration()->mem_config; > rte_rwlock_write_unlock(&mcfg->mplock); > } > + > +void > +rte_mcfg_timer_lock(void) > +{ > + struct rte_mem_config *mcfg = > rte_eal_get_configuration()->mem_config; > + rte_spinlock_lock(&mcfg->tlock); > +} > + > +void > +rte_mcfg_timer_unlock(void) > +{ > + struct rte_mem_config *mcfg = > rte_eal_get_configuration()->mem_config; > + rte_spinlock_unlock(&mcfg->tlock); > +} > diff --git a/lib/librte_eal/common/eal_memcfg.h > b/lib/librte_eal/common/eal_memcfg.h > index e1aae32df..00370cece 100644 > --- a/lib/librte_eal/common/eal_memcfg.h > +++ b/lib/librte_eal/common/eal_memcfg.h > @@ -11,6 +11,7 @@ > #include <rte_memory.h> > #include <rte_memzone.h> > #include <rte_pause.h> > +#include <rte_spinlock.h> > #include <rte_rwlock.h> > #include <rte_tailq.h> > > @@ -38,6 +39,7 @@ struct rte_mem_config { > rte_rwlock_t mlock; /**< only used by memzone LIB for > thread-safe. */ > rte_rwlock_t qlock; /**< used for tailq operation for thread > safe. */ > rte_rwlock_t mplock; /**< only used by mempool LIB for > thread-safe. */ > + rte_spinlock_t tlock; /**< needed for timer lib thread safety. */ > > rte_rwlock_t memory_hotplug_lock; > /**< indicates whether memory hotplug request is in progress. */ > diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h > b/lib/librte_eal/common/include/rte_eal_memconfig.h > index 1b615c892..05a32e12a 100644 > --- a/lib/librte_eal/common/include/rte_eal_memconfig.h > +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h > @@ -109,6 +109,24 @@ rte_mcfg_mempool_write_lock(void); > void > rte_mcfg_mempool_write_unlock(void); > > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * Lock the internal EAL Timer Library lock for exclusive access. > + */ > +void __rte_experimental > As mentionned by Erik, rte_experimental void +rte_mcfg_timer_lock(void); > + > +/** > + * @warning > + * @b EXPERIMENTAL: this API may change without prior notice > + * > + * Unlock the internal EAL Timer Library lock for exclusive access. > + */ > +void __rte_experimental > rte_experimental void > +rte_mcfg_timer_unlock(void); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_eal/rte_eal_version.map > b/lib/librte_eal/rte_eal_version.map > index c28951f65..bc08fc4df 100644 > --- a/lib/librte_eal/rte_eal_version.map > +++ b/lib/librte_eal/rte_eal_version.map > @@ -367,6 +367,8 @@ EXPERIMENTAL { > rte_malloc_heap_memory_detach; > rte_malloc_heap_memory_remove; > rte_malloc_heap_socket_is_external; > + rte_mcfg_timer_lock; > + rte_mcfg_timer_unlock; > rte_mem_alloc_validator_register; > rte_mem_alloc_validator_unregister; > rte_mem_check_dma_mask; > -- > 2.17.1 > Can you put this at the end of the EXPERIMENTAL block under the 19.08 comment?
On 04-Jul-19 10:09 AM, David Marchand wrote: > On Tue, Jun 25, 2019 at 6:12 PM Anatoly Burakov <anatoly.burakov@intel.com> > wrote: > >> Currently, timer library has a memory leak because there is no >> way to concurrently initialize/deinitialize shared memory because >> of race conditions [1]. >> >> Add a spinlock to the shared mem config to have a way to >> exclusively initialize/deinitialize the timer library without >> any races. >> >> [1] See the following email thread: >> https://mails.dpdk.org/archives/dev/2019-May/131498.html >> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> >> --- >> lib/librte_eal/common/eal_common_mcfg.c | 14 ++++++++++++++ >> lib/librte_eal/common/eal_memcfg.h | 2 ++ >> .../common/include/rte_eal_memconfig.h | 18 ++++++++++++++++++ >> lib/librte_eal/rte_eal_version.map | 2 ++ >> 4 files changed, 36 insertions(+) >> >> diff --git a/lib/librte_eal/common/eal_common_mcfg.c >> b/lib/librte_eal/common/eal_common_mcfg.c >> index 1825d9083..066549432 100644 >> --- a/lib/librte_eal/common/eal_common_mcfg.c >> +++ b/lib/librte_eal/common/eal_common_mcfg.c >> @@ -147,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void) >> struct rte_mem_config *mcfg = >> rte_eal_get_configuration()->mem_config; >> rte_rwlock_write_unlock(&mcfg->mplock); >> } >> + >> +void >> +rte_mcfg_timer_lock(void) >> +{ >> + struct rte_mem_config *mcfg = >> rte_eal_get_configuration()->mem_config; >> + rte_spinlock_lock(&mcfg->tlock); >> +} >> + >> +void >> +rte_mcfg_timer_unlock(void) >> +{ >> + struct rte_mem_config *mcfg = >> rte_eal_get_configuration()->mem_config; >> + rte_spinlock_unlock(&mcfg->tlock); >> +} >> diff --git a/lib/librte_eal/common/eal_memcfg.h >> b/lib/librte_eal/common/eal_memcfg.h >> index e1aae32df..00370cece 100644 >> --- a/lib/librte_eal/common/eal_memcfg.h >> +++ b/lib/librte_eal/common/eal_memcfg.h >> @@ -11,6 +11,7 @@ >> #include <rte_memory.h> >> #include <rte_memzone.h> >> #include <rte_pause.h> >> +#include <rte_spinlock.h> >> #include <rte_rwlock.h> >> #include <rte_tailq.h> >> >> @@ -38,6 +39,7 @@ struct rte_mem_config { >> rte_rwlock_t mlock; /**< only used by memzone LIB for >> thread-safe. */ >> rte_rwlock_t qlock; /**< used for tailq operation for thread >> safe. */ >> rte_rwlock_t mplock; /**< only used by mempool LIB for >> thread-safe. */ >> + rte_spinlock_t tlock; /**< needed for timer lib thread safety. */ >> > >> rte_rwlock_t memory_hotplug_lock; >> /**< indicates whether memory hotplug request is in progress. */ >> diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h >> b/lib/librte_eal/common/include/rte_eal_memconfig.h >> index 1b615c892..05a32e12a 100644 >> --- a/lib/librte_eal/common/include/rte_eal_memconfig.h >> +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h >> @@ -109,6 +109,24 @@ rte_mcfg_mempool_write_lock(void); >> void >> rte_mcfg_mempool_write_unlock(void); >> >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice >> + * >> + * Lock the internal EAL Timer Library lock for exclusive access. >> + */ >> +void __rte_experimental >> > > As mentionned by Erik, > > rte_experimental > void > > +rte_mcfg_timer_lock(void); >> + >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice >> + * >> + * Unlock the internal EAL Timer Library lock for exclusive access. >> + */ >> +void __rte_experimental >> > > rte_experimental > void > > > >> +rte_mcfg_timer_unlock(void); >> + >> #ifdef __cplusplus >> } >> #endif >> diff --git a/lib/librte_eal/rte_eal_version.map >> b/lib/librte_eal/rte_eal_version.map >> index c28951f65..bc08fc4df 100644 >> --- a/lib/librte_eal/rte_eal_version.map >> +++ b/lib/librte_eal/rte_eal_version.map >> @@ -367,6 +367,8 @@ EXPERIMENTAL { >> rte_malloc_heap_memory_detach; >> rte_malloc_heap_memory_remove; >> rte_malloc_heap_socket_is_external; >> + rte_mcfg_timer_lock; >> + rte_mcfg_timer_unlock; >> rte_mem_alloc_validator_register; >> rte_mem_alloc_validator_unregister; >> rte_mem_check_dma_mask; >> -- >> 2.17.1 >> > > Can you put this at the end of the EXPERIMENTAL block under the 19.08 > comment? > OK, will do both for v2.
diff --git a/lib/librte_eal/common/eal_common_mcfg.c b/lib/librte_eal/common/eal_common_mcfg.c index 1825d9083..066549432 100644 --- a/lib/librte_eal/common/eal_common_mcfg.c +++ b/lib/librte_eal/common/eal_common_mcfg.c @@ -147,3 +147,17 @@ rte_mcfg_mempool_write_unlock(void) struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config; rte_rwlock_write_unlock(&mcfg->mplock); } + +void +rte_mcfg_timer_lock(void) +{ + struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config; + rte_spinlock_lock(&mcfg->tlock); +} + +void +rte_mcfg_timer_unlock(void) +{ + struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config; + rte_spinlock_unlock(&mcfg->tlock); +} diff --git a/lib/librte_eal/common/eal_memcfg.h b/lib/librte_eal/common/eal_memcfg.h index e1aae32df..00370cece 100644 --- a/lib/librte_eal/common/eal_memcfg.h +++ b/lib/librte_eal/common/eal_memcfg.h @@ -11,6 +11,7 @@ #include <rte_memory.h> #include <rte_memzone.h> #include <rte_pause.h> +#include <rte_spinlock.h> #include <rte_rwlock.h> #include <rte_tailq.h> @@ -38,6 +39,7 @@ struct rte_mem_config { rte_rwlock_t mlock; /**< only used by memzone LIB for thread-safe. */ rte_rwlock_t qlock; /**< used for tailq operation for thread safe. */ rte_rwlock_t mplock; /**< only used by mempool LIB for thread-safe. */ + rte_spinlock_t tlock; /**< needed for timer lib thread safety. */ rte_rwlock_t memory_hotplug_lock; /**< indicates whether memory hotplug request is in progress. */ diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h b/lib/librte_eal/common/include/rte_eal_memconfig.h index 1b615c892..05a32e12a 100644 --- a/lib/librte_eal/common/include/rte_eal_memconfig.h +++ b/lib/librte_eal/common/include/rte_eal_memconfig.h @@ -109,6 +109,24 @@ rte_mcfg_mempool_write_lock(void); void rte_mcfg_mempool_write_unlock(void); +/** + * @warning + * @b EXPERIMENTAL: this API may change without prior notice + * + * Lock the internal EAL Timer Library lock for exclusive access. + */ +void __rte_experimental +rte_mcfg_timer_lock(void); + +/** + * @warning + * @b EXPERIMENTAL: this API may change without prior notice + * + * Unlock the internal EAL Timer Library lock for exclusive access. + */ +void __rte_experimental +rte_mcfg_timer_unlock(void); + #ifdef __cplusplus } #endif diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map index c28951f65..bc08fc4df 100644 --- a/lib/librte_eal/rte_eal_version.map +++ b/lib/librte_eal/rte_eal_version.map @@ -367,6 +367,8 @@ EXPERIMENTAL { rte_malloc_heap_memory_detach; rte_malloc_heap_memory_remove; rte_malloc_heap_socket_is_external; + rte_mcfg_timer_lock; + rte_mcfg_timer_unlock; rte_mem_alloc_validator_register; rte_mem_alloc_validator_unregister; rte_mem_check_dma_mask;
Currently, timer library has a memory leak because there is no way to concurrently initialize/deinitialize shared memory because of race conditions [1]. Add a spinlock to the shared mem config to have a way to exclusively initialize/deinitialize the timer library without any races. [1] See the following email thread: https://mails.dpdk.org/archives/dev/2019-May/131498.html Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> --- lib/librte_eal/common/eal_common_mcfg.c | 14 ++++++++++++++ lib/librte_eal/common/eal_memcfg.h | 2 ++ .../common/include/rte_eal_memconfig.h | 18 ++++++++++++++++++ lib/librte_eal/rte_eal_version.map | 2 ++ 4 files changed, 36 insertions(+)