diff mbox

[BUG] wrong thread_id entry in thread_tbl[hereee!!!] for few odp applications

Message ID 20140730132639.GA27986@localhost
State New
Headers show

Commit Message

Stuart Haslam July 30, 2014, 1:26 p.m. UTC
On Wed, Jul 30, 2014 at 12:10:24PM +0100, Santosh Shukla wrote:
> Hi,
> 
> I noticed that few example odp application (l2fwd, generator, pktio
> and odp_timer_ping) likely to store wrong thread_id because of the way
> they were using odp_linux_pthread_create() api.
> 
> Example:
> 
> Refer  file example/l2fwd/odp_l2fwd.c, odp_generator.c and odp_time_ping.c
> 
> code snippet look like this
> 
>         for (i = 0; i < num_workers; ++i) {
> 
>                 ...............
>                 .............
>                 odp_linux_pthread_create(thread_tbl, 1, core, thr_run_func,
>                                          &gbl_args->thread[i]);
>         }
> The above will overwrite thread_tbl[0] for each iteration.
> 
> So if you print thread_tbl[] array, you will find only thread_tbl[0]
> with valid value and rest with zero.
> 
> Problem is inner for loop of odp_linux_pthread_create() refills entry
> in thread_tbl starting from index 0 till max number (arg3 above)
> 
> In my isolation case as well other cases like generator, l2fwd app,
> don't want odp_linux_pthread_create to fill thread_tbl entry starting
> from 0 everytime.

This looks like a bug in the application rather than in the API or the
implementation. Why not just pass the correct address from the
application?

As far as I can tell, this (completely untested) patch would have the
same effect as yours;


--
Stuart.

Comments

Santosh Shukla July 30, 2014, 1:45 p.m. UTC | #1
On 30 July 2014 18:56, Stuart Haslam <stuart.haslam@arm.com> wrote:
> On Wed, Jul 30, 2014 at 12:10:24PM +0100, Santosh Shukla wrote:
>> Hi,
>>
>> I noticed that few example odp application (l2fwd, generator, pktio
>> and odp_timer_ping) likely to store wrong thread_id because of the way
>> they were using odp_linux_pthread_create() api.
>>
>> Example:
>>
>> Refer  file example/l2fwd/odp_l2fwd.c, odp_generator.c and odp_time_ping.c
>>
>> code snippet look like this
>>
>>         for (i = 0; i < num_workers; ++i) {
>>
>>                 ...............
>>                 .............
>>                 odp_linux_pthread_create(thread_tbl, 1, core, thr_run_func,
>>                                          &gbl_args->thread[i]);
>>         }
>> The above will overwrite thread_tbl[0] for each iteration.
>>
>> So if you print thread_tbl[] array, you will find only thread_tbl[0]
>> with valid value and rest with zero.
>>
>> Problem is inner for loop of odp_linux_pthread_create() refills entry
>> in thread_tbl starting from index 0 till max number (arg3 above)
>>
>> In my isolation case as well other cases like generator, l2fwd app,
>> don't want odp_linux_pthread_create to fill thread_tbl entry starting
>> from 0 everytime.
>
> This looks like a bug in the application rather than in the API or the
> implementation. Why not just pass the correct address from the
> application?
>
> As far as I can tell, this (completely untested) patch would have the
> same effect as yours;
>
> diff --git a/test/api_test/odp_timer_ping.c b/test/api_test/odp_timer_ping.c
> index cd67e0d..88650d6 100644
> --- a/test/api_test/odp_timer_ping.c
> +++ b/test/api_test/odp_timer_ping.c
> @@ -367,7 +367,7 @@ int main(int argc ODP_UNUSED, char *argv[] ODP_UNUSED)
>                         run_thread = send_ping;
>
>                 /* Create and launch worker threads */
> -               odp_linux_pthread_create(thread_tbl, 1, i,
> +               odp_linux_pthread_create(&thread_tbl[i], 1, i,
>                                          run_thread, (pthrd_arg *)&pingarg);
>         }

Yes and Its a better fix.

Ignore other patches sent on this topic. I will send series of patch
set where we want to fix this problem happening on other odp example
application.

Thanks.

>
>
> --
> Stuart.
>
Taras Kondratiuk Aug. 3, 2014, 2:55 p.m. UTC | #2
On 07/30/2014 04:45 PM, Santosh Shukla wrote:
> On 30 July 2014 18:56, Stuart Haslam <stuart.haslam@arm.com> wrote:
>> As far as I can tell, this (completely untested) patch would have the
>> same effect as yours;
>>
>> diff --git a/test/api_test/odp_timer_ping.c b/test/api_test/odp_timer_ping.c
>> index cd67e0d..88650d6 100644
>> --- a/test/api_test/odp_timer_ping.c
>> +++ b/test/api_test/odp_timer_ping.c
>> @@ -367,7 +367,7 @@ int main(int argc ODP_UNUSED, char *argv[] ODP_UNUSED)
>>                          run_thread = send_ping;
>>
>>                  /* Create and launch worker threads */
>> -               odp_linux_pthread_create(thread_tbl, 1, i,
>> +               odp_linux_pthread_create(&thread_tbl[i], 1, i,
>>                                           run_thread, (pthrd_arg *)&pingarg);
>>          }
>
> Yes and Its a better fix.
>
> Ignore other patches sent on this topic. I will send series of patch
> set where we want to fix this problem happening on other odp example
> application.

Hi Santosh,

Will you send the fix?
Santosh Shukla Aug. 3, 2014, 4:51 p.m. UTC | #3
Yup, tomorrow.

On 3 August 2014 20:25, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote:
> On 07/30/2014 04:45 PM, Santosh Shukla wrote:
>>
>> On 30 July 2014 18:56, Stuart Haslam <stuart.haslam@arm.com> wrote:
>>>
>>> As far as I can tell, this (completely untested) patch would have the
>>> same effect as yours;
>>>
>>> diff --git a/test/api_test/odp_timer_ping.c
>>> b/test/api_test/odp_timer_ping.c
>>> index cd67e0d..88650d6 100644
>>> --- a/test/api_test/odp_timer_ping.c
>>> +++ b/test/api_test/odp_timer_ping.c
>>> @@ -367,7 +367,7 @@ int main(int argc ODP_UNUSED, char *argv[]
>>> ODP_UNUSED)
>>>                          run_thread = send_ping;
>>>
>>>                  /* Create and launch worker threads */
>>> -               odp_linux_pthread_create(thread_tbl, 1, i,
>>> +               odp_linux_pthread_create(&thread_tbl[i], 1, i,
>>>                                           run_thread, (pthrd_arg
>>> *)&pingarg);
>>>          }
>>
>>
>> Yes and Its a better fix.
>>
>> Ignore other patches sent on this topic. I will send series of patch
>> set where we want to fix this problem happening on other odp example
>> application.
>
>
> Hi Santosh,
>
> Will you send the fix?
diff mbox

Patch

diff --git a/test/api_test/odp_timer_ping.c b/test/api_test/odp_timer_ping.c
index cd67e0d..88650d6 100644
--- a/test/api_test/odp_timer_ping.c
+++ b/test/api_test/odp_timer_ping.c
@@ -367,7 +367,7 @@  int main(int argc ODP_UNUSED, char *argv[] ODP_UNUSED)
 			run_thread = send_ping;
 
 		/* Create and launch worker threads */
-		odp_linux_pthread_create(thread_tbl, 1, i,
+		odp_linux_pthread_create(&thread_tbl[i], 1, i,
 					 run_thread, (pthrd_arg *)&pingarg);
 	}