validation: odp_pool: add double destroy

Message ID 1429643683-9050-1-git-send-email-mike.holmes@linaro.org
State New
Headers show

Commit Message

Mike Holmes April 21, 2015, 7:14 p.m.
Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
---
 test/validation/odp_pool.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Bill Fischofer April 21, 2015, 7:54 p.m. | #1
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, &params);
> +       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
>
Taras Kondratiuk April 22, 2015, 10:44 a.m. | #2
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, &params);
> +	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.
Bill Fischofer April 22, 2015, 11:26 a.m. | #3
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, &params);
>> +       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 April 22, 2015, 11:26 a.m. | #4
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, &params);
>> +       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 April 22, 2015, 11:29 a.m. | #5
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, &params);
>>> +       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
>>
>
>
Bill Fischofer April 22, 2015, 11:33 a.m. | #6
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, &params);
>>>> +       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
>
>
>
Taras Kondratiuk April 22, 2015, 11:47 a.m. | #7
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."
Maxim Uvarov April 22, 2015, 12:13 p.m. | #8
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,
>         &params);
>         +       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
Bill Fischofer April 22, 2015, 12:53 p.m. | #9
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,
>>         &params);
>>         +       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
>
Mike Holmes April 22, 2015, 1 p.m. | #10
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
>
>
Savolainen, Petri (Nokia - FI/Espoo) April 22, 2015, 1:04 p.m. | #11
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
Maxim Uvarov April 22, 2015, 2:13 p.m. | #12
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,
>                 &params);
>                 +       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
>
>
Ciprian Barbu April 22, 2015, 4:06 p.m. | #13
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,
>>                 &params);
>>                 +       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
Maxim Uvarov April 22, 2015, 5:54 p.m. | #14
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,
>>>                  &params);
>>>                  +       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
Taras Kondratiuk April 23, 2015, 10:09 a.m. | #15
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 */
Maxim Uvarov April 23, 2015, 11:26 a.m. | #16
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.
Balasubramanian Manoharan April 23, 2015, 11:55 a.m. | #17
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
Ola Liljedahl April 23, 2015, 3:44 p.m. | #18
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, &params);
>>>> +       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
>
>

Patch

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, &params);
+	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,
 };