| Message ID | 1429643683-9050-1-git-send-email-mike.holmes@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 21, 2015 at 2:14 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/test/validation/odp_pool.c b/test/validation/odp_pool.c > index 1a518a0..c2f9a1b 100644 > --- a/test/validation/odp_pool.c > +++ b/test/validation/odp_pool.c > @@ -11,6 +11,30 @@ static int pool_name_number = 1; > static const int default_buffer_size = 1500; > static const int default_buffer_num = 1000; > > +static void pool_double_destroy(void) > +{ > + odp_pool_param_t params = { > + .buf = { > + .size = default_buffer_size, > + .align = ODP_CACHE_LINE_SIZE, > + .num = default_buffer_num, > + }, > + .type = ODP_POOL_BUFFER, > + }; > + odp_pool_t pool; > + char pool_name[ODP_POOL_NAME_LEN]; > + > + snprintf(pool_name, sizeof(pool_name), > + "test_pool-%d", pool_name_number++); > + > + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, ¶ms); > + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); > + CU_ASSERT(odp_pool_to_u64(pool) != > + odp_pool_to_u64(ODP_POOL_INVALID)); > + CU_ASSERT(odp_pool_destroy(pool) == 0); > + CU_ASSERT(odp_pool_destroy(pool) < 0); > +} > + > static void pool_create_destroy(odp_pool_param_t *params) > { > odp_pool_t pool; > @@ -133,6 +157,7 @@ CU_TestInfo pool_tests[] = { > _CU_TEST_INFO(pool_create_destroy_timeout), > _CU_TEST_INFO(pool_create_destroy_buffer_shm), > _CU_TEST_INFO(pool_lookup_info_print), > + _CU_TEST_INFO(pool_double_destroy), > CU_TEST_INFO_NULL, > }; > > -- > 2.1.0 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 04/21/2015 10:14 PM, Mike Holmes wrote: > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > --- > test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/test/validation/odp_pool.c b/test/validation/odp_pool.c > index 1a518a0..c2f9a1b 100644 > --- a/test/validation/odp_pool.c > +++ b/test/validation/odp_pool.c > @@ -11,6 +11,30 @@ static int pool_name_number = 1; > static const int default_buffer_size = 1500; > static const int default_buffer_num = 1000; > > +static void pool_double_destroy(void) > +{ > + odp_pool_param_t params = { > + .buf = { > + .size = default_buffer_size, > + .align = ODP_CACHE_LINE_SIZE, > + .num = default_buffer_num, > + }, > + .type = ODP_POOL_BUFFER, > + }; > + odp_pool_t pool; > + char pool_name[ODP_POOL_NAME_LEN]; > + > + snprintf(pool_name, sizeof(pool_name), > + "test_pool-%d", pool_name_number++); > + > + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, ¶ms); > + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); > + CU_ASSERT(odp_pool_to_u64(pool) != > + odp_pool_to_u64(ODP_POOL_INVALID)); > + CU_ASSERT(odp_pool_destroy(pool) == 0); > + CU_ASSERT(odp_pool_destroy(pool) < 0); Is this an expected behavior? Do we have it documented somewhere? I assume behavior should be undefined in this case. After pool is destroyed its handle can't be used anymore. This test is single-threaded, but assume that there is another thread which created a pool with the same odp_pool_t handle just between two odp_pool_destroy(pool) calls. A second odp_pool_destroy() will destroy a new pool and return 0. If you demand this behavior, then you effectively force implementation to use generation-tagged handles.
Good points. I agree it's better to leave this behavior undefined. On Wed, Apr 22, 2015 at 5:44 AM, Taras Kondratiuk < taras.kondratiuk@linaro.org> wrote: > On 04/21/2015 10:14 PM, Mike Holmes wrote: > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >> --- >> test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/test/validation/odp_pool.c b/test/validation/odp_pool.c >> index 1a518a0..c2f9a1b 100644 >> --- a/test/validation/odp_pool.c >> +++ b/test/validation/odp_pool.c >> @@ -11,6 +11,30 @@ static int pool_name_number = 1; >> static const int default_buffer_size = 1500; >> static const int default_buffer_num = 1000; >> >> +static void pool_double_destroy(void) >> +{ >> + odp_pool_param_t params = { >> + .buf = { >> + .size = default_buffer_size, >> + .align = ODP_CACHE_LINE_SIZE, >> + .num = default_buffer_num, >> + }, >> + .type = ODP_POOL_BUFFER, >> + }; >> + odp_pool_t pool; >> + char pool_name[ODP_POOL_NAME_LEN]; >> + >> + snprintf(pool_name, sizeof(pool_name), >> + "test_pool-%d", pool_name_number++); >> + >> + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, ¶ms); >> + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); >> + CU_ASSERT(odp_pool_to_u64(pool) != >> + odp_pool_to_u64(ODP_POOL_INVALID)); >> + CU_ASSERT(odp_pool_destroy(pool) == 0); >> + CU_ASSERT(odp_pool_destroy(pool) < 0); >> > > Is this an expected behavior? Do we have it documented somewhere? > I assume behavior should be undefined in this case. After pool is > destroyed its handle can't be used anymore. > > This test is single-threaded, but assume that there is another thread > which created a pool with the same odp_pool_t handle just between > two odp_pool_destroy(pool) calls. A second odp_pool_destroy() will > destroy a new pool and return 0. > If you demand this behavior, then you effectively force implementation > to use generation-tagged handles. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 22 April 2015 at 06:44, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote: > On 04/21/2015 10:14 PM, Mike Holmes wrote: > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >> --- >> test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/test/validation/odp_pool.c b/test/validation/odp_pool.c >> index 1a518a0..c2f9a1b 100644 >> --- a/test/validation/odp_pool.c >> +++ b/test/validation/odp_pool.c >> @@ -11,6 +11,30 @@ static int pool_name_number = 1; >> static const int default_buffer_size = 1500; >> static const int default_buffer_num = 1000; >> >> +static void pool_double_destroy(void) >> +{ >> + odp_pool_param_t params = { >> + .buf = { >> + .size = default_buffer_size, >> + .align = ODP_CACHE_LINE_SIZE, >> + .num = default_buffer_num, >> + }, >> + .type = ODP_POOL_BUFFER, >> + }; >> + odp_pool_t pool; >> + char pool_name[ODP_POOL_NAME_LEN]; >> + >> + snprintf(pool_name, sizeof(pool_name), >> + "test_pool-%d", pool_name_number++); >> + >> + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, ¶ms); >> + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); >> + CU_ASSERT(odp_pool_to_u64(pool) != >> + odp_pool_to_u64(ODP_POOL_INVALID)); >> + CU_ASSERT(odp_pool_destroy(pool) == 0); >> + CU_ASSERT(odp_pool_destroy(pool) < 0); >> > > Is this an expected behavior? Yes I think it could be if you delete a bad pool. This test arose more specifically to stress the error leg of this function using only the public API - our validation does very little to quantify what is permissible beyond very optimistic cases, and in part to clarify the behavior in the docs when you do try to do those other things. Do we have it documented somewhere? > Not yet but I want it to be. If the test goes in then we can add the specific case that deleted a destroyed pool returns an error, if not we can document that it is undefined. I assume behavior should be undefined in this case. After pool is > destroyed its handle can't be used anymore. > We need to document that in words that that is the case. > > This test is single-threaded, but assume that there is another thread > which created a pool with the same odp_pool_t handle just between > two odp_pool_destroy(pool) calls. A second odp_pool_destroy() will > destroy a new pool and return 0. > If you demand this behavior, then you effectively force implementation > to use generation-tagged handles. >
On 22 April 2015 at 07:26, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Good points. I agree it's better to leave this behavior undefined. > If that is consensus I will send a patch for the docs to add. "Deleting an already deleted pool results in unspecified behavior." > > On Wed, Apr 22, 2015 at 5:44 AM, Taras Kondratiuk < > taras.kondratiuk@linaro.org> wrote: > >> On 04/21/2015 10:14 PM, Mike Holmes wrote: >> >>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >>> --- >>> test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/test/validation/odp_pool.c b/test/validation/odp_pool.c >>> index 1a518a0..c2f9a1b 100644 >>> --- a/test/validation/odp_pool.c >>> +++ b/test/validation/odp_pool.c >>> @@ -11,6 +11,30 @@ static int pool_name_number = 1; >>> static const int default_buffer_size = 1500; >>> static const int default_buffer_num = 1000; >>> >>> +static void pool_double_destroy(void) >>> +{ >>> + odp_pool_param_t params = { >>> + .buf = { >>> + .size = default_buffer_size, >>> + .align = ODP_CACHE_LINE_SIZE, >>> + .num = default_buffer_num, >>> + }, >>> + .type = ODP_POOL_BUFFER, >>> + }; >>> + odp_pool_t pool; >>> + char pool_name[ODP_POOL_NAME_LEN]; >>> + >>> + snprintf(pool_name, sizeof(pool_name), >>> + "test_pool-%d", pool_name_number++); >>> + >>> + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, ¶ms); >>> + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); >>> + CU_ASSERT(odp_pool_to_u64(pool) != >>> + odp_pool_to_u64(ODP_POOL_INVALID)); >>> + CU_ASSERT(odp_pool_destroy(pool) == 0); >>> + CU_ASSERT(odp_pool_destroy(pool) < 0); >>> >> >> Is this an expected behavior? Do we have it documented somewhere? >> I assume behavior should be undefined in this case. After pool is >> destroyed its handle can't be used anymore. >> >> This test is single-threaded, but assume that there is another thread >> which created a pool with the same odp_pool_t handle just between >> two odp_pool_destroy(pool) calls. A second odp_pool_destroy() will >> destroy a new pool and return 0. >> If you demand this behavior, then you effectively force implementation >> to use generation-tagged handles. >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >
We just need to be specific about what we're testing here. This really is no different than attempting to free a memory area twice. Clearly an error, but do we require an implementation to detect and report this or is it an application responsibility? Taras' point about race conditions in multithreaded apps is a good one. While this specific test is single-threaded, that's not going to be the case for most ODP applications. Perhaps things like this are better detected by tools like coverity? On Wed, Apr 22, 2015 at 6:29 AM, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 22 April 2015 at 07:26, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> Good points. I agree it's better to leave this behavior undefined. >> > > If that is consensus I will send a patch for the docs to add. > > "Deleting an already deleted pool results in unspecified behavior." > > >> >> On Wed, Apr 22, 2015 at 5:44 AM, Taras Kondratiuk < >> taras.kondratiuk@linaro.org> wrote: >> >>> On 04/21/2015 10:14 PM, Mike Holmes wrote: >>> >>>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >>>> --- >>>> test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ >>>> 1 file changed, 25 insertions(+) >>>> >>>> diff --git a/test/validation/odp_pool.c b/test/validation/odp_pool.c >>>> index 1a518a0..c2f9a1b 100644 >>>> --- a/test/validation/odp_pool.c >>>> +++ b/test/validation/odp_pool.c >>>> @@ -11,6 +11,30 @@ static int pool_name_number = 1; >>>> static const int default_buffer_size = 1500; >>>> static const int default_buffer_num = 1000; >>>> >>>> +static void pool_double_destroy(void) >>>> +{ >>>> + odp_pool_param_t params = { >>>> + .buf = { >>>> + .size = default_buffer_size, >>>> + .align = ODP_CACHE_LINE_SIZE, >>>> + .num = default_buffer_num, >>>> + }, >>>> + .type = ODP_POOL_BUFFER, >>>> + }; >>>> + odp_pool_t pool; >>>> + char pool_name[ODP_POOL_NAME_LEN]; >>>> + >>>> + snprintf(pool_name, sizeof(pool_name), >>>> + "test_pool-%d", pool_name_number++); >>>> + >>>> + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, ¶ms); >>>> + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); >>>> + CU_ASSERT(odp_pool_to_u64(pool) != >>>> + odp_pool_to_u64(ODP_POOL_INVALID)); >>>> + CU_ASSERT(odp_pool_destroy(pool) == 0); >>>> + CU_ASSERT(odp_pool_destroy(pool) < 0); >>>> >>> >>> Is this an expected behavior? Do we have it documented somewhere? >>> I assume behavior should be undefined in this case. After pool is >>> destroyed its handle can't be used anymore. >>> >>> This test is single-threaded, but assume that there is another thread >>> which created a pool with the same odp_pool_t handle just between >>> two odp_pool_destroy(pool) calls. A second odp_pool_destroy() will >>> destroy a new pool and return 0. >>> If you demand this behavior, then you effectively force implementation >>> to use generation-tagged handles. >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > > >
On 04/22/2015 02:29 PM, Mike Holmes wrote: > > > On 22 April 2015 at 07:26, Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> wrote: > > Good points. I agree it's better to leave this behavior undefined. > > > If that is consensus I will send a patch for the docs to add. > > "Deleting an already deleted pool results in unspecified behavior." odp_pktio_t, odp_shm_t, etc also cannot be destroyed twice. I think it valid for any ODP handle. I think it should be more generic. "Using an already destroyed ODP handle results in undefined behavior."
I think in that case double free of the pool *has to be defined*. The other deal is that odp_pool_destroy(odp_pool_t pool_hdl) should lock pool table, then check if handle is valid and if it's not valid - then return error. Maxim. On 04/22/15 14:26, Mike Holmes wrote: > > > On 22 April 2015 at 06:44, Taras Kondratiuk > <taras.kondratiuk@linaro.org <mailto:taras.kondratiuk@linaro.org>> wrote: > > On 04/21/2015 10:14 PM, Mike Holmes wrote: > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>> > --- > test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/test/validation/odp_pool.c > b/test/validation/odp_pool.c > index 1a518a0..c2f9a1b 100644 > --- a/test/validation/odp_pool.c > +++ b/test/validation/odp_pool.c > @@ -11,6 +11,30 @@ static int pool_name_number = 1; > static const int default_buffer_size = 1500; > static const int default_buffer_num = 1000; > > +static void pool_double_destroy(void) > +{ > + odp_pool_param_t params = { > + .buf = { > + .size = default_buffer_size, > + .align = ODP_CACHE_LINE_SIZE, > + .num = default_buffer_num, > + }, > + .type = ODP_POOL_BUFFER, > + }; > + odp_pool_t pool; > + char pool_name[ODP_POOL_NAME_LEN]; > + > + snprintf(pool_name, sizeof(pool_name), > + "test_pool-%d", pool_name_number++); > + > + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, > ¶ms); > + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); > + CU_ASSERT(odp_pool_to_u64(pool) != > + odp_pool_to_u64(ODP_POOL_INVALID)); > + CU_ASSERT(odp_pool_destroy(pool) == 0); > + CU_ASSERT(odp_pool_destroy(pool) < 0); > > > Is this an expected behavior? > > > Yes I think it could be if you delete a bad pool. > This test arose more specifically to stress the error leg of this > function using only the public API - our validation does very little > to quantify what is permissible beyond very optimistic cases, and in > part to clarify the behavior in the docs when you do try to do those > other things. > > Do we have it documented somewhere? > > Not yet but I want it to be. > If the test goes in then we can add the specific case that deleted a > destroyed pool returns an error, if not we can document that it is > undefined. > > > I assume behavior should be undefined in this case. After pool is > destroyed its handle can't be used anymore. > > > We need to document that in words that that is the case. > > > This test is single-threaded, but assume that there is another thread > which created a pool with the same odp_pool_t handle just between > two odp_pool_destroy(pool) calls. A second odp_pool_destroy() will > destroy a new pool and return 0. > If you demand this behavior, then you effectively force implementation > to use generation-tagged handles. > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
It does that, but as Taras points out there is a race condition. If one thread does an odp_pool_destroy() and it succeeds, another thread could do an odp_pool_create() before the second odp_pool_destroy() and get the same pool handle which would then be deleted by the second odp_pool_destroy() call. On Wed, Apr 22, 2015 at 7:13 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > I think in that case double free of the pool *has to be defined*. The > other deal is that odp_pool_destroy(odp_pool_t pool_hdl) > should lock pool table, then > check if handle is valid > and if it's not valid - then return error. > > > Maxim. > > On 04/22/15 14:26, Mike Holmes wrote: > >> >> >> On 22 April 2015 at 06:44, Taras Kondratiuk <taras.kondratiuk@linaro.org >> <mailto:taras.kondratiuk@linaro.org>> wrote: >> >> On 04/21/2015 10:14 PM, Mike Holmes wrote: >> >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org >> <mailto:mike.holmes@linaro.org>> >> >> --- >> test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/test/validation/odp_pool.c >> b/test/validation/odp_pool.c >> index 1a518a0..c2f9a1b 100644 >> --- a/test/validation/odp_pool.c >> +++ b/test/validation/odp_pool.c >> @@ -11,6 +11,30 @@ static int pool_name_number = 1; >> static const int default_buffer_size = 1500; >> static const int default_buffer_num = 1000; >> >> +static void pool_double_destroy(void) >> +{ >> + odp_pool_param_t params = { >> + .buf = { >> + .size = default_buffer_size, >> + .align = ODP_CACHE_LINE_SIZE, >> + .num = default_buffer_num, >> + }, >> + .type = ODP_POOL_BUFFER, >> + }; >> + odp_pool_t pool; >> + char pool_name[ODP_POOL_NAME_LEN]; >> + >> + snprintf(pool_name, sizeof(pool_name), >> + "test_pool-%d", pool_name_number++); >> + >> + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, >> ¶ms); >> + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); >> + CU_ASSERT(odp_pool_to_u64(pool) != >> + odp_pool_to_u64(ODP_POOL_INVALID)); >> + CU_ASSERT(odp_pool_destroy(pool) == 0); >> + CU_ASSERT(odp_pool_destroy(pool) < 0); >> >> >> Is this an expected behavior? >> >> Yes I think it could be if you delete a bad pool. >> This test arose more specifically to stress the error leg of this >> function using only the public API - our validation does very little to >> quantify what is permissible beyond very optimistic cases, and in part to >> clarify the behavior in the docs when you do try to do those other things. >> >> Do we have it documented somewhere? >> >> Not yet but I want it to be. >> If the test goes in then we can add the specific case that deleted a >> destroyed pool returns an error, if not we can document that it is >> undefined. >> >> >> I assume behavior should be undefined in this case. After pool is >> destroyed its handle can't be used anymore. >> >> >> We need to document that in words that that is the case. >> >> >> This test is single-threaded, but assume that there is another thread >> which created a pool with the same odp_pool_t handle just between >> two odp_pool_destroy(pool) calls. A second odp_pool_destroy() will >> destroy a new pool and return 0. >> If you demand this behavior, then you effectively force implementation >> to use generation-tagged handles. >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM >> SoCs >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 22 April 2015 at 08:57, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of ext > > Taras Kondratiuk > > Sent: Wednesday, April 22, 2015 2:47 PM > > To: Mike Holmes > > Cc: LNG ODP Mailman List > > Subject: Re: [lng-odp] [PATCH] validation: odp_pool: add double destroy > > > > On 04/22/2015 02:29 PM, Mike Holmes wrote: > > > > > > > > > On 22 April 2015 at 07:26, Bill Fischofer <bill.fischofer@linaro.org > > > <mailto:bill.fischofer@linaro.org>> wrote: > > > > > > Good points. I agree it's better to leave this behavior undefined. > > > > > > > > > If that is consensus I will send a patch for the docs to add. > > > > > > "Deleting an already deleted pool results in unspecified behavior." > > > > odp_pktio_t, odp_shm_t, etc also cannot be destroyed twice. I think it > > valid for any ODP handle. > > > > I think it should be more generic. > > "Using an already destroyed ODP handle results in undefined behavior." > > Agree. This could be documented on the top level of API reference manual > (doxygen docs). > It can be at the top as a general point, should be as a principle I agree. However that is not what an Engineer sees when looking up an API, I think we should state it per delete API, that is a very small overhead and very explicit for the few delete apis. > > -Petri > >
From: ext Mike Holmes [mailto:mike.holmes@linaro.org] Sent: Wednesday, April 22, 2015 4:00 PM To: Savolainen, Petri (Nokia - FI/Espoo) Cc: ext Taras Kondratiuk; LNG ODP Mailman List Subject: Re: [lng-odp] [PATCH] validation: odp_pool: add double destroy On 22 April 2015 at 08:57, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com<mailto:petri.savolainen@nokia.com>> wrote: > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org<mailto:lng-odp-bounces@lists.linaro.org>] On Behalf Of ext > Taras Kondratiuk > Sent: Wednesday, April 22, 2015 2:47 PM > To: Mike Holmes > Cc: LNG ODP Mailman List > Subject: Re: [lng-odp] [PATCH] validation: odp_pool: add double destroy > > On 04/22/2015 02:29 PM, Mike Holmes wrote: > > > > > > On 22 April 2015 at 07:26, Bill Fischofer <bill.fischofer@linaro.org<mailto:bill.fischofer@linaro.org> > > <mailto:bill.fischofer@linaro.org<mailto:bill.fischofer@linaro.org>>> wrote: > > > > Good points. I agree it's better to leave this behavior undefined. > > > > > > If that is consensus I will send a patch for the docs to add. > > > > "Deleting an already deleted pool results in unspecified behavior." > > odp_pktio_t, odp_shm_t, etc also cannot be destroyed twice. I think it > valid for any ODP handle. > > I think it should be more generic. > "Using an already destroyed ODP handle results in undefined behavior." Agree. This could be documented on the top level of API reference manual (doxygen docs). It can be at the top as a general point, should be as a principle I agree. However that is not what an Engineer sees when looking up an API, I think we should state it per delete API, that is a very small overhead and very explicit for the few delete apis. Per delete API is OK. No need to repeat it on every API call that has a handle parameter. -Petri
On 04/22/15 15:53, Bill Fischofer wrote: > It does that, but as Taras points out there is a race condition. If > one thread does an odp_pool_destroy() and it succeeds, another thread > could do an odp_pool_create() before the second odp_pool_destroy() and > get the same pool handle which would then be deleted by the second > odp_pool_destroy() call. > odp_pool_destroy() should hold lock inside it. (i.e. it already does that). > On Wed, Apr 22, 2015 at 7:13 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > I think in that case double free of the pool *has to be defined*. > The other deal is that odp_pool_destroy(odp_pool_t pool_hdl) > should lock pool table, then > check if handle is valid > and if it's not valid - then return error. > > > Maxim. > > On 04/22/15 14:26, Mike Holmes wrote: > > > > On 22 April 2015 at 06:44, Taras Kondratiuk > <taras.kondratiuk@linaro.org > <mailto:taras.kondratiuk@linaro.org> > <mailto:taras.kondratiuk@linaro.org > <mailto:taras.kondratiuk@linaro.org>>> wrote: > > On 04/21/2015 10:14 PM, Mike Holmes wrote: > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org> > <mailto:mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>>> > > --- > test/validation/odp_pool.c | 25 > +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/test/validation/odp_pool.c > b/test/validation/odp_pool.c > index 1a518a0..c2f9a1b 100644 > --- a/test/validation/odp_pool.c > +++ b/test/validation/odp_pool.c > @@ -11,6 +11,30 @@ static int pool_name_number = 1; > static const int default_buffer_size = 1500; > static const int default_buffer_num = 1000; > > +static void pool_double_destroy(void) > +{ > + odp_pool_param_t params = { > + .buf = { > + .size = > default_buffer_size, > + .align = > ODP_CACHE_LINE_SIZE, > + .num = > default_buffer_num, > + }, > + .type = ODP_POOL_BUFFER, > + }; > + odp_pool_t pool; > + char pool_name[ODP_POOL_NAME_LEN]; > + > + snprintf(pool_name, sizeof(pool_name), > + "test_pool-%d", pool_name_number++); > + > + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, > ¶ms); > + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); > + CU_ASSERT(odp_pool_to_u64(pool) != > + odp_pool_to_u64(ODP_POOL_INVALID)); > + CU_ASSERT(odp_pool_destroy(pool) == 0); > + CU_ASSERT(odp_pool_destroy(pool) < 0); > > > Is this an expected behavior? > > Yes I think it could be if you delete a bad pool. > This test arose more specifically to stress the error leg of > this function using only the public API - our validation does > very little to quantify what is permissible beyond very > optimistic cases, and in part to clarify the behavior in the > docs when you do try to do those other things. > > Do we have it documented somewhere? > > Not yet but I want it to be. > If the test goes in then we can add the specific case that > deleted a destroyed pool returns an error, if not we can > document that it is undefined. > > > I assume behavior should be undefined in this case. After > pool is > destroyed its handle can't be used anymore. > > > We need to document that in words that that is the case. > > > This test is single-threaded, but assume that there is > another thread > which created a pool with the same odp_pool_t handle just > between > two odp_pool_destroy(pool) calls. A second > odp_pool_destroy() will > destroy a new pool and return 0. > If you demand this behavior, then you effectively force > implementation > to use generation-tagged handles. > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/>***│ *Open source software > for ARM SoCs > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
On Wed, Apr 22, 2015 at 5:13 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 04/22/15 15:53, Bill Fischofer wrote: >> >> It does that, but as Taras points out there is a race condition. If one >> thread does an odp_pool_destroy() and it succeeds, another thread could do >> an odp_pool_create() before the second odp_pool_destroy() and get the same >> pool handle which would then be deleted by the second odp_pool_destroy() >> call. >> > odp_pool_destroy() should hold lock inside it. (i.e. it already does that). The scenario is not about a race condition on create/delete, it's actually even simpler: th1: lock -> create_pool -> unlock th1: lock -> destroy_pool -> unlock th2: lock -> create_pool -> unlock th1: lock -> destroy_pool -> unlock > > >> On Wed, Apr 22, 2015 at 7:13 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> I think in that case double free of the pool *has to be defined*. >> The other deal is that odp_pool_destroy(odp_pool_t pool_hdl) >> should lock pool table, then >> check if handle is valid >> and if it's not valid - then return error. >> >> >> Maxim. >> >> On 04/22/15 14:26, Mike Holmes wrote: >> >> >> >> On 22 April 2015 at 06:44, Taras Kondratiuk >> <taras.kondratiuk@linaro.org >> <mailto:taras.kondratiuk@linaro.org> >> <mailto:taras.kondratiuk@linaro.org >> <mailto:taras.kondratiuk@linaro.org>>> wrote: >> >> On 04/21/2015 10:14 PM, Mike Holmes wrote: >> >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org >> <mailto:mike.holmes@linaro.org> >> <mailto:mike.holmes@linaro.org >> >> <mailto:mike.holmes@linaro.org>>> >> >> --- >> test/validation/odp_pool.c | 25 >> +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/test/validation/odp_pool.c >> b/test/validation/odp_pool.c >> index 1a518a0..c2f9a1b 100644 >> --- a/test/validation/odp_pool.c >> +++ b/test/validation/odp_pool.c >> @@ -11,6 +11,30 @@ static int pool_name_number = 1; >> static const int default_buffer_size = 1500; >> static const int default_buffer_num = 1000; >> >> +static void pool_double_destroy(void) >> +{ >> + odp_pool_param_t params = { >> + .buf = { >> + .size = >> default_buffer_size, >> + .align = >> ODP_CACHE_LINE_SIZE, >> + .num = >> default_buffer_num, >> + }, >> + .type = ODP_POOL_BUFFER, >> + }; >> + odp_pool_t pool; >> + char pool_name[ODP_POOL_NAME_LEN]; >> + >> + snprintf(pool_name, sizeof(pool_name), >> + "test_pool-%d", pool_name_number++); >> + >> + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, >> ¶ms); >> + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); >> + CU_ASSERT(odp_pool_to_u64(pool) != >> + odp_pool_to_u64(ODP_POOL_INVALID)); >> + CU_ASSERT(odp_pool_destroy(pool) == 0); >> + CU_ASSERT(odp_pool_destroy(pool) < 0); >> >> >> Is this an expected behavior? >> >> Yes I think it could be if you delete a bad pool. >> This test arose more specifically to stress the error leg of >> this function using only the public API - our validation does >> very little to quantify what is permissible beyond very >> optimistic cases, and in part to clarify the behavior in the >> docs when you do try to do those other things. >> >> Do we have it documented somewhere? >> >> Not yet but I want it to be. >> If the test goes in then we can add the specific case that >> deleted a destroyed pool returns an error, if not we can >> document that it is undefined. >> >> >> I assume behavior should be undefined in this case. After >> pool is >> destroyed its handle can't be used anymore. >> >> >> We need to document that in words that that is the case. >> >> >> This test is single-threaded, but assume that there is >> another thread >> which created a pool with the same odp_pool_t handle just >> between >> two odp_pool_destroy(pool) calls. A second >> odp_pool_destroy() will >> destroy a new pool and return 0. >> If you demand this behavior, then you effectively force >> implementation >> to use generation-tagged handles. >> >> >> >> >> -- Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software >> for ARM SoCs >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
On 04/22/15 19:06, Ciprian Barbu wrote: > On Wed, Apr 22, 2015 at 5:13 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> On 04/22/15 15:53, Bill Fischofer wrote: >>> It does that, but as Taras points out there is a race condition. If one >>> thread does an odp_pool_destroy() and it succeeds, another thread could do >>> an odp_pool_create() before the second odp_pool_destroy() and get the same >>> pool handle which would then be deleted by the second odp_pool_destroy() >>> call. >>> >> odp_pool_destroy() should hold lock inside it. (i.e. it already does that). > The scenario is not about a race condition on create/delete, it's > actually even simpler: > > th1: lock -> create_pool -> unlock > th1: lock -> destroy_pool -> unlock > th2: lock -> create_pool -> unlock > th1: lock -> destroy_pool -> unlock Ok, but it's not undefined behavior. If you work with threads you should know what you do. So behavior is: destroy function fails if pool already destroyed. I still think that Mikes test is valid. It's single threaded application with very exact case. Analogy for example: char *a = malloc(10); a[5] = 1; On your logic it has to be undefined behavior due to some other thread can free or malloc with different size. Maxim. >> >>> On Wed, Apr 22, 2015 at 7:13 AM, Maxim Uvarov <maxim.uvarov@linaro.org >>> <mailto:maxim.uvarov@linaro.org>> wrote: >>> >>> I think in that case double free of the pool *has to be defined*. >>> The other deal is that odp_pool_destroy(odp_pool_t pool_hdl) >>> should lock pool table, then >>> check if handle is valid >>> and if it's not valid - then return error. >>> >>> >>> Maxim. >>> >>> On 04/22/15 14:26, Mike Holmes wrote: >>> >>> >>> >>> On 22 April 2015 at 06:44, Taras Kondratiuk >>> <taras.kondratiuk@linaro.org >>> <mailto:taras.kondratiuk@linaro.org> >>> <mailto:taras.kondratiuk@linaro.org >>> <mailto:taras.kondratiuk@linaro.org>>> wrote: >>> >>> On 04/21/2015 10:14 PM, Mike Holmes wrote: >>> >>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org >>> <mailto:mike.holmes@linaro.org> >>> <mailto:mike.holmes@linaro.org >>> >>> <mailto:mike.holmes@linaro.org>>> >>> >>> --- >>> test/validation/odp_pool.c | 25 >>> +++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/test/validation/odp_pool.c >>> b/test/validation/odp_pool.c >>> index 1a518a0..c2f9a1b 100644 >>> --- a/test/validation/odp_pool.c >>> +++ b/test/validation/odp_pool.c >>> @@ -11,6 +11,30 @@ static int pool_name_number = 1; >>> static const int default_buffer_size = 1500; >>> static const int default_buffer_num = 1000; >>> >>> +static void pool_double_destroy(void) >>> +{ >>> + odp_pool_param_t params = { >>> + .buf = { >>> + .size = >>> default_buffer_size, >>> + .align = >>> ODP_CACHE_LINE_SIZE, >>> + .num = >>> default_buffer_num, >>> + }, >>> + .type = ODP_POOL_BUFFER, >>> + }; >>> + odp_pool_t pool; >>> + char pool_name[ODP_POOL_NAME_LEN]; >>> + >>> + snprintf(pool_name, sizeof(pool_name), >>> + "test_pool-%d", pool_name_number++); >>> + >>> + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, >>> ¶ms); >>> + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); >>> + CU_ASSERT(odp_pool_to_u64(pool) != >>> + odp_pool_to_u64(ODP_POOL_INVALID)); >>> + CU_ASSERT(odp_pool_destroy(pool) == 0); >>> + CU_ASSERT(odp_pool_destroy(pool) < 0); >>> >>> >>> Is this an expected behavior? >>> >>> Yes I think it could be if you delete a bad pool. >>> This test arose more specifically to stress the error leg of >>> this function using only the public API - our validation does >>> very little to quantify what is permissible beyond very >>> optimistic cases, and in part to clarify the behavior in the >>> docs when you do try to do those other things. >>> >>> Do we have it documented somewhere? >>> >>> Not yet but I want it to be. >>> If the test goes in then we can add the specific case that >>> deleted a destroyed pool returns an error, if not we can >>> document that it is undefined. >>> >>> >>> I assume behavior should be undefined in this case. After >>> pool is >>> destroyed its handle can't be used anymore. >>> >>> >>> We need to document that in words that that is the case. >>> >>> >>> This test is single-threaded, but assume that there is >>> another thread >>> which created a pool with the same odp_pool_t handle just >>> between >>> two odp_pool_destroy(pool) calls. A second >>> odp_pool_destroy() will >>> destroy a new pool and return 0. >>> If you demand this behavior, then you effectively force >>> implementation >>> to use generation-tagged handles. >>> >>> >>> >>> >>> -- Mike Holmes >>> Technical Manager - Linaro Networking Group >>> Linaro.org <http://www.linaro.org/>***│ *Open source software >>> for ARM SoCs >>> >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >>> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp
On 04/22/2015 08:54 PM, Maxim Uvarov wrote: > On 04/22/15 19:06, Ciprian Barbu wrote: >> On Wed, Apr 22, 2015 at 5:13 PM, Maxim Uvarov >> <maxim.uvarov@linaro.org> wrote: >>> On 04/22/15 15:53, Bill Fischofer wrote: >>>> It does that, but as Taras points out there is a race condition. If >>>> one >>>> thread does an odp_pool_destroy() and it succeeds, another thread >>>> could do >>>> an odp_pool_create() before the second odp_pool_destroy() and get >>>> the same >>>> pool handle which would then be deleted by the second >>>> odp_pool_destroy() >>>> call. >>>> >>> odp_pool_destroy() should hold lock inside it. (i.e. it already does >>> that). >> The scenario is not about a race condition on create/delete, it's >> actually even simpler: >> >> th1: lock -> create_pool -> unlock >> th1: lock -> destroy_pool -> unlock >> th2: lock -> create_pool -> unlock >> th1: lock -> destroy_pool -> unlock > Ok, but it's not undefined behavior. If you work with threads you should > know what you do. > > So behavior is: destroy function fails if pool already destroyed. > > I still think that Mikes test is valid. It's single threaded application > with very exact case. > Analogy for example: > char *a = malloc(10); > a[5] = 1; > > On your logic it has to be undefined behavior due to some other thread > can free or malloc with different size. This in not a valid analogy. Should be something like this. char *a = malloc(10); free(a); free(a); /* Here you may free a block which is allocated again by another thread */
On 04/23/15 13:09, Taras Kondratiuk wrote: > On 04/22/2015 08:54 PM, Maxim Uvarov wrote: >> On 04/22/15 19:06, Ciprian Barbu wrote: >>> On Wed, Apr 22, 2015 at 5:13 PM, Maxim Uvarov >>> <maxim.uvarov@linaro.org> wrote: >>>> On 04/22/15 15:53, Bill Fischofer wrote: >>>>> It does that, but as Taras points out there is a race condition. If >>>>> one >>>>> thread does an odp_pool_destroy() and it succeeds, another thread >>>>> could do >>>>> an odp_pool_create() before the second odp_pool_destroy() and get >>>>> the same >>>>> pool handle which would then be deleted by the second >>>>> odp_pool_destroy() >>>>> call. >>>>> >>>> odp_pool_destroy() should hold lock inside it. (i.e. it already does >>>> that). >>> The scenario is not about a race condition on create/delete, it's >>> actually even simpler: >>> >>> th1: lock -> create_pool -> unlock >>> th1: lock -> destroy_pool -> unlock >>> th2: lock -> create_pool -> unlock >>> th1: lock -> destroy_pool -> unlock >> Ok, but it's not undefined behavior. If you work with threads you should >> know what you do. >> >> So behavior is: destroy function fails if pool already destroyed. >> >> I still think that Mikes test is valid. It's single threaded application >> with very exact case. >> Analogy for example: >> char *a = malloc(10); >> a[5] = 1; >> >> On your logic it has to be undefined behavior due to some other thread >> can free or malloc with different size. > This in not a valid analogy. Should be something like this. > > char *a = malloc(10); > free(a); > free(a); /* Here you may free a block which is allocated again by another thread */ Yes, and glibc will warn if you do double free. I still think that it's reasonable to accept that patch. Maxim.
On 23 April 2015 at 16:56, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 04/23/15 13:09, Taras Kondratiuk wrote: >> >> On 04/22/2015 08:54 PM, Maxim Uvarov wrote: >>> >>> On 04/22/15 19:06, Ciprian Barbu wrote: >>>> >>>> On Wed, Apr 22, 2015 at 5:13 PM, Maxim Uvarov >>>> <maxim.uvarov@linaro.org> wrote: >>>>> >>>>> On 04/22/15 15:53, Bill Fischofer wrote: >>>>>> >>>>>> It does that, but as Taras points out there is a race condition. If >>>>>> one >>>>>> thread does an odp_pool_destroy() and it succeeds, another thread >>>>>> could do >>>>>> an odp_pool_create() before the second odp_pool_destroy() and get >>>>>> the same >>>>>> pool handle which would then be deleted by the second >>>>>> odp_pool_destroy() >>>>>> call. >>>>>> >>>>> odp_pool_destroy() should hold lock inside it. (i.e. it already does >>>>> that). >>>> >>>> The scenario is not about a race condition on create/delete, it's >>>> actually even simpler: >>>> >>>> th1: lock -> create_pool -> unlock >>>> th1: lock -> destroy_pool -> unlock >>>> th2: lock -> create_pool -> unlock >>>> th1: lock -> destroy_pool -> unlock >>> >>> Ok, but it's not undefined behavior. If you work with threads you should >>> know what you do. >>> >>> So behavior is: destroy function fails if pool already destroyed. >>> >>> I still think that Mikes test is valid. It's single threaded application >>> with very exact case. >>> Analogy for example: >>> char *a = malloc(10); >>> a[5] = 1; >>> >>> On your logic it has to be undefined behavior due to some other thread >>> can free or malloc with different size. >> >> This in not a valid analogy. Should be something like this. >> >> char *a = malloc(10); >> free(a); >> free(a); /* Here you may free a block which is allocated again by another >> thread */ > > Yes, and glibc will warn if you do double free. I still think that it's > reasonable to accept that patch. I agree with Taras. pool handle will map to a specific buffer management hardware in the system and the value of odp_pool_t will be the same even if the same pool gets allocated to a different process. Regards, Bala > > Maxim. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
On 22 April 2015 at 13:29, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 22 April 2015 at 07:26, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> Good points. I agree it's better to leave this behavior undefined. >> > > If that is consensus I will send a patch for the docs to add. > > "Deleting an already deleted pool results in unspecified behavior." > "Attempting to delete an already deleted resource results in unspecified behavior." > > >> >> On Wed, Apr 22, 2015 at 5:44 AM, Taras Kondratiuk < >> taras.kondratiuk@linaro.org> wrote: >> >>> On 04/21/2015 10:14 PM, Mike Holmes wrote: >>> >>>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >>>> --- >>>> test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ >>>> 1 file changed, 25 insertions(+) >>>> >>>> diff --git a/test/validation/odp_pool.c b/test/validation/odp_pool.c >>>> index 1a518a0..c2f9a1b 100644 >>>> --- a/test/validation/odp_pool.c >>>> +++ b/test/validation/odp_pool.c >>>> @@ -11,6 +11,30 @@ static int pool_name_number = 1; >>>> static const int default_buffer_size = 1500; >>>> static const int default_buffer_num = 1000; >>>> >>>> +static void pool_double_destroy(void) >>>> +{ >>>> + odp_pool_param_t params = { >>>> + .buf = { >>>> + .size = default_buffer_size, >>>> + .align = ODP_CACHE_LINE_SIZE, >>>> + .num = default_buffer_num, >>>> + }, >>>> + .type = ODP_POOL_BUFFER, >>>> + }; >>>> + odp_pool_t pool; >>>> + char pool_name[ODP_POOL_NAME_LEN]; >>>> + >>>> + snprintf(pool_name, sizeof(pool_name), >>>> + "test_pool-%d", pool_name_number++); >>>> + >>>> + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, ¶ms); >>>> + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); >>>> + CU_ASSERT(odp_pool_to_u64(pool) != >>>> + odp_pool_to_u64(ODP_POOL_INVALID)); >>>> + CU_ASSERT(odp_pool_destroy(pool) == 0); >>>> + CU_ASSERT(odp_pool_destroy(pool) < 0); >>>> >>> >>> Is this an expected behavior? Do we have it documented somewhere? >>> I assume behavior should be undefined in this case. After pool is >>> destroyed its handle can't be used anymore. >>> >>> This test is single-threaded, but assume that there is another thread >>> which created a pool with the same odp_pool_t handle just between >>> two odp_pool_destroy(pool) calls. A second odp_pool_destroy() will >>> destroy a new pool and return 0. >>> If you demand this behavior, then you effectively force implementation >>> to use generation-tagged handles. >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp > >
diff --git a/test/validation/odp_pool.c b/test/validation/odp_pool.c index 1a518a0..c2f9a1b 100644 --- a/test/validation/odp_pool.c +++ b/test/validation/odp_pool.c @@ -11,6 +11,30 @@ static int pool_name_number = 1; static const int default_buffer_size = 1500; static const int default_buffer_num = 1000; +static void pool_double_destroy(void) +{ + odp_pool_param_t params = { + .buf = { + .size = default_buffer_size, + .align = ODP_CACHE_LINE_SIZE, + .num = default_buffer_num, + }, + .type = ODP_POOL_BUFFER, + }; + odp_pool_t pool; + char pool_name[ODP_POOL_NAME_LEN]; + + snprintf(pool_name, sizeof(pool_name), + "test_pool-%d", pool_name_number++); + + pool = odp_pool_create(pool_name, ODP_SHM_INVALID, ¶ms); + CU_ASSERT_FATAL(pool != ODP_POOL_INVALID); + CU_ASSERT(odp_pool_to_u64(pool) != + odp_pool_to_u64(ODP_POOL_INVALID)); + CU_ASSERT(odp_pool_destroy(pool) == 0); + CU_ASSERT(odp_pool_destroy(pool) < 0); +} + static void pool_create_destroy(odp_pool_param_t *params) { odp_pool_t pool; @@ -133,6 +157,7 @@ CU_TestInfo pool_tests[] = { _CU_TEST_INFO(pool_create_destroy_timeout), _CU_TEST_INFO(pool_create_destroy_buffer_shm), _CU_TEST_INFO(pool_lookup_info_print), + _CU_TEST_INFO(pool_double_destroy), CU_TEST_INFO_NULL, };
Signed-off-by: Mike Holmes <mike.holmes@linaro.org> --- test/validation/odp_pool.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)