[Xenomai] [PATCH 0 of 2] Xenomai-forge thread_obj: unset __THREAD_S_SAFE when not needed

[Xenomai] [PATCH 0 of 2] Xenomai-forge thread_obj: unset __THREAD_S_SAFE when not needed

[Kim De Mey]

Hello list,

I believe that there is a problem in xenomai-forge when you create tasks
from within another task and the newly created tasks have a lower 
priority than the priority of the task where you created them from.

In the threadobj_start() function the thobj->status is set to 
__THREAD_S_STARTED|__THREAD_S_SAFE. After this there is a check if the 
thobj priority is lower or equal to the current thobj priority. If that 
is the case, it will return there. If it is not the case, 
synchronization needs to be done.

As far as I understand the code, however, I think that the 
__THREAD_S_SAFE is only set there because you don't want the 
finalize_thread to already clean up the thobj in case you are still 
waiting for the synchronization in the threadobj_start(). Correct?
If so then this is where I think there is a problem. In case that you 
don't need to wait for the synchronization and return already before it,
the __THREAD_S_SAFE is set but never unset. This has as result that in 
the finalize_thread() (on deletion) the destroy_thread() function will 
not be called, which has as consequence that proper clean up is not 
done. The first patch should fix this.

I came to this conclusion after investigating a segmentation fault on a 
test application that creates, starts and deletes tasks in a loop. I can 
reproduce the issue with the test code at the end of this post. In the 
code there is a main_task, with priority 90, from where the test_tasks 
are created, started and deleted. The priority of the test_tasks is 50. 

The result is that a lot of tasks created are not properly deleted. 
This causes (among maybe other things) a LOT of file descriptors staying 
open (4 per task I think). These file descriptors are created in the 
notifier_init() function by doing two pipe() syscalls. If this fails, an 
error is returned by notifier_init(). However the function 
threadobj_setup_corespec() does not check this. At a certain point, no 
more file descriptors are allowed on the process (max is 1024 here) and 
the pipe() call will fail. If this now happens on the creation of a task
with a higher priority than 90 (which is the last step in the test 
code), then threadobj will be properly deleted or attempt this at least. 
In the deletion, the notifier_destroy() function will be called. This 
will give a segmentation fault when trying to do pvlist_remove_init() as 
the notifier_init() function never ran completely in the first place 
(because of error on pipe()).   
To make sure that the fail on the pipe() call is seen, I created the 
second patch. There could be a better thing to do than sending a panic 
but at least you notice that there is a problem.

Regards,
Kim

 lib/copperplate/notifier.c  |  4 ++--
 lib/copperplate/threadobj.c |  4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)


test code snippet:

static void test_task(u_long a,u_long b,u_long c,u_long d)
{
  while (1)
    tm_wkafter(1000);
}

static void main_task(u_long a,u_long b,u_long c,u_long d)
{
  u_long tid,args[4] = {0,0,0,0};
  int i;
  char name[32];
  
  for (i=0;i<256;i++) {
    printf("counter i: %d\n", i);
    sprintf(name, "TEST_%d", i);
    t_create (name,50,0,0,0,&tid);
    t_start(tid,T_PREEMPT,test_task,args);
    t_delete(tid);
  }
  t_create ("FAIL",95,0,0,0,&tid);
  t_start(tid,T_PREEMPT,test_task,args);
  t_delete(tid);

  while (1) tm_wkafter(1000);
}

int main(int argc, char * const argv[])
{
  u_long tid,args[4] = {0,0,0,0};

  psos_long_names = 1;

  mlockall(MCL_CURRENT | MCL_FUTURE);
  copperplate_init(&argc,&argv);

  t_create("MAIN",90,0,0,0,&tid);
  t_start(tid,0,main_task, args);
  while (1) tm_wkafter(1000);
  return 0;
}

[Xenomai] [PATCH 1 of 2] Xenomai-forge thread_obj: unset __THREAD_S_SAFE when not needed

[Kim De Mey]

unset __THREAD_S_SAFE for thobj->status when the priority of the
task is lower than then priority of the current task.

