| Message ID | 1411660186731.24886@caviumnetworks.com |
|---|---|
| State | New |
| Headers | show |
That's an interesting suggestion and something that should be relatively easy to implement since one of the pieces of buffer pool meta data could be an allocation count that is incremented on each buffer_alloc() and decremented on each buffer_free(). Bill On Thu, Sep 25, 2014 at 10:49 AM, Rosenboim, Leonid < Leonid.Rosenboim@caviumnetworks.com> wrote: > How about checking that all buffers have been freed to the pool prior to > destroying the pool itself ? > > > Also, since odp_buffer_pool_create() gets memory from the caller, the > destroy description should spell out that the caller is also responsable > for freeing this memory for complete reverse of the action. > > > Then, if some buffers have not been returned, what to do ? > > > One option is to warn and proceed, another option is to bail and return > an error, a third option is to mark the pool as destined for deletion, > which prevents new buffers from being doled out, but allows buffer frees, > and trigger an automatic pool destruction when the last buffer is returned. > > > The risk of proceeding with destruction while some buffers are > outstanding is potential memory corruption that is very hard to debug. > > > ------------------------------ > *From:* lng-odp-bounces@lists.linaro.org <lng-odp-bounces@lists.linaro.org> > on behalf of Anders Roxell <anders.roxell@linaro.org> > *Sent:* Thursday, September 25, 2014 5:48 AM > *To:* lng-odp-forward > *Subject:* Re: [lng-odp] [PATCH] Add API odp_buffer_pool_delete > > > > On 24 September 2014 22:12, Anders Roxell <anders.roxell@linaro.org> > wrote: > >> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >> --- >> .../linux-generic/include/api/odp_buffer_pool.h | 9 ++++++++ >> platform/linux-generic/odp_buffer_pool.c | 25 >> ++++++++++++++++++++++ >> 2 files changed, 34 insertions(+) >> >> diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h >> b/platform/linux-generic/include/api/odp_buffer_pool.h >> index fe88898..fca2c62 100644 >> --- a/platform/linux-generic/include/api/odp_buffer_pool.h >> +++ b/platform/linux-generic/include/api/odp_buffer_pool.h >> @@ -34,6 +34,15 @@ typedef uint32_t odp_buffer_pool_t; >> >> >> /** >> + * Delete a buffer pool >> + * >> + * @param pool_hdl Buffer pool handle >> + * >> + * @return 0 if successful else -1 >> + */ >> +int odp_buffer_pool_delete(odp_buffer_pool_t pool_hdl); >> + >> +/** >> * Create a buffer pool >> * >> * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 >> chars) >> diff --git a/platform/linux-generic/odp_buffer_pool.c >> b/platform/linux-generic/odp_buffer_pool.c >> index 4d9ff45..7ba793d 100644 >> --- a/platform/linux-generic/odp_buffer_pool.c >> +++ b/platform/linux-generic/odp_buffer_pool.c >> @@ -375,6 +375,31 @@ static void link_bufs(pool_entry_t *pool) >> } >> } >> >> +int odp_buffer_pool_delete(odp_buffer_pool_t pool_hdl) >> +{ >> + pool_entry_t *pool; >> + if (odp_unlikely(ODP_CONFIG_BUFFER_POOLS <= pool_hdl)) >> + ODP_ERR("set_handle: Bad pool handle %u\n", pool_hdl); >> + >> + pool = get_pool_entry(pool_hdl); >> + pool->s.free_bufs = 0; >> + strcpy(pool->s.name, ""); >> + /* Need to restore this because this is setup in >> + * odp_buffer_pool_init_global >> + * */ >> + pool->s.pool_hdl = pool_hdl; >> + pool->s.buf_base = 0; >> + pool->s.buf_size = 0; >> + pool->s.buf_offset = 0; >> + pool->s.num_bufs = 0; >> + pool->s.pool_base_addr = NULL; >> + pool->s.pool_size = 0; >> + pool->s.user_size = 0; >> + pool->s.user_align = 0; >> + pool->s.buf_type = ODP_BUFFER_POOL_INVALID; >> + pool->s.hdr_size = 0; >> + return 0; >> +} >> >> odp_buffer_pool_t odp_buffer_pool_create(const char *name, >> void *base_addr, uint64_t size, >> -- >> 2.1.0 >> >> > > Hi all, > > I had a chat with Ola and he proposed destroy instead of delete > > I think I like odp_buffer_pool_destroy more than what I've done in the > patch odp_buffer_pool_delete... =) > > Comments? > > Cheers, > Anders > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > >
Free/use counters updated on every alloc and free call might not be possible to support, e.g. for HW buffer pools which allow allocation from HW blocks. Also SW implementations of (shared) counters are not scalable. Better to attempt to check if any buffers are still in use when buffer pool destruction is attempted. Perhaps buffers should be marked as free or in use and odp_buffer_pool_destroy() could traverse the buffer pool and check the flags. On 25 September 2014 17:55, Bill Fischofer <bill.fischofer@linaro.org> wrote: > That's an interesting suggestion and something that should be relatively > easy to implement since one of the pieces of buffer pool meta data could be > an allocation count that is incremented on each buffer_alloc() and > decremented on each buffer_free(). > > Bill > > On Thu, Sep 25, 2014 at 10:49 AM, Rosenboim, Leonid < > Leonid.Rosenboim@caviumnetworks.com> wrote: > >> How about checking that all buffers have been freed to the pool prior >> to destroying the pool itself ? >> >> >> Also, since odp_buffer_pool_create() gets memory from the caller, the >> destroy description should spell out that the caller is also responsable >> for freeing this memory for complete reverse of the action. >> >> >> Then, if some buffers have not been returned, what to do ? >> >> >> One option is to warn and proceed, another option is to bail and return >> an error, a third option is to mark the pool as destined for deletion, >> which prevents new buffers from being doled out, but allows buffer frees, >> and trigger an automatic pool destruction when the last buffer is returned. >> >> >> The risk of proceeding with destruction while some buffers are >> outstanding is potential memory corruption that is very hard to debug. >> >> >> ------------------------------ >> *From:* lng-odp-bounces@lists.linaro.org < >> lng-odp-bounces@lists.linaro.org> on behalf of Anders Roxell < >> anders.roxell@linaro.org> >> *Sent:* Thursday, September 25, 2014 5:48 AM >> *To:* lng-odp-forward >> *Subject:* Re: [lng-odp] [PATCH] Add API odp_buffer_pool_delete >> >> >> >> On 24 September 2014 22:12, Anders Roxell <anders.roxell@linaro.org> >> wrote: >> >>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >>> --- >>> .../linux-generic/include/api/odp_buffer_pool.h | 9 ++++++++ >>> platform/linux-generic/odp_buffer_pool.c | 25 >>> ++++++++++++++++++++++ >>> 2 files changed, 34 insertions(+) >>> >>> diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h >>> b/platform/linux-generic/include/api/odp_buffer_pool.h >>> index fe88898..fca2c62 100644 >>> --- a/platform/linux-generic/include/api/odp_buffer_pool.h >>> +++ b/platform/linux-generic/include/api/odp_buffer_pool.h >>> @@ -34,6 +34,15 @@ typedef uint32_t odp_buffer_pool_t; >>> >>> >>> /** >>> + * Delete a buffer pool >>> + * >>> + * @param pool_hdl Buffer pool handle >>> + * >>> + * @return 0 if successful else -1 >>> + */ >>> +int odp_buffer_pool_delete(odp_buffer_pool_t pool_hdl); >>> + >>> +/** >>> * Create a buffer pool >>> * >>> * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 >>> chars) >>> diff --git a/platform/linux-generic/odp_buffer_pool.c >>> b/platform/linux-generic/odp_buffer_pool.c >>> index 4d9ff45..7ba793d 100644 >>> --- a/platform/linux-generic/odp_buffer_pool.c >>> +++ b/platform/linux-generic/odp_buffer_pool.c >>> @@ -375,6 +375,31 @@ static void link_bufs(pool_entry_t *pool) >>> } >>> } >>> >>> +int odp_buffer_pool_delete(odp_buffer_pool_t pool_hdl) >>> +{ >>> + pool_entry_t *pool; >>> + if (odp_unlikely(ODP_CONFIG_BUFFER_POOLS <= pool_hdl)) >>> + ODP_ERR("set_handle: Bad pool handle %u\n", pool_hdl); >>> + >>> + pool = get_pool_entry(pool_hdl); >>> + pool->s.free_bufs = 0; >>> + strcpy(pool->s.name, ""); >>> + /* Need to restore this because this is setup in >>> + * odp_buffer_pool_init_global >>> + * */ >>> + pool->s.pool_hdl = pool_hdl; >>> + pool->s.buf_base = 0; >>> + pool->s.buf_size = 0; >>> + pool->s.buf_offset = 0; >>> + pool->s.num_bufs = 0; >>> + pool->s.pool_base_addr = NULL; >>> + pool->s.pool_size = 0; >>> + pool->s.user_size = 0; >>> + pool->s.user_align = 0; >>> + pool->s.buf_type = ODP_BUFFER_POOL_INVALID; >>> + pool->s.hdr_size = 0; >>> + return 0; >>> +} >>> >>> odp_buffer_pool_t odp_buffer_pool_create(const char *name, >>> void *base_addr, uint64_t size, >>> -- >>> 2.1.0 >>> >>> >> >> Hi all, >> >> I had a chat with Ola and he proposed destroy instead of delete >> >> I think I like odp_buffer_pool_destroy more than what I've done in the >> patch odp_buffer_pool_delete... =) >> >> Comments? >> >> Cheers, >> Anders >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > >
Given possible diversity of implementations I would suggest the wording for buffer destroy operation that is assumes that all buffer are freed and if not behavior is undefined. Implementation that can detect unfreed buffer can complain and fail upon destroy operation itself, implementation that have hard time tracking allocated h/w buffer destroy operation may proceed fine, but latter crash and burn with memory corruption case. All that will fall into "undefined" realm. Otherwise I am concerned that some implementation may have hard time to implement account all allocated buffers situation. Some other wording with MAY and MUST could be used. I.e implementation MAY fail if it detects that not all buffers are freed (but it should not be MUST). Thanks, Victor On 25 September 2014 10:53, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > Free/use counters updated on every alloc and free call might not be possible > to support, e.g. for HW buffer pools which allow allocation from HW blocks. > Also SW implementations of (shared) counters are not scalable. Better to > attempt to check if any buffers are still in use when buffer pool > destruction is attempted. Perhaps buffers should be marked as free or in use > and odp_buffer_pool_destroy() could traverse the buffer pool and check the > flags. > > On 25 September 2014 17:55, Bill Fischofer <bill.fischofer@linaro.org> > wrote: >> >> That's an interesting suggestion and something that should be relatively >> easy to implement since one of the pieces of buffer pool meta data could be >> an allocation count that is incremented on each buffer_alloc() and >> decremented on each buffer_free(). >> >> Bill >> >> On Thu, Sep 25, 2014 at 10:49 AM, Rosenboim, Leonid >> <Leonid.Rosenboim@caviumnetworks.com> wrote: >>> >>> How about checking that all buffers have been freed to the pool prior to >>> destroying the pool itself ? >>> >>> >>> Also, since odp_buffer_pool_create() gets memory from the caller, the >>> destroy description should spell out that the caller is also responsable for >>> freeing this memory for complete reverse of the action. >>> >>> >>> Then, if some buffers have not been returned, what to do ? >>> >>> >>> One option is to warn and proceed, another option is to bail and return >>> an error, a third option is to mark the pool as destined for deletion, which >>> prevents new buffers from being doled out, but allows buffer frees, and >>> trigger an automatic pool destruction when the last buffer is returned. >>> >>> >>> The risk of proceeding with destruction while some buffers are >>> outstanding is potential memory corruption that is very hard to debug. >>> >>> >>> ________________________________ >>> From: lng-odp-bounces@lists.linaro.org <lng-odp-bounces@lists.linaro.org> >>> on behalf of Anders Roxell <anders.roxell@linaro.org> >>> Sent: Thursday, September 25, 2014 5:48 AM >>> To: lng-odp-forward >>> Subject: Re: [lng-odp] [PATCH] Add API odp_buffer_pool_delete >>> >>> >>> >>> On 24 September 2014 22:12, Anders Roxell <anders.roxell@linaro.org> >>> wrote: >>>> >>>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >>>> --- >>>> .../linux-generic/include/api/odp_buffer_pool.h | 9 ++++++++ >>>> platform/linux-generic/odp_buffer_pool.c | 25 >>>> ++++++++++++++++++++++ >>>> 2 files changed, 34 insertions(+) >>>> >>>> diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h >>>> b/platform/linux-generic/include/api/odp_buffer_pool.h >>>> index fe88898..fca2c62 100644 >>>> --- a/platform/linux-generic/include/api/odp_buffer_pool.h >>>> +++ b/platform/linux-generic/include/api/odp_buffer_pool.h >>>> @@ -34,6 +34,15 @@ typedef uint32_t odp_buffer_pool_t; >>>> >>>> >>>> /** >>>> + * Delete a buffer pool >>>> + * >>>> + * @param pool_hdl Buffer pool handle >>>> + * >>>> + * @return 0 if successful else -1 >>>> + */ >>>> +int odp_buffer_pool_delete(odp_buffer_pool_t pool_hdl); >>>> + >>>> +/** >>>> * Create a buffer pool >>>> * >>>> * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 >>>> chars) >>>> diff --git a/platform/linux-generic/odp_buffer_pool.c >>>> b/platform/linux-generic/odp_buffer_pool.c >>>> index 4d9ff45..7ba793d 100644 >>>> --- a/platform/linux-generic/odp_buffer_pool.c >>>> +++ b/platform/linux-generic/odp_buffer_pool.c >>>> @@ -375,6 +375,31 @@ static void link_bufs(pool_entry_t *pool) >>>> } >>>> } >>>> >>>> +int odp_buffer_pool_delete(odp_buffer_pool_t pool_hdl) >>>> +{ >>>> + pool_entry_t *pool; >>>> + if (odp_unlikely(ODP_CONFIG_BUFFER_POOLS <= pool_hdl)) >>>> + ODP_ERR("set_handle: Bad pool handle %u\n", pool_hdl); >>>> + >>>> + pool = get_pool_entry(pool_hdl); >>>> + pool->s.free_bufs = 0; >>>> + strcpy(pool->s.name, ""); >>>> + /* Need to restore this because this is setup in >>>> + * odp_buffer_pool_init_global >>>> + * */ >>>> + pool->s.pool_hdl = pool_hdl; >>>> + pool->s.buf_base = 0; >>>> + pool->s.buf_size = 0; >>>> + pool->s.buf_offset = 0; >>>> + pool->s.num_bufs = 0; >>>> + pool->s.pool_base_addr = NULL; >>>> + pool->s.pool_size = 0; >>>> + pool->s.user_size = 0; >>>> + pool->s.user_align = 0; >>>> + pool->s.buf_type = ODP_BUFFER_POOL_INVALID; >>>> + pool->s.hdr_size = 0; >>>> + return 0; >>>> +} >>>> >>>> odp_buffer_pool_t odp_buffer_pool_create(const char *name, >>>> void *base_addr, uint64_t size, >>>> -- >>>> 2.1.0 >>>> >>> >>> >>> Hi all, >>> >>> I had a chat with Ola and he proposed destroy instead of delete >>> >>> I think I like odp_buffer_pool_destroy more than what I've done in the >>> patch odp_buffer_pool_delete... =) >>> >>> Comments? >>> >>> Cheers, >>> Anders >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h index fe88898..fca2c62 100644 --- a/platform/linux-generic/include/api/odp_buffer_pool.h +++ b/platform/linux-generic/include/api/odp_buffer_pool.h @@ -34,6 +34,15 @@ typedef uint32_t odp_buffer_pool_t; /** + * Delete a buffer pool + * + * @param pool_hdl Buffer pool handle + * + * @return 0 if successful else -1 + */ +int odp_buffer_pool_delete(odp_buffer_pool_t pool_hdl); + +/** * Create a buffer pool * * @param name Name of the pool (max ODP_BUFFER_POOL_NAME_LEN - 1 chars) diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c index 4d9ff45..7ba793d 100644 --- a/platform/linux-generic/odp_buffer_pool.c +++ b/platform/linux-generic/odp_buffer_pool.c @@ -375,6 +375,31 @@ static void link_bufs(pool_entry_t *pool) } } +int odp_buffer_pool_delete(odp_buffer_pool_t pool_hdl) +{ + pool_entry_t *pool; + if (odp_unlikely(ODP_CONFIG_BUFFER_POOLS <= pool_hdl)) + ODP_ERR("set_handle: Bad pool handle %u\n", pool_hdl); + + pool = get_pool_entry(pool_hdl); + pool->s.free_bufs = 0; + strcpy(pool->s.name<http://s.name>, ""); + /* Need to restore this because this is setup in + * odp_buffer_pool_init_global + * */ + pool->s.pool_hdl = pool_hdl; + pool->s.buf_base = 0; + pool->s.buf_size = 0; + pool->s.buf_offset = 0; + pool->s.num_bufs = 0; + pool->s.pool_base_addr = NULL; + pool->s.pool_size = 0; + pool->s.user_size = 0; + pool->s.user_align = 0; + pool->s.buf_type = ODP_BUFFER_POOL_INVALID; + pool->s.hdr_size = 0; + return 0; +} odp_buffer_pool_t odp_buffer_pool_create(const char *name, void *base_addr, uint64_t size,