| Message ID | 1424723769-8762-7-git-send-email-robking@cisco.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/23/2015 11:36 PM, Robbie King wrote: > +int odp_pool_term_global(void) > +{ > + int i; > + pool_entry_t *pool; > + int ret = 0; > + int rc = 0; > + > + for (i = 0; i < ODP_CONFIG_POOLS; i++) { > + pool = get_pool_entry(i); > + > + POOL_LOCK(&pool->s.lock); > + if (pool->s.pool_shm != ODP_SHM_INVALID) { > + ODP_ERR("Not destroyed pool: %s\n", pool->s.name); > + rc = -1; > + } > + POOL_UNLOCK(&pool->s.lock); > + } what this for does? you should do: pool->s.name[0] = 0; pool->s.flags.has_name = 1; unmap(pool->s.pool_base_addr); Also I would like to zero everything that odp_pool_create() created. Including atomic counters. Thanks, Maxim. > + > + ret = odp_shm_free(odp_shm_lookup(SHM_DEFAULT_NAME)); > + if (ret < 0) { > + ODP_ERR("shm free failed for %s", SHM_DEFAULT_NAME); > + rc = -1; > + } > + > + return rc; > +} > +
Maxim, there's no need to do all that since odp_pool_create() handles a full re-init of the pool structures as part of (re)allocating it for another create operation. Actually, having vestigial info about past usage can be valuable for debugging core dumps. A pool is unallocated if pool->s.pool_shm == ODP_SHM_INVALID and that's the key variable that determines whether the rest of the items in the struct are active. Bill On Tue, Feb 24, 2015 at 7:19 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 02/23/2015 11:36 PM, Robbie King wrote: > >> +int odp_pool_term_global(void) >> +{ >> + int i; >> + pool_entry_t *pool; >> + int ret = 0; >> + int rc = 0; >> + >> + for (i = 0; i < ODP_CONFIG_POOLS; i++) { >> + pool = get_pool_entry(i); >> + >> + POOL_LOCK(&pool->s.lock); >> + if (pool->s.pool_shm != ODP_SHM_INVALID) { >> + ODP_ERR("Not destroyed pool: %s\n", pool->s.name >> ); >> + rc = -1; >> + } >> + POOL_UNLOCK(&pool->s.lock); >> + } >> > what this for does? > you should do: > pool->s.name[0] = 0; > pool->s.flags.has_name = 1; > unmap(pool->s.pool_base_addr); > > Also I would like to zero everything that odp_pool_create() created. > Including atomic counters. > > Thanks, > Maxim. > > + >> + ret = odp_shm_free(odp_shm_lookup(SHM_DEFAULT_NAME)); >> + if (ret < 0) { >> + ODP_ERR("shm free failed for %s", SHM_DEFAULT_NAME); >> + rc = -1; >> + } >> + >> + return rc; >> +} >> + >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 02/25/2015 01:21 AM, Bill Fischofer wrote: > Maxim, there's no need to do all that since odp_pool_create() handles > a full re-init of the pool structures as part of (re)allocating it for > another create operation. > > Actually, having vestigial info about past usage can be valuable for > debugging core dumps. A pool is unallocated if pool->s.pool_shm == > ODP_SHM_INVALID and that's the key variable that determines whether > the rest of the items in the struct are active. > > Bill > But at least we need unmap(pool->s.pool_base_addr), to prevent memory leaks right? Maxim. > On Tue, Feb 24, 2015 at 7:19 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 02/23/2015 11:36 PM, Robbie King wrote: > > +int odp_pool_term_global(void) > +{ > + int i; > + pool_entry_t *pool; > + int ret = 0; > + int rc = 0; > + > + for (i = 0; i < ODP_CONFIG_POOLS; i++) { > + pool = get_pool_entry(i); > + > + POOL_LOCK(&pool->s.lock); > + if (pool->s.pool_shm != ODP_SHM_INVALID) { > + ODP_ERR("Not destroyed pool: %s\n", > pool->s.name <http://s.name>); > + rc = -1; > + } > + POOL_UNLOCK(&pool->s.lock); > + } > > what this for does? > you should do: > pool->s.name <http://s.name>[0] = 0; > pool->s.flags.has_name = 1; > unmap(pool->s.pool_base_addr); > > Also I would like to zero everything that odp_pool_create() > created. Including atomic counters. > > Thanks, > Maxim. > > + > + ret = odp_shm_free(odp_shm_lookup(SHM_DEFAULT_NAME)); > + if (ret < 0) { > + ODP_ERR("shm free failed for %s", > SHM_DEFAULT_NAME); > + rc = -1; > + } > + > + return rc; > +} > + > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > >
odp_pool_create() issues an odp_shm_reserve() call. odp_pool_destroy() issues a corresponding odp_shm_free() call. What else is needed? Presumably any needed map/unmap calls are internal to the odp_shm_xxx( routines, no? On Wed, Feb 25, 2015 at 3:27 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 02/25/2015 01:21 AM, Bill Fischofer wrote: > >> Maxim, there's no need to do all that since odp_pool_create() handles a >> full re-init of the pool structures as part of (re)allocating it for >> another create operation. >> >> Actually, having vestigial info about past usage can be valuable for >> debugging core dumps. A pool is unallocated if pool->s.pool_shm == >> ODP_SHM_INVALID and that's the key variable that determines whether the >> rest of the items in the struct are active. >> >> Bill >> >> But at least we need unmap(pool->s.pool_base_addr), to prevent memory > leaks right? > > Maxim. > > On Tue, Feb 24, 2015 at 7:19 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> On 02/23/2015 11:36 PM, Robbie King wrote: >> >> +int odp_pool_term_global(void) >> +{ >> + int i; >> + pool_entry_t *pool; >> + int ret = 0; >> + int rc = 0; >> + >> + for (i = 0; i < ODP_CONFIG_POOLS; i++) { >> + pool = get_pool_entry(i); >> + >> + POOL_LOCK(&pool->s.lock); >> + if (pool->s.pool_shm != ODP_SHM_INVALID) { >> + ODP_ERR("Not destroyed pool: %s\n", >> pool->s.name <http://s.name>); >> + rc = -1; >> + } >> + POOL_UNLOCK(&pool->s.lock); >> + } >> >> what this for does? >> you should do: >> pool->s.name <http://s.name>[0] = 0; >> pool->s.flags.has_name = 1; >> unmap(pool->s.pool_base_addr); >> >> Also I would like to zero everything that odp_pool_create() >> created. Including atomic counters. >> >> Thanks, >> Maxim. >> >> + >> + ret = odp_shm_free(odp_shm_lookup(SHM_DEFAULT_NAME)); >> + if (ret < 0) { >> + ODP_ERR("shm free failed for %s", >> SHM_DEFAULT_NAME); >> + rc = -1; >> + } >> + >> + return rc; >> +} >> + >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
On 02/25/2015 02:03 PM, Bill Fischofer wrote: > odp_pool_create() issues an odp_shm_reserve() call. > odp_pool_destroy() issues a corresponding odp_shm_free() call. What > else is needed? Presumably any needed map/unmap calls are internal to > the odp_shm_xxx( routines, no? yes, odp_shm_free() frees shm, which is the same as pool->s.pool_base_addr. But I still think that pool termination has to be simple: for (i = 0; i < ODP_CONFIG_POOLS; i++) { pool = get_pool_entry(i); ret += odp_pool_destroy(pool_index_to_handle(pool)); } return ret; The reasons are: 1. That flag needed to be accounted: if (!pool->s.flags.user_supplied_shm) odp_shm_free(pool->s.pool_shm); You can not free pool which was not automatically allocated. 2. Set to invalid: pool->s.pool_shm = ODP_SHM_INVALID 3. No idea what should we do if there are still some packets. Probably also fail to terminate pool. /* Call fails if pool has allocated buffers */ if (odp_atomic_load_u32(&pool->s.bufcount) < pool->s.buf_num) { POOL_UNLOCK(&pool->s.lock); return -1; } Thanks, Maxim. > > On Wed, Feb 25, 2015 at 3:27 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 02/25/2015 01:21 AM, Bill Fischofer wrote: > > Maxim, there's no need to do all that since odp_pool_create() > handles a full re-init of the pool structures as part of > (re)allocating it for another create operation. > > Actually, having vestigial info about past usage can be > valuable for debugging core dumps. A pool is unallocated if > pool->s.pool_shm == ODP_SHM_INVALID and that's the key > variable that determines whether the rest of the items in the > struct are active. > > Bill > > But at least we need unmap(pool->s.pool_base_addr), to prevent > memory leaks right? > > Maxim. > > On Tue, Feb 24, 2015 at 7:19 AM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > On 02/23/2015 11:36 PM, Robbie King wrote: > > +int odp_pool_term_global(void) > +{ > + int i; > + pool_entry_t *pool; > + int ret = 0; > + int rc = 0; > + > + for (i = 0; i < ODP_CONFIG_POOLS; i++) { > + pool = get_pool_entry(i); > + > + POOL_LOCK(&pool->s.lock); > + if (pool->s.pool_shm != ODP_SHM_INVALID) { > + ODP_ERR("Not destroyed pool: > %s\n", > pool->s.name <http://s.name> <http://s.name>); > + rc = -1; > + } > + POOL_UNLOCK(&pool->s.lock); > + } > > what this for does? > you should do: > pool->s.name <http://s.name> <http://s.name>[0] = 0; > pool->s.flags.has_name = 1; > unmap(pool->s.pool_base_addr); > > Also I would like to zero everything that odp_pool_create() > created. Including atomic counters. > > Thanks, > Maxim. > > + > + ret = > odp_shm_free(odp_shm_lookup(SHM_DEFAULT_NAME)); > + if (ret < 0) { > + ODP_ERR("shm free failed for %s", > SHM_DEFAULT_NAME); > + rc = -1; > + } > + > + return rc; > +} > + > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > >
The term routines cover normal termination. That means the application is responsible for all "bookend" calls. If the application called odp_pool_create() then it is the application's responsibility to call odp_pool_destroy() as part of its normal termination path. As a result odp_pool_term_global() simply verifies that this has been done--it does not do this for the application. This is consistent with all other term routines. On Wed, Feb 25, 2015 at 6:54 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 02/25/2015 02:03 PM, Bill Fischofer wrote: > >> odp_pool_create() issues an odp_shm_reserve() call. odp_pool_destroy() >> issues a corresponding odp_shm_free() call. What else is needed? Presumably >> any needed map/unmap calls are internal to the odp_shm_xxx( routines, no? >> > yes, odp_shm_free() frees shm, which is the same as > pool->s.pool_base_addr. But I still think that pool termination has to be > simple: > for (i = 0; i < ODP_CONFIG_POOLS; i++) { > pool = get_pool_entry(i); > ret += odp_pool_destroy(pool_index_to_handle(pool)); > } > return ret; > > The reasons are: > 1. That flag needed to be accounted: > if (!pool->s.flags.user_supplied_shm) > odp_shm_free(pool->s.pool_shm); > You can not free pool which was not automatically allocated. > > 2. Set to invalid: > pool->s.pool_shm = ODP_SHM_INVALID > > 3. No idea what should we do if there are still some packets. Probably > also fail to terminate pool. > /* Call fails if pool has allocated buffers */ > if (odp_atomic_load_u32(&pool->s.bufcount) < pool->s.buf_num) { > POOL_UNLOCK(&pool->s.lock); > return -1; > } > > Thanks, > Maxim. > > >> On Wed, Feb 25, 2015 at 3:27 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> On 02/25/2015 01:21 AM, Bill Fischofer wrote: >> >> Maxim, there's no need to do all that since odp_pool_create() >> handles a full re-init of the pool structures as part of >> (re)allocating it for another create operation. >> >> Actually, having vestigial info about past usage can be >> valuable for debugging core dumps. A pool is unallocated if >> pool->s.pool_shm == ODP_SHM_INVALID and that's the key >> variable that determines whether the rest of the items in the >> struct are active. >> >> Bill >> >> But at least we need unmap(pool->s.pool_base_addr), to prevent >> memory leaks right? >> >> Maxim. >> >> On Tue, Feb 24, 2015 at 7:19 AM, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>> wrote: >> >> On 02/23/2015 11:36 PM, Robbie King wrote: >> >> +int odp_pool_term_global(void) >> +{ >> + int i; >> + pool_entry_t *pool; >> + int ret = 0; >> + int rc = 0; >> + >> + for (i = 0; i < ODP_CONFIG_POOLS; i++) { >> + pool = get_pool_entry(i); >> + >> + POOL_LOCK(&pool->s.lock); >> + if (pool->s.pool_shm != ODP_SHM_INVALID) { >> + ODP_ERR("Not destroyed pool: >> %s\n", >> pool->s.name <http://s.name> <http://s.name>); >> + rc = -1; >> + } >> + POOL_UNLOCK(&pool->s.lock); >> + } >> >> what this for does? >> you should do: >> pool->s.name <http://s.name> <http://s.name>[0] = 0; >> pool->s.flags.has_name = 1; >> unmap(pool->s.pool_base_addr); >> >> Also I would like to zero everything that odp_pool_create() >> created. Including atomic counters. >> >> Thanks, >> Maxim. >> >> + >> + ret = >> odp_shm_free(odp_shm_lookup(SHM_DEFAULT_NAME)); >> + if (ret < 0) { >> + ODP_ERR("shm free failed for %s", >> SHM_DEFAULT_NAME); >> + rc = -1; >> + } >> + >> + return rc; >> +} >> + >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org >> <mailto:lng-odp@lists.linaro.org>> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> >
On 02/25/2015 04:02 PM, Bill Fischofer wrote: > The term routines cover normal termination. That means the > application is responsible for all "bookend" calls. If the application > called odp_pool_create() then it is the application's responsibility > to call odp_pool_destroy() as part of its normal termination path. As > a result odp_pool_term_global() simply verifies that this has been > done--it does not do this for the application. > > This is consistent with all other term routines. > From that point of view patches are ok. Maxim. > On Wed, Feb 25, 2015 at 6:54 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 02/25/2015 02:03 PM, Bill Fischofer wrote: > > odp_pool_create() issues an odp_shm_reserve() call. > odp_pool_destroy() issues a corresponding odp_shm_free() call. > What else is needed? Presumably any needed map/unmap calls are > internal to the odp_shm_xxx( routines, no? > > yes, odp_shm_free() frees shm, which is the same as > pool->s.pool_base_addr. But I still think that pool termination > has to be simple: > for (i = 0; i < ODP_CONFIG_POOLS; i++) { > pool = get_pool_entry(i); > ret += odp_pool_destroy(pool_index_to_handle(pool)); > } > return ret; > > The reasons are: > 1. That flag needed to be accounted: > if (!pool->s.flags.user_supplied_shm) > odp_shm_free(pool->s.pool_shm); > You can not free pool which was not automatically allocated. > > 2. Set to invalid: > pool->s.pool_shm = ODP_SHM_INVALID > > 3. No idea what should we do if there are still some packets. > Probably also fail to terminate pool. > /* Call fails if pool has allocated buffers */ > if (odp_atomic_load_u32(&pool->s.bufcount) < > pool->s.buf_num) { > POOL_UNLOCK(&pool->s.lock); > return -1; > } > > Thanks, > Maxim. > > > On Wed, Feb 25, 2015 at 3:27 AM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > On 02/25/2015 01:21 AM, Bill Fischofer wrote: > > Maxim, there's no need to do all that since > odp_pool_create() > handles a full re-init of the pool structures as part of > (re)allocating it for another create operation. > > Actually, having vestigial info about past usage can be > valuable for debugging core dumps. A pool is > unallocated if > pool->s.pool_shm == ODP_SHM_INVALID and that's the key > variable that determines whether the rest of the items > in the > struct are active. > > Bill > > But at least we need unmap(pool->s.pool_base_addr), to prevent > memory leaks right? > > Maxim. > > On Tue, Feb 24, 2015 at 7:19 AM, Maxim Uvarov > <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>>> wrote: > > On 02/23/2015 11:36 PM, Robbie King wrote: > > +int odp_pool_term_global(void) > +{ > + int i; > + pool_entry_t *pool; > + int ret = 0; > + int rc = 0; > + > + for (i = 0; i < ODP_CONFIG_POOLS; i++) { > + pool = get_pool_entry(i); > + > + POOL_LOCK(&pool->s.lock); > + if (pool->s.pool_shm != > ODP_SHM_INVALID) { > + ODP_ERR("Not destroyed > pool: > %s\n", > pool->s.name <http://s.name> <http://s.name> > <http://s.name>); > + rc = -1; > + } > + POOL_UNLOCK(&pool->s.lock); > + } > > what this for does? > you should do: > pool->s.name <http://s.name> <http://s.name> > <http://s.name>[0] = 0; > pool->s.flags.has_name = 1; > unmap(pool->s.pool_base_addr); > > Also I would like to zero everything that > odp_pool_create() > created. Including atomic counters. > > Thanks, > Maxim. > > + > + ret = > odp_shm_free(odp_shm_lookup(SHM_DEFAULT_NAME)); > + if (ret < 0) { > + ODP_ERR("shm free failed for %s", > SHM_DEFAULT_NAME); > + rc = -1; > + } > + > + return rc; > +} > + > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>>> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > >
diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h index c6da30b..c82012d 100644 --- a/platform/linux-generic/include/odp_internal.h +++ b/platform/linux-generic/include/odp_internal.h @@ -39,6 +39,7 @@ int odp_shm_init_global(void); int odp_shm_init_local(void); int odp_pool_init_global(void); +int odp_pool_term_global(void); int odp_pktio_init_global(void); int odp_pktio_term_global(void); diff --git a/platform/linux-generic/odp_init.c b/platform/linux-generic/odp_init.c index f5e0f25..82735b0 100644 --- a/platform/linux-generic/odp_init.c +++ b/platform/linux-generic/odp_init.c @@ -103,6 +103,11 @@ int odp_term_global(void) rc = -1; } + if (odp_pool_term_global()) { + ODP_ERR("ODP buffer pool term failed.\n"); + rc = -1; + } + return rc; } diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c index 422ff2b..441429a 100644 --- a/platform/linux-generic/odp_pool.c +++ b/platform/linux-generic/odp_pool.c @@ -55,6 +55,7 @@ typedef struct pool_table_t { /* The pool table */ static pool_table_t *pool_tbl; +static const char SHM_DEFAULT_NAME[] = "odp_buffer_pools"; /* Pool entry pointers (for inlining) */ void *pool_entry_ptr[ODP_CONFIG_POOLS]; @@ -67,7 +68,7 @@ int odp_pool_init_global(void) uint32_t i; odp_shm_t shm; - shm = odp_shm_reserve("odp_pools", + shm = odp_shm_reserve(SHM_DEFAULT_NAME, sizeof(pool_table_t), sizeof(pool_entry_t), 0); @@ -95,6 +96,33 @@ int odp_pool_init_global(void) return 0; } +int odp_pool_term_global(void) +{ + int i; + pool_entry_t *pool; + int ret = 0; + int rc = 0; + + for (i = 0; i < ODP_CONFIG_POOLS; i++) { + pool = get_pool_entry(i); + + POOL_LOCK(&pool->s.lock); + if (pool->s.pool_shm != ODP_SHM_INVALID) { + ODP_ERR("Not destroyed pool: %s\n", pool->s.name); + rc = -1; + } + POOL_UNLOCK(&pool->s.lock); + } + + ret = odp_shm_free(odp_shm_lookup(SHM_DEFAULT_NAME)); + if (ret < 0) { + ODP_ERR("shm free failed for %s", SHM_DEFAULT_NAME); + rc = -1; + } + + return rc; +} + /** * Pool creation */