Signed-off-by: Kim De Mey <kim.demey@gmail.com>

---

 lib/copperplate/threadobj.c |  4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lib/copperplate/threadobj.c b/lib/copperplate/threadobj.c
--- a/lib/copperplate/threadobj.c
+++ b/lib/copperplate/threadobj.c
@@ -929,8 +929,10 @@ int threadobj_start(struct threadobj *th
 	thobj->status |= __THREAD_S_STARTED|__THREAD_S_SAFE;
 	__RT(pthread_cond_signal(&thobj->barrier));
 
-	if (current && thobj->priority <= current->priority)
+	if (current && thobj->priority <= current->priority) {
+		thobj->status &= ~__THREAD_S_SAFE;
 		return 0;
+	}
 
 	/*
 	 * Caller needs synchronization with the thread being started,

[Xenomai] [PATCH 2 of 2] Xenomai-forge notifier_init: panic on failed pipe

[Kim De Mey]

Panic if one of the pipe() system calls fails. To indicate that there
is an underlying problem.

Signed-off-by: Kim De Mey <kim.demey@gmail.com>

---

 lib/copperplate/notifier.c |  4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/copperplate/notifier.c b/lib/copperplate/notifier.c
--- a/lib/copperplate/notifier.c
+++ b/lib/copperplate/notifier.c
@@ -144,12 +144,12 @@ int notifier_init(struct notifier *nf,
 	int fd;
 
 	if (pipe(nf->psfd) < 0)
-		return __bt(-errno);
+		panic("failed to create file descriptors");
 
 	if (pipe(nf->pwfd) < 0) {
 		__STD(close(nf->psfd[0]));
 		__STD(close(nf->psfd[1]));
-		return __bt(-errno);
+		panic("failed to create file descriptors");
 	}
 
 	nf->callback = callback;

Re: [Xenomai] [PATCH 2 of 2] Xenomai-forge notifier_init: panic on failed pipe

[Philippe Gerum]

On 10/18/2013 02:50 PM, Kim De Mey wrote:
> Panic if one of the pipe() system calls fails. To indicate that there
> is an underlying problem.
>
> Signed-off-by: Kim De Mey <kim.demey@gmail.com>
>
> ---
>
>   lib/copperplate/notifier.c |  4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/copperplate/notifier.c b/lib/copperplate/notifier.c
> --- a/lib/copperplate/notifier.c
> +++ b/lib/copperplate/notifier.c
> @@ -144,12 +144,12 @@ int notifier_init(struct notifier *nf,
>   	int fd;
>
>   	if (pipe(nf->psfd) < 0)
> -		return __bt(-errno);
> +		panic("failed to create file descriptors");
>
>   	if (pipe(nf->pwfd) < 0) {
>   		__STD(close(nf->psfd[0]));
>   		__STD(close(nf->psfd[1]));
> -		return __bt(-errno);
> +		panic("failed to create file descriptors");
>   	}
>
>   	nf->callback = callback;
>

Nack. This routine returns a status code, the caller should check it.

-- 
Philippe.

Re: [Xenomai] [PATCH 2 of 2] Xenomai-forge notifier_init: panic on failed pipe

[Philippe Gerum]

On 10/18/2013 02:54 PM, Philippe Gerum wrote:
> On 10/18/2013 02:50 PM, Kim De Mey wrote:
>> Panic if one of the pipe() system calls fails. To indicate that there
>> is an underlying problem.
>>
>> Signed-off-by: Kim De Mey <kim.demey@gmail.com>
>>
>> ---
>>
>>   lib/copperplate/notifier.c |  4 ++--
>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/copperplate/notifier.c b/lib/copperplate/notifier.c
>> --- a/lib/copperplate/notifier.c
>> +++ b/lib/copperplate/notifier.c
>> @@ -144,12 +144,12 @@ int notifier_init(struct notifier *nf,
>>       int fd;
>>
>>       if (pipe(nf->psfd) < 0)
>> -        return __bt(-errno);
>> +        panic("failed to create file descriptors");
>>
>>       if (pipe(nf->pwfd) < 0) {
>>           __STD(close(nf->psfd[0]));
>>           __STD(close(nf->psfd[1]));
>> -        return __bt(-errno);
>> +        panic("failed to create file descriptors");
>>       }
>>
>>       nf->callback = callback;
>>
>
> Nack. This routine returns a status code, the caller should check it.
>

warning() would be acceptable though, for the same purpose, while still 
propagating the error code instead of pulling the break arbitrarily.

-- 
Philippe.

Re: [Xenomai] [PATCH 0 of 2] Xenomai-forge thread_obj: unset __THREAD_S_SAFE when not needed

[Philippe Gerum]

On 10/18/2013 02:50 PM, Kim De Mey wrote:
> Hello list,
>
> I believe that there is a problem in xenomai-forge when you create tasks
> from within another task and the newly created tasks have a lower
> priority than the priority of the task where you created them from.
>
> In the threadobj_start() function the thobj->status is set to
> __THREAD_S_STARTED|__THREAD_S_SAFE. After this there is a check if the
> thobj priority is lower or equal to the current thobj priority. If that
> is the case, it will return there. If it is not the case,
> synchronization needs to be done.
>
> As far as I understand the code, however, I think that the
> __THREAD_S_SAFE is only set there because you don't want the
> finalize_thread to already clean up the thobj in case you are still
> waiting for the synchronization in the threadobj_start(). Correct?
> If so then this is where I think there is a problem. In case that you
> don't need to wait for the synchronization and return already before it,
> the __THREAD_S_SAFE is set but never unset. This has as result that in
> the finalize_thread() (on deletion) the destroy_thread() function will
> not be called, which has as consequence that proper clean up is not
> done. The first patch should fix this.
>

Ack. Thanks for spotting this. The patch below should fix it the same 
way, avoiding the additional bit toggling.

diff --git a/lib/copperplate/threadobj.c b/lib/copperplate/threadobj.c
index 8aff287..a283b22 100644
--- a/lib/copperplate/threadobj.c
+++ b/lib/copperplate/threadobj.c
@@ -926,7 +926,7 @@ int threadobj_start(struct threadobj *thobj)	/* 
thobj->lock held. */
  	if (thobj->status & __THREAD_S_STARTED)
  		return 0;

-	thobj->status |= __THREAD_S_STARTED|__THREAD_S_SAFE;
+	thobj->status |= __THREAD_S_STARTED;
  	__RT(pthread_cond_signal(&thobj->barrier));

  	if (current && thobj->priority <= current->priority)
@@ -943,6 +943,7 @@ int threadobj_start(struct threadobj *thobj)	/* 
thobj->lock held. */
  	 */
  	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate);

+	thobj->status |= __THREAD_S_SAFE;
  	wait_on_barrier(thobj, __THREAD_S_ACTIVE);
  	thobj->status &= ~__THREAD_S_SAFE;

-- 
Philippe.

Re: [Xenomai] [PATCH 2 of 2] Xenomai-forge notifier_init: panic on failed pipe

[Kim De Mey]

2013/10/18 Philippe Gerum <rpm@xenomai.org>

> On 10/18/2013 02:54 PM, Philippe Gerum wrote:
>
>> On 10/18/2013 02:50 PM, Kim De Mey wrote:
>>
>>> Panic if one of the pipe() system calls fails. To indicate that there
>>> is an underlying problem.
>>>
>>> Signed-off-by: Kim De Mey <kim.demey@gmail.com>
>>>
>>> ---
>>>
>>>   lib/copperplate/notifier.c |  4 ++--
>>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/copperplate/notifier.c b/lib/copperplate/notifier.c
>>> --- a/lib/copperplate/notifier.c
>>> +++ b/lib/copperplate/notifier.c
>>> @@ -144,12 +144,12 @@ int notifier_init(struct notifier *nf,
>>>       int fd;
>>>
>>>       if (pipe(nf->psfd) < 0)
>>> -        return __bt(-errno);
>>> +        panic("failed to create file descriptors");
>>>
>>>       if (pipe(nf->pwfd) < 0) {
>>>           __STD(close(nf->psfd[0]));
>>>           __STD(close(nf->psfd[1]));
>>> -        return __bt(-errno);
>>> +        panic("failed to create file descriptors");
>>>       }
>>>
>>>       nf->callback = callback;
>>>
>>>
>> Nack. This routine returns a status code, the caller should check it.
>>
>>
> warning() would be acceptable though, for the same purpose, while still
> propagating the error code instead of pulling the break arbitrarily.


Agreed it would be much better if the caller checked it. But it is
currently not checked in threadobj_setup_corespec(). And as I am unsure of
what to do with it in that function I did this patch.

Although a warning is probably enough indeed in case nothing is done with
the error in threadobj_setup_corespec().


>
> --
> Philippe.
>

Re: [Xenomai] [PATCH 2 of 2] Xenomai-forge notifier_init: panic on failed pipe

[Philippe Gerum]

On 10/18/2013 03:21 PM, Kim De Mey wrote:
>
>
>
> 2013/10/18 Philippe Gerum <rpm@xenomai.org <mailto:rpm@xenomai.org>>
>
>     On 10/18/2013 02:54 PM, Philippe Gerum wrote:
>
>         On 10/18/2013 02:50 PM, Kim De Mey wrote:
>
>             Panic if one of the pipe() system calls fails. To indicate
>             that there
>             is an underlying problem.
>
>             Signed-off-by: Kim De Mey <kim.demey@gmail.com
>             <mailto:kim.demey@gmail.com>>
>
>             ---
>
>                lib/copperplate/notifier.c |  4 ++--
>                1 files changed, 2 insertions(+), 2 deletions(-)
>
>             diff --git a/lib/copperplate/notifier.c
>             b/lib/copperplate/notifier.c
>             --- a/lib/copperplate/notifier.c
>             +++ b/lib/copperplate/notifier.c
>             @@ -144,12 +144,12 @@ int notifier_init(struct notifier *nf,
>                    int fd;
>
>                    if (pipe(nf->psfd) < 0)
>             -        return __bt(-errno);
>             +        panic("failed to create file descriptors");
>
>                    if (pipe(nf->pwfd) < 0) {
>                        __STD(close(nf->psfd[0]));
>                        __STD(close(nf->psfd[1]));
>             -        return __bt(-errno);
>             +        panic("failed to create file descriptors");
>                    }
>
>                    nf->callback = callback;
>
>
>         Nack. This routine returns a status code, the caller should
>         check it.
>
>
>     warning() would be acceptable though, for the same purpose, while
>     still propagating the error code instead of pulling the break
>     arbitrarily.
>
>
> Agreed it would be much better if the caller checked it. But it is
> currently not checked in threadobj_setup_corespec(). And as I am unsure
> of what to do with it in that function I did this patch.
>
> Although a warning is probably enough indeed in case nothing is done
> with the error in threadobj_setup_corespec().
>

We can't rely on this assumption, since this may evolve, including for 
Mercury. This said, assert() on the status code of notifier_init() 
within that routine would make sense, so that it triggers in debug mode 
when the app starts.


-- 
Philippe.

Re: [Xenomai] [PATCH 2 of 2] Xenomai-forge notifier_init: panic on failed pipe

[Kim De Mey]

2013/10/18 Philippe Gerum <rpm@xenomai.org>

> On 10/18/2013 03:21 PM, Kim De Mey wrote:
>
>>
>>
>>
>> 2013/10/18 Philippe Gerum <rpm@xenomai.org <mailto:rpm@xenomai.org>>
>>
>>
>>     On 10/18/2013 02:54 PM, Philippe Gerum wrote:
>>
>>         On 10/18/2013 02:50 PM, Kim De Mey wrote:
>>
>>             Panic if one of the pipe() system calls fails. To indicate
>>             that there
>>             is an underlying problem.
>>
>>             Signed-off-by: Kim De Mey <kim.demey@gmail.com
>>             <mailto:kim.demey@gmail.com>>
>>
>>
>>             ---
>>
>>                lib/copperplate/notifier.c |  4 ++--
>>                1 files changed, 2 insertions(+), 2 deletions(-)
>>
>>             diff --git a/lib/copperplate/notifier.c
>>             b/lib/copperplate/notifier.c
>>             --- a/lib/copperplate/notifier.c
>>             +++ b/lib/copperplate/notifier.c
>>             @@ -144,12 +144,12 @@ int notifier_init(struct notifier *nf,
>>                    int fd;
>>
>>                    if (pipe(nf->psfd) < 0)
>>             -        return __bt(-errno);
>>             +        panic("failed to create file descriptors");
>>
>>                    if (pipe(nf->pwfd) < 0) {
>>                        __STD(close(nf->psfd[0]));
>>                        __STD(close(nf->psfd[1]));
>>             -        return __bt(-errno);
>>             +        panic("failed to create file descriptors");
>>                    }
>>
>>                    nf->callback = callback;
>>
>>
>>         Nack. This routine returns a status code, the caller should
>>         check it.
>>
>>
>>     warning() would be acceptable though, for the same purpose, while
>>     still propagating the error code instead of pulling the break
>>     arbitrarily.
>>
>>
>> Agreed it would be much better if the caller checked it. But it is
>> currently not checked in threadobj_setup_corespec(). And as I am unsure
>> of what to do with it in that function I did this patch.
>>
>> Although a warning is probably enough indeed in case nothing is done
>> with the error in threadobj_setup_corespec().
>>
>>
> We can't rely on this assumption, since this may evolve, including for
> Mercury. This said, assert() on the status code of notifier_init() within
> that routine would make sense, so that it triggers in debug mode when the
> app starts.
>
> So if I understand it right, you would add an assert() on the return value
of notifier_init() and also the warning() in notifier_init().
Or not the warning?

>
> --
> Philippe.
>

Re: [Xenomai] [PATCH 0 of 2] Xenomai-forge thread_obj: unset __THREAD_S_SAFE when not needed

[Kim De Mey]

2013/10/18 Philippe Gerum <rpm@xenomai.org>

> On 10/18/2013 02:50 PM, Kim De Mey wrote:
>
>> Hello list,
>>
>> I believe that there is a problem in xenomai-forge when you create tasks
>> from within another task and the newly created tasks have a lower
>> priority than the priority of the task where you created them from.
>>
>> In the threadobj_start() function the thobj->status is set to
>> __THREAD_S_STARTED|__THREAD_S_**SAFE. After this there is a check if the
>> thobj priority is lower or equal to the current thobj priority. If that
>> is the case, it will return there. If it is not the case,
>> synchronization needs to be done.
>>
>> As far as I understand the code, however, I think that the
>> __THREAD_S_SAFE is only set there because you don't want the
>> finalize_thread to already clean up the thobj in case you are still
>> waiting for the synchronization in the threadobj_start(). Correct?
>> If so then this is where I think there is a problem. In case that you
>> don't need to wait for the synchronization and return already before it,
>> the __THREAD_S_SAFE is set but never unset. This has as result that in
>> the finalize_thread() (on deletion) the destroy_thread() function will
>> not be called, which has as consequence that proper clean up is not
>> done. The first patch should fix this.
>>
>>
> Ack. Thanks for spotting this. The patch below should fix it the same way,
> avoiding the additional bit toggling.
>

Tested your patch and it works also.


>
> diff --git a/lib/copperplate/threadobj.c b/lib/copperplate/threadobj.c
> index 8aff287..a283b22 100644
> --- a/lib/copperplate/threadobj.c
> +++ b/lib/copperplate/threadobj.c
> @@ -926,7 +926,7 @@ int threadobj_start(struct threadobj *thobj)        /*
> thobj->lock held. */
>         if (thobj->status & __THREAD_S_STARTED)
>                 return 0;
>
> -       thobj->status |= __THREAD_S_STARTED|__THREAD_S_**SAFE;
> +       thobj->status |= __THREAD_S_STARTED;
>         __RT(pthread_cond_signal(&**thobj->barrier));
>
>
>         if (current && thobj->priority <= current->priority)
> @@ -943,6 +943,7 @@ int threadobj_start(struct threadobj *thobj)        /*
> thobj->lock held. */
>          */
>         pthread_setcancelstate(**PTHREAD_CANCEL_DISABLE, &oldstate);
>
> +       thobj->status |= __THREAD_S_SAFE;
>         wait_on_barrier(thobj, __THREAD_S_ACTIVE);
>         thobj->status &= ~__THREAD_S_SAFE;
>
> --
> Philippe.
>

Re: [Xenomai] [PATCH 0 of 2] Xenomai-forge thread_obj: unset __THREAD_S_SAFE when not needed

[Ronny Meeus]

>
>
> test code snippet:
>
> static void test_task(u_long a,u_long b,u_long c,u_long d)
> {
>   while (1)
>     tm_wkafter(1000);
> }
>
> static void main_task(u_long a,u_long b,u_long c,u_long d)
> {
>   u_long tid,args[4] = {0,0,0,0};
>   int i;
>   char name[32];
>
>   for (i=0;i<256;i++) {
>     printf("counter i: %d\n", i);
>     sprintf(name, "TEST_%d", i);
>     t_create (name,50,0,0,0,&tid);
>     t_start(tid,T_PREEMPT,test_task,args);
>     t_delete(tid);
>   }
>   t_create ("FAIL",95,0,0,0,&tid);
>   t_start(tid,T_PREEMPT,test_task,args);
>   t_delete(tid);
>
>   while (1) tm_wkafter(1000);
> }
>
> int main(int argc, char * const argv[])
> {
>   u_long tid,args[4] = {0,0,0,0};
>
>   psos_long_names = 1;
>
>   mlockall(MCL_CURRENT | MCL_FUTURE);
>   copperplate_init(&argc,&argv);
>
>   t_create("MAIN",90,0,0,0,&tid);
>   t_start(tid,0,main_task, args);
>   while (1) tm_wkafter(1000);
>   return 0;
> }
>
>
Philippe,

I think it would be good to add this test to the testsuite of xenomai since
we discovered a bug in an earlier version of Xenomai forge already with
exactly this test.

Regards,
Ronny

Re: [Xenomai] [PATCH 0 of 2] Xenomai-forge thread_obj: unset __THREAD_S_SAFE when not needed

[Philippe Gerum]

On 10/19/2013 08:42 PM, Ronny Meeus wrote:
>
>     test code snippet:
>
>     static void test_task(u_long a,u_long b,u_long c,u_long d)
>     {
>        while (1)
>          tm_wkafter(1000);
>     }
>
>     static void main_task(u_long a,u_long b,u_long c,u_long d)
>     {
>        u_long tid,args[4] = {0,0,0,0};
>        int i;
>        char name[32];
>
>        for (i=0;i<256;i++) {
>          printf("counter i: %d\n", i);
>          sprintf(name, "TEST_%d", i);
>          t_create (name,50,0,0,0,&tid);
>          t_start(tid,T_PREEMPT,test_task,args);
>          t_delete(tid);
>        }
>        t_create ("FAIL",95,0,0,0,&tid);
>        t_start(tid,T_PREEMPT,test_task,args);
>        t_delete(tid);
>
>        while (1) tm_wkafter(1000);
>     }
>
>     int main(int argc, char * const argv[])
>     {
>        u_long tid,args[4] = {0,0,0,0};
>
>        psos_long_names = 1;
>
>        mlockall(MCL_CURRENT | MCL_FUTURE);
>        copperplate_init(&argc,&argv);
>
>        t_create("MAIN",90,0,0,0,&tid);
>        t_start(tid,0,main_task, args);
>        while (1) tm_wkafter(1000);
>        return 0;
>     }
>
> Philippe,
> I think it would be good to add this test to the testsuite of xenomai
> since we discovered a bug in an earlier version of Xenomai forge already
> with exactly this test.

Makes sense. Please push a patch, I'll merge it.

Thanks,

-- 
Philippe.