[Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Gilles Chanteperdrix]

When exiting a process, rtdk buffers are not flushed. Fix that by
introducing a destructor for the library which flushes the buffers.

When forking, if the main thread has an rt_printf buffer, it is reused by
the child, which is a good thing, however, it is not emptied, so could cause
the same message to be printed twice. Fix that by emptying the main thread
buffer upon fork.

When spawning the rt_print thread, it uses a stack size of
PTHREAD_STACK_MIN, which may be too small for printf to work reliably on
some architectures, set the stack size to the
empirically-known-to-be-working size of... 32Kb.
---
 src/rtdk/init.c     |    5 +++++
 src/rtdk/internal.h |    1 +
 src/rtdk/rt_print.c |   21 ++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/src/rtdk/init.c b/src/rtdk/init.c
index 2084c73..b864175 100644
--- a/src/rtdk/init.c
+++ b/src/rtdk/init.c
@@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
 {
 	__rt_print_init();
 }
+
+static __attribute__ ((destructor)) void __exit_rtdk(void)
+{
+	__rt_print_exit();
+}
diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
index bd15b2d..345d4fa 100644
--- a/src/rtdk/internal.h
+++ b/src/rtdk/internal.h
@@ -23,6 +23,7 @@
 #include <sys/time.h>
 
 void __rt_print_init(void);
+void __rt_print_exit(void);
 
 void __real_free(void *ptr);
 void *__real_malloc(size_t size);
diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
index c6b5c55..dae7d7b 100644
--- a/src/rtdk/rt_print.c
+++ b/src/rtdk/rt_print.c
@@ -455,7 +455,7 @@ static void spawn_printer_thread(void)
 	pthread_attr_t thattr;
 
 	pthread_attr_init(&thattr);
-	pthread_attr_setstacksize(&thattr, PTHREAD_STACK_MIN);
+	pthread_attr_setstacksize(&thattr, 32768);
 	pthread_create(&printer_thread, &thattr, printer_loop, NULL);
 }
 
@@ -464,6 +464,15 @@ static void forked_child_init(void)
 	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
 	struct print_buffer **pbuffer = &first_buffer;
 
+	if (my_buffer) {
+		/* Any content of my_buffer should be printed by our parent,
+		   not us. */
+		memset(my_buffer->ring, 0, my_buffer->size);
+
+		my_buffer->read_pos  = 0;
+		my_buffer->write_pos = 0;
+	}
+
 	/* re-init to avoid finding it locked by some parent thread */
 	pthread_mutex_init(&buffer_lock, NULL);
 
@@ -518,3 +527,13 @@ void __rt_print_init(void)
 	spawn_printer_thread();
 	pthread_atfork(NULL, NULL, forked_child_init);
 }
+
+void __rt_print_exit(void)
+{
+	if (buffers) {
+		/* Flush the buffers. Do not call print_buffers here
+		 * since we do not know if our stack is big enough. */
+		nanosleep(&print_period, NULL);
+		nanosleep(&print_period, NULL);
+	}
+}
-- 
1.5.6.5

Re: [Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]

Gilles Chanteperdrix wrote:
> When exiting a process, rtdk buffers are not flushed. Fix that by
> introducing a destructor for the library which flushes the buffers.
> 
> When forking, if the main thread has an rt_printf buffer, it is reused by
> the child, which is a good thing, however, it is not emptied, so could cause
> the same message to be printed twice. Fix that by emptying the main thread
> buffer upon fork.
> 
> When spawning the rt_print thread, it uses a stack size of
> PTHREAD_STACK_MIN, which may be too small for printf to work reliably on
> some architectures, set the stack size to the
> empirically-known-to-be-working size of... 32Kb.
> ---
>  src/rtdk/init.c     |    5 +++++
>  src/rtdk/internal.h |    1 +
>  src/rtdk/rt_print.c |   21 ++++++++++++++++++++-
>  3 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/src/rtdk/init.c b/src/rtdk/init.c
> index 2084c73..b864175 100644
> --- a/src/rtdk/init.c
> +++ b/src/rtdk/init.c
> @@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
>  {
>  	__rt_print_init();
>  }
> +
> +static __attribute__ ((destructor)) void __exit_rtdk(void)
> +{
> +	__rt_print_exit();
> +}
> diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
> index bd15b2d..345d4fa 100644
> --- a/src/rtdk/internal.h
> +++ b/src/rtdk/internal.h
> @@ -23,6 +23,7 @@
>  #include <sys/time.h>
>  
>  void __rt_print_init(void);
> +void __rt_print_exit(void);
>  
>  void __real_free(void *ptr);
>  void *__real_malloc(size_t size);
> diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
> index c6b5c55..dae7d7b 100644
> --- a/src/rtdk/rt_print.c
> +++ b/src/rtdk/rt_print.c
> @@ -455,7 +455,7 @@ static void spawn_printer_thread(void)
>  	pthread_attr_t thattr;
>  
>  	pthread_attr_init(&thattr);
> -	pthread_attr_setstacksize(&thattr, PTHREAD_STACK_MIN);
> +	pthread_attr_setstacksize(&thattr, 32768);

Mmh... maybe rather $something +PTHREAD_STACK_MIN?

>  	pthread_create(&printer_thread, &thattr, printer_loop, NULL);
>  }
>  
> @@ -464,6 +464,15 @@ static void forked_child_init(void)
>  	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
>  	struct print_buffer **pbuffer = &first_buffer;
>  
> +	if (my_buffer) {
> +		/* Any content of my_buffer should be printed by our parent,
> +		   not us. */
> +		memset(my_buffer->ring, 0, my_buffer->size);
> +
> +		my_buffer->read_pos  = 0;
> +		my_buffer->write_pos = 0;
> +	}
> +

Ack.

>  	/* re-init to avoid finding it locked by some parent thread */
>  	pthread_mutex_init(&buffer_lock, NULL);
>  
> @@ -518,3 +527,13 @@ void __rt_print_init(void)
>  	spawn_printer_thread();
>  	pthread_atfork(NULL, NULL, forked_child_init);
>  }
> +
> +void __rt_print_exit(void)
> +{
> +	if (buffers) {
> +		/* Flush the buffers. Do not call print_buffers here
> +		 * since we do not know if our stack is big enough. */
> +		nanosleep(&print_period, NULL);
> +		nanosleep(&print_period, NULL);
> +	}
> +}

IIRC, that's what cleanup_buffer is supposed to do, triggered by the
pthread key destruction. Can you explain why it failed in your scenario.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> When exiting a process, rtdk buffers are not flushed. Fix that by
>> introducing a destructor for the library which flushes the buffers.
>>
>> When forking, if the main thread has an rt_printf buffer, it is reused by
>> the child, which is a good thing, however, it is not emptied, so could cause
>> the same message to be printed twice. Fix that by emptying the main thread
>> buffer upon fork.
>>
>> When spawning the rt_print thread, it uses a stack size of
>> PTHREAD_STACK_MIN, which may be too small for printf to work reliably on
>> some architectures, set the stack size to the
>> empirically-known-to-be-working size of... 32Kb.
>> ---
>>  src/rtdk/init.c     |    5 +++++
>>  src/rtdk/internal.h |    1 +
>>  src/rtdk/rt_print.c |   21 ++++++++++++++++++++-
>>  3 files changed, 26 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/rtdk/init.c b/src/rtdk/init.c
>> index 2084c73..b864175 100644
>> --- a/src/rtdk/init.c
>> +++ b/src/rtdk/init.c
>> @@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
>>  {
>>  	__rt_print_init();
>>  }
>> +
>> +static __attribute__ ((destructor)) void __exit_rtdk(void)
>> +{
>> +	__rt_print_exit();
>> +}
>> diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
>> index bd15b2d..345d4fa 100644
>> --- a/src/rtdk/internal.h
>> +++ b/src/rtdk/internal.h
>> @@ -23,6 +23,7 @@
>>  #include <sys/time.h>
>>  
>>  void __rt_print_init(void);
>> +void __rt_print_exit(void);
>>  
>>  void __real_free(void *ptr);
>>  void *__real_malloc(size_t size);
>> diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
>> index c6b5c55..dae7d7b 100644
>> --- a/src/rtdk/rt_print.c
>> +++ b/src/rtdk/rt_print.c
>> @@ -455,7 +455,7 @@ static void spawn_printer_thread(void)
>>  	pthread_attr_t thattr;
>>  
>>  	pthread_attr_init(&thattr);
>> -	pthread_attr_setstacksize(&thattr, PTHREAD_STACK_MIN);
>> +	pthread_attr_setstacksize(&thattr, 32768);
> 
> Mmh... maybe rather $something +PTHREAD_STACK_MIN?

No, PTHREAD_STACK_MIN sucks, it is not appropriate, and have very
varying values on different platforms. On some plaforms, with some
versions of glibc, it is 4096, way too small, so that printf generates a
segfault due to a stack overflow.

However, due to the work done when solving the stack issue reported by
Stefan, I hid the magic constants in a function called xeno_stacksize
and xeno_stacksize(0) gives a reasonnable default value.

> 
>>  	pthread_create(&printer_thread, &thattr, printer_loop, NULL);
>>  }
>>  
>> @@ -464,6 +464,15 @@ static void forked_child_init(void)
>>  	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
>>  	struct print_buffer **pbuffer = &first_buffer;
>>  
>> +	if (my_buffer) {
>> +		/* Any content of my_buffer should be printed by our parent,
>> +		   not us. */
>> +		memset(my_buffer->ring, 0, my_buffer->size);
>> +
>> +		my_buffer->read_pos  = 0;
>> +		my_buffer->write_pos = 0;
>> +	}
>> +
> 
> Ack.
> 
>>  	/* re-init to avoid finding it locked by some parent thread */
>>  	pthread_mutex_init(&buffer_lock, NULL);
>>  
>> @@ -518,3 +527,13 @@ void __rt_print_init(void)
>>  	spawn_printer_thread();
>>  	pthread_atfork(NULL, NULL, forked_child_init);
>>  }
>> +
>> +void __rt_print_exit(void)
>> +{
>> +	if (buffers) {
>> +		/* Flush the buffers. Do not call print_buffers here
>> +		 * since we do not know if our stack is big enough. */
>> +		nanosleep(&print_period, NULL);
>> +		nanosleep(&print_period, NULL);
>> +	}
>> +}
> 
> IIRC, that's what cleanup_buffer is supposed to do, triggered by the
> pthread key destruction. Can you explain why it failed in your scenario.

pthread_key destruction is called when a thread is cancelled. My
testcase was calling exit. Besides, cleanup_buffer flushes the buffers
from the context of the calling thread, which is a bad idea, due to the
stack sizes situation.

-- 
Gilles Chanteperdrix, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4457 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> When exiting a process, rtdk buffers are not flushed. Fix that by
>>> introducing a destructor for the library which flushes the buffers.
>>>
>>> When forking, if the main thread has an rt_printf buffer, it is reused by
>>> the child, which is a good thing, however, it is not emptied, so could cause
>>> the same message to be printed twice. Fix that by emptying the main thread
>>> buffer upon fork.
>>>
>>> When spawning the rt_print thread, it uses a stack size of
>>> PTHREAD_STACK_MIN, which may be too small for printf to work reliably on
>>> some architectures, set the stack size to the
>>> empirically-known-to-be-working size of... 32Kb.
>>> ---
>>>  src/rtdk/init.c     |    5 +++++
>>>  src/rtdk/internal.h |    1 +
>>>  src/rtdk/rt_print.c |   21 ++++++++++++++++++++-
>>>  3 files changed, 26 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/src/rtdk/init.c b/src/rtdk/init.c
>>> index 2084c73..b864175 100644
>>> --- a/src/rtdk/init.c
>>> +++ b/src/rtdk/init.c
>>> @@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
>>>  {
>>>  	__rt_print_init();
>>>  }
>>> +
>>> +static __attribute__ ((destructor)) void __exit_rtdk(void)
>>> +{
>>> +	__rt_print_exit();
>>> +}
>>> diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
>>> index bd15b2d..345d4fa 100644
>>> --- a/src/rtdk/internal.h
>>> +++ b/src/rtdk/internal.h
>>> @@ -23,6 +23,7 @@
>>>  #include <sys/time.h>
>>>  
>>>  void __rt_print_init(void);
>>> +void __rt_print_exit(void);
>>>  
>>>  void __real_free(void *ptr);
>>>  void *__real_malloc(size_t size);
>>> diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
>>> index c6b5c55..dae7d7b 100644
>>> --- a/src/rtdk/rt_print.c
>>> +++ b/src/rtdk/rt_print.c
>>> @@ -455,7 +455,7 @@ static void spawn_printer_thread(void)
>>>  	pthread_attr_t thattr;
>>>  
>>>  	pthread_attr_init(&thattr);
>>> -	pthread_attr_setstacksize(&thattr, PTHREAD_STACK_MIN);
>>> +	pthread_attr_setstacksize(&thattr, 32768);
>> Mmh... maybe rather $something +PTHREAD_STACK_MIN?
> 
> No, PTHREAD_STACK_MIN sucks, it is not appropriate, and have very
> varying values on different platforms. On some plaforms, with some
> versions of glibc, it is 4096, way too small, so that printf generates a
> segfault due to a stack overflow.
> 
> However, due to the work done when solving the stack issue reported by
> Stefan, I hid the magic constants in a function called xeno_stacksize
> and xeno_stacksize(0) gives a reasonnable default value.

OK, then go for this service (is it already merged)?

> 
>>>  	pthread_create(&printer_thread, &thattr, printer_loop, NULL);
>>>  }
>>>  
>>> @@ -464,6 +464,15 @@ static void forked_child_init(void)
>>>  	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
>>>  	struct print_buffer **pbuffer = &first_buffer;
>>>  
>>> +	if (my_buffer) {
>>> +		/* Any content of my_buffer should be printed by our parent,
>>> +		   not us. */
>>> +		memset(my_buffer->ring, 0, my_buffer->size);
>>> +
>>> +		my_buffer->read_pos  = 0;
>>> +		my_buffer->write_pos = 0;
>>> +	}
>>> +
>> Ack.
>>
>>>  	/* re-init to avoid finding it locked by some parent thread */
>>>  	pthread_mutex_init(&buffer_lock, NULL);
>>>  
>>> @@ -518,3 +527,13 @@ void __rt_print_init(void)
>>>  	spawn_printer_thread();
>>>  	pthread_atfork(NULL, NULL, forked_child_init);
>>>  }
>>> +
>>> +void __rt_print_exit(void)
>>> +{
>>> +	if (buffers) {
>>> +		/* Flush the buffers. Do not call print_buffers here
>>> +		 * since we do not know if our stack is big enough. */
>>> +		nanosleep(&print_period, NULL);
>>> +		nanosleep(&print_period, NULL);
>>> +	}
>>> +}
>> IIRC, that's what cleanup_buffer is supposed to do, triggered by the
>> pthread key destruction. Can you explain why it failed in your scenario.
> 
> pthread_key destruction is called when a thread is cancelled. My
> testcase was calling exit.

exit() does not trigger these cleanup hooks? Too bad.

> Besides, cleanup_buffer flushes the buffers
> from the context of the calling thread, which is a bad idea, due to the
> stack sizes situation.

If a thread requesting rt_printk services is not capable of performing
this work during it's cleanup, when there is no significant stack load,
I would say it's misconfigured /wrt stack size.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> When exiting a process, rtdk buffers are not flushed. Fix that by
>>>> introducing a destructor for the library which flushes the buffers.
>>>>
>>>> When forking, if the main thread has an rt_printf buffer, it is reused by
>>>> the child, which is a good thing, however, it is not emptied, so could cause
>>>> the same message to be printed twice. Fix that by emptying the main thread
>>>> buffer upon fork.
>>>>
>>>> When spawning the rt_print thread, it uses a stack size of
>>>> PTHREAD_STACK_MIN, which may be too small for printf to work reliably on
>>>> some architectures, set the stack size to the
>>>> empirically-known-to-be-working size of... 32Kb.
>>>> ---
>>>>  src/rtdk/init.c     |    5 +++++
>>>>  src/rtdk/internal.h |    1 +
>>>>  src/rtdk/rt_print.c |   21 ++++++++++++++++++++-
>>>>  3 files changed, 26 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/src/rtdk/init.c b/src/rtdk/init.c
>>>> index 2084c73..b864175 100644
>>>> --- a/src/rtdk/init.c
>>>> +++ b/src/rtdk/init.c
>>>> @@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
>>>>  {
>>>>  	__rt_print_init();
>>>>  }
>>>> +
>>>> +static __attribute__ ((destructor)) void __exit_rtdk(void)
>>>> +{
>>>> +	__rt_print_exit();
>>>> +}
>>>> diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
>>>> index bd15b2d..345d4fa 100644
>>>> --- a/src/rtdk/internal.h
>>>> +++ b/src/rtdk/internal.h
>>>> @@ -23,6 +23,7 @@
>>>>  #include <sys/time.h>
>>>>  
>>>>  void __rt_print_init(void);
>>>> +void __rt_print_exit(void);
>>>>  
>>>>  void __real_free(void *ptr);
>>>>  void *__real_malloc(size_t size);
>>>> diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
>>>> index c6b5c55..dae7d7b 100644
>>>> --- a/src/rtdk/rt_print.c
>>>> +++ b/src/rtdk/rt_print.c
>>>> @@ -455,7 +455,7 @@ static void spawn_printer_thread(void)
>>>>  	pthread_attr_t thattr;
>>>>  
>>>>  	pthread_attr_init(&thattr);
>>>> -	pthread_attr_setstacksize(&thattr, PTHREAD_STACK_MIN);
>>>> +	pthread_attr_setstacksize(&thattr, 32768);
>>> Mmh... maybe rather $something +PTHREAD_STACK_MIN?
>> No, PTHREAD_STACK_MIN sucks, it is not appropriate, and have very
>> varying values on different platforms. On some plaforms, with some
>> versions of glibc, it is 4096, way too small, so that printf generates a
>> segfault due to a stack overflow.
>>
>> However, due to the work done when solving the stack issue reported by
>> Stefan, I hid the magic constants in a function called xeno_stacksize
>> and xeno_stacksize(0) gives a reasonnable default value.
> 
> OK, then go for this service (is it already merged)?
> 
>>>>  	pthread_create(&printer_thread, &thattr, printer_loop, NULL);
>>>>  }
>>>>  
>>>> @@ -464,6 +464,15 @@ static void forked_child_init(void)
>>>>  	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
>>>>  	struct print_buffer **pbuffer = &first_buffer;
>>>>  
>>>> +	if (my_buffer) {
>>>> +		/* Any content of my_buffer should be printed by our parent,
>>>> +		   not us. */
>>>> +		memset(my_buffer->ring, 0, my_buffer->size);
>>>> +
>>>> +		my_buffer->read_pos  = 0;
>>>> +		my_buffer->write_pos = 0;
>>>> +	}
>>>> +
>>> Ack.
>>>
>>>>  	/* re-init to avoid finding it locked by some parent thread */
>>>>  	pthread_mutex_init(&buffer_lock, NULL);
>>>>  
>>>> @@ -518,3 +527,13 @@ void __rt_print_init(void)
>>>>  	spawn_printer_thread();
>>>>  	pthread_atfork(NULL, NULL, forked_child_init);
>>>>  }
>>>> +
>>>> +void __rt_print_exit(void)
>>>> +{
>>>> +	if (buffers) {
>>>> +		/* Flush the buffers. Do not call print_buffers here
>>>> +		 * since we do not know if our stack is big enough. */
>>>> +		nanosleep(&print_period, NULL);
>>>> +		nanosleep(&print_period, NULL);
>>>> +	}
>>>> +}
>>> IIRC, that's what cleanup_buffer is supposed to do, triggered by the
>>> pthread key destruction. Can you explain why it failed in your scenario.
>> pthread_key destruction is called when a thread is cancelled. My
>> testcase was calling exit.
> 
> exit() does not trigger these cleanup hooks? Too bad.
> 
>> Besides, cleanup_buffer flushes the buffers
>> from the context of the calling thread, which is a bad idea, due to the
>> stack sizes situation.
> 
> If a thread requesting rt_printk services is not capable of performing
> this work during it's cleanup, when there is no significant stack load,
> I would say it's misconfigured /wrt stack size.

Ok, but is is printf which causes the significant stack load. It is fine
with me. However, in the case of exit, the main thread is not even
necessarily supposed to be able to call rt_printf, so, I think leaving
the job to the printing thread is better.

-- 
Gilles Chanteperdrix, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> When exiting a process, rtdk buffers are not flushed. Fix that by
>>>> introducing a destructor for the library which flushes the buffers.
>>>>
>>>> When forking, if the main thread has an rt_printf buffer, it is reused by
>>>> the child, which is a good thing, however, it is not emptied, so could cause
>>>> the same message to be printed twice. Fix that by emptying the main thread
>>>> buffer upon fork.
>>>>
>>>> When spawning the rt_print thread, it uses a stack size of
>>>> PTHREAD_STACK_MIN, which may be too small for printf to work reliably on
>>>> some architectures, set the stack size to the
>>>> empirically-known-to-be-working size of... 32Kb.
>>>> ---
>>>>  src/rtdk/init.c     |    5 +++++
>>>>  src/rtdk/internal.h |    1 +
>>>>  src/rtdk/rt_print.c |   21 ++++++++++++++++++++-
>>>>  3 files changed, 26 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/src/rtdk/init.c b/src/rtdk/init.c
>>>> index 2084c73..b864175 100644
>>>> --- a/src/rtdk/init.c
>>>> +++ b/src/rtdk/init.c
>>>> @@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
>>>>  {
>>>>  	__rt_print_init();
>>>>  }
>>>> +
>>>> +static __attribute__ ((destructor)) void __exit_rtdk(void)
>>>> +{
>>>> +	__rt_print_exit();
>>>> +}
>>>> diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
>>>> index bd15b2d..345d4fa 100644
>>>> --- a/src/rtdk/internal.h
>>>> +++ b/src/rtdk/internal.h
>>>> @@ -23,6 +23,7 @@
>>>>  #include <sys/time.h>
>>>>  
>>>>  void __rt_print_init(void);
>>>> +void __rt_print_exit(void);
>>>>  
>>>>  void __real_free(void *ptr);
>>>>  void *__real_malloc(size_t size);
>>>> diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
>>>> index c6b5c55..dae7d7b 100644
>>>> --- a/src/rtdk/rt_print.c
>>>> +++ b/src/rtdk/rt_print.c
>>>> @@ -455,7 +455,7 @@ static void spawn_printer_thread(void)
>>>>  	pthread_attr_t thattr;
>>>>  
>>>>  	pthread_attr_init(&thattr);
>>>> -	pthread_attr_setstacksize(&thattr, PTHREAD_STACK_MIN);
>>>> +	pthread_attr_setstacksize(&thattr, 32768);
>>> Mmh... maybe rather $something +PTHREAD_STACK_MIN?
>> No, PTHREAD_STACK_MIN sucks, it is not appropriate, and have very
>> varying values on different platforms. On some plaforms, with some
>> versions of glibc, it is 4096, way too small, so that printf generates a
>> segfault due to a stack overflow.
>>
>> However, due to the work done when solving the stack issue reported by
>> Stefan, I hid the magic constants in a function called xeno_stacksize
>> and xeno_stacksize(0) gives a reasonnable default value.
> 
> OK, then go for this service (is it already merged)?

it is pending, was waiting for Stefan confirmation that it fixed his
issue, should be pushed soon.

-- 
Gilles Chanteperdrix, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> When exiting a process, rtdk buffers are not flushed. Fix that by
>>>> introducing a destructor for the library which flushes the buffers.
>>>>
>>>> When forking, if the main thread has an rt_printf buffer, it is reused by
>>>> the child, which is a good thing, however, it is not emptied, so could cause
>>>> the same message to be printed twice. Fix that by emptying the main thread
>>>> buffer upon fork.
>>>>
>>>> When spawning the rt_print thread, it uses a stack size of
>>>> PTHREAD_STACK_MIN, which may be too small for printf to work reliably on
>>>> some architectures, set the stack size to the
>>>> empirically-known-to-be-working size of... 32Kb.
>>>> ---
>>>>  src/rtdk/init.c     |    5 +++++
>>>>  src/rtdk/internal.h |    1 +
>>>>  src/rtdk/rt_print.c |   21 ++++++++++++++++++++-
>>>>  3 files changed, 26 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/src/rtdk/init.c b/src/rtdk/init.c
>>>> index 2084c73..b864175 100644
>>>> --- a/src/rtdk/init.c
>>>> +++ b/src/rtdk/init.c
>>>> @@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
>>>>  {
>>>>  	__rt_print_init();
>>>>  }
>>>> +
>>>> +static __attribute__ ((destructor)) void __exit_rtdk(void)
>>>> +{
>>>> +	__rt_print_exit();
>>>> +}
>>>> diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
>>>> index bd15b2d..345d4fa 100644
>>>> --- a/src/rtdk/internal.h
>>>> +++ b/src/rtdk/internal.h
>>>> @@ -23,6 +23,7 @@
>>>>  #include <sys/time.h>
>>>>  
>>>>  void __rt_print_init(void);
>>>> +void __rt_print_exit(void);
>>>>  
>>>>  void __real_free(void *ptr);
>>>>  void *__real_malloc(size_t size);
>>>> diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
>>>> index c6b5c55..dae7d7b 100644
>>>> --- a/src/rtdk/rt_print.c
>>>> +++ b/src/rtdk/rt_print.c
>>>> @@ -455,7 +455,7 @@ static void spawn_printer_thread(void)
>>>>  	pthread_attr_t thattr;
>>>>  
>>>>  	pthread_attr_init(&thattr);
>>>> -	pthread_attr_setstacksize(&thattr, PTHREAD_STACK_MIN);
>>>> +	pthread_attr_setstacksize(&thattr, 32768);
>>> Mmh... maybe rather $something +PTHREAD_STACK_MIN?
>> No, PTHREAD_STACK_MIN sucks, it is not appropriate, and have very
>> varying values on different platforms. On some plaforms, with some
>> versions of glibc, it is 4096, way too small, so that printf generates a
>> segfault due to a stack overflow.
>>
>> However, due to the work done when solving the stack issue reported by
>> Stefan, I hid the magic constants in a function called xeno_stacksize
>> and xeno_stacksize(0) gives a reasonnable default value.
> 
> OK, then go for this service (is it already merged)?
> 
>>>>  	pthread_create(&printer_thread, &thattr, printer_loop, NULL);
>>>>  }
>>>>  
>>>> @@ -464,6 +464,15 @@ static void forked_child_init(void)
>>>>  	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
>>>>  	struct print_buffer **pbuffer = &first_buffer;
>>>>  
>>>> +	if (my_buffer) {
>>>> +		/* Any content of my_buffer should be printed by our parent,
>>>> +		   not us. */
>>>> +		memset(my_buffer->ring, 0, my_buffer->size);
>>>> +
>>>> +		my_buffer->read_pos  = 0;
>>>> +		my_buffer->write_pos = 0;
>>>> +	}
>>>> +
>>> Ack.
>>>
>>>>  	/* re-init to avoid finding it locked by some parent thread */
>>>>  	pthread_mutex_init(&buffer_lock, NULL);
>>>>  
>>>> @@ -518,3 +527,13 @@ void __rt_print_init(void)
>>>>  	spawn_printer_thread();
>>>>  	pthread_atfork(NULL, NULL, forked_child_init);
>>>>  }
>>>> +
>>>> +void __rt_print_exit(void)
>>>> +{
>>>> +	if (buffers) {
>>>> +		/* Flush the buffers. Do not call print_buffers here
>>>> +		 * since we do not know if our stack is big enough. */
>>>> +		nanosleep(&print_period, NULL);
>>>> +		nanosleep(&print_period, NULL);
>>>> +	}
>>>> +}
>>> IIRC, that's what cleanup_buffer is supposed to do, triggered by the
>>> pthread key destruction. Can you explain why it failed in your scenario.
>> pthread_key destruction is called when a thread is cancelled. My
>> testcase was calling exit.
> 
> exit() does not trigger these cleanup hooks? Too bad.
> 
>> Besides, cleanup_buffer flushes the buffers
>> from the context of the calling thread, which is a bad idea, due to the
>> stack sizes situation.
> 
> If a thread requesting rt_printk services is not capable of performing
> this work during it's cleanup, when there is no significant stack load,
> I would say it's misconfigured /wrt stack size.

Do you agree with this rebased patch?

diff --git a/src/rtdk/init.c b/src/rtdk/init.c
index 2084c73..b864175 100644
--- a/src/rtdk/init.c
+++ b/src/rtdk/init.c
@@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
 {
 	__rt_print_init();
 }
+
+static __attribute__ ((destructor)) void __exit_rtdk(void)
+{
+	__rt_print_exit();
+}
diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
index bd15b2d..345d4fa 100644
--- a/src/rtdk/internal.h
+++ b/src/rtdk/internal.h
@@ -23,6 +23,7 @@
 #include <sys/time.h>
 
 void __rt_print_init(void);
+void __rt_print_exit(void);
 
 void __real_free(void *ptr);
 void *__real_malloc(size_t size);
diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
index 1d2b70a..fc170d0 100644
--- a/src/rtdk/rt_print.c
+++ b/src/rtdk/rt_print.c
@@ -29,6 +29,7 @@
 
 #include <rtdk.h>
 #include <asm/xenomai/system.h>
+#include <asm-generic/stacksize.h>
 
 #define RT_PRINT_BUFFER_ENV		"RT_PRINT_BUFFER"
 #define RT_PRINT_DEFAULT_BUFFER		16*1024
@@ -464,6 +465,15 @@ static void forked_child_init(void)
 	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
 	struct print_buffer **pbuffer = &first_buffer;
 
+	if (my_buffer) {
+		/* Any content of my_buffer should be printed by our parent,
+		   not us. */
+		memset(my_buffer->ring, 0, my_buffer->size);
+
+		my_buffer->read_pos  = 0;
+		my_buffer->write_pos = 0;
+	}
+
 	/* re-init to avoid finding it locked by some parent thread */
 	pthread_mutex_init(&buffer_lock, NULL);
 
@@ -518,3 +528,13 @@ void __rt_print_init(void)
 	spawn_printer_thread();
 	pthread_atfork(NULL, NULL, forked_child_init);
 }
+
+void __rt_print_exit(void)
+{
+	if (buffers) {
+		/* Flush the buffers. Do not call print_buffers here
+		 * since we do not know if our stack is big enough. */
+		nanosleep(&print_period, NULL);
+		nanosleep(&print_period, NULL);
+	}
+}

-- 
					    Gilles.

Re: [Xenomai-core] [PATCH 1/1] rtdk: various fixes.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 6853 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> When exiting a process, rtdk buffers are not flushed. Fix that by
>>>>> introducing a destructor for the library which flushes the buffers.
>>>>>
>>>>> When forking, if the main thread has an rt_printf buffer, it is reused by
>>>>> the child, which is a good thing, however, it is not emptied, so could cause
>>>>> the same message to be printed twice. Fix that by emptying the main thread
>>>>> buffer upon fork.
>>>>>
>>>>> When spawning the rt_print thread, it uses a stack size of
>>>>> PTHREAD_STACK_MIN, which may be too small for printf to work reliably on
>>>>> some architectures, set the stack size to the
>>>>> empirically-known-to-be-working size of... 32Kb.
>>>>> ---
>>>>>  src/rtdk/init.c     |    5 +++++
>>>>>  src/rtdk/internal.h |    1 +
>>>>>  src/rtdk/rt_print.c |   21 ++++++++++++++++++++-
>>>>>  3 files changed, 26 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/src/rtdk/init.c b/src/rtdk/init.c
>>>>> index 2084c73..b864175 100644
>>>>> --- a/src/rtdk/init.c
>>>>> +++ b/src/rtdk/init.c
>>>>> @@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
>>>>>  {
>>>>>  	__rt_print_init();
>>>>>  }
>>>>> +
>>>>> +static __attribute__ ((destructor)) void __exit_rtdk(void)
>>>>> +{
>>>>> +	__rt_print_exit();
>>>>> +}
>>>>> diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
>>>>> index bd15b2d..345d4fa 100644
>>>>> --- a/src/rtdk/internal.h
>>>>> +++ b/src/rtdk/internal.h
>>>>> @@ -23,6 +23,7 @@
>>>>>  #include <sys/time.h>
>>>>>  
>>>>>  void __rt_print_init(void);
>>>>> +void __rt_print_exit(void);
>>>>>  
>>>>>  void __real_free(void *ptr);
>>>>>  void *__real_malloc(size_t size);
>>>>> diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
>>>>> index c6b5c55..dae7d7b 100644
>>>>> --- a/src/rtdk/rt_print.c
>>>>> +++ b/src/rtdk/rt_print.c
>>>>> @@ -455,7 +455,7 @@ static void spawn_printer_thread(void)
>>>>>  	pthread_attr_t thattr;
>>>>>  
>>>>>  	pthread_attr_init(&thattr);
>>>>> -	pthread_attr_setstacksize(&thattr, PTHREAD_STACK_MIN);
>>>>> +	pthread_attr_setstacksize(&thattr, 32768);
>>>> Mmh... maybe rather $something +PTHREAD_STACK_MIN?
>>> No, PTHREAD_STACK_MIN sucks, it is not appropriate, and have very
>>> varying values on different platforms. On some plaforms, with some
>>> versions of glibc, it is 4096, way too small, so that printf generates a
>>> segfault due to a stack overflow.
>>>
>>> However, due to the work done when solving the stack issue reported by
>>> Stefan, I hid the magic constants in a function called xeno_stacksize
>>> and xeno_stacksize(0) gives a reasonnable default value.
>> OK, then go for this service (is it already merged)?
>>
>>>>>  	pthread_create(&printer_thread, &thattr, printer_loop, NULL);
>>>>>  }
>>>>>  
>>>>> @@ -464,6 +464,15 @@ static void forked_child_init(void)
>>>>>  	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
>>>>>  	struct print_buffer **pbuffer = &first_buffer;
>>>>>  
>>>>> +	if (my_buffer) {
>>>>> +		/* Any content of my_buffer should be printed by our parent,
>>>>> +		   not us. */
>>>>> +		memset(my_buffer->ring, 0, my_buffer->size);
>>>>> +
>>>>> +		my_buffer->read_pos  = 0;
>>>>> +		my_buffer->write_pos = 0;
>>>>> +	}
>>>>> +
>>>> Ack.
>>>>
>>>>>  	/* re-init to avoid finding it locked by some parent thread */
>>>>>  	pthread_mutex_init(&buffer_lock, NULL);
>>>>>  
>>>>> @@ -518,3 +527,13 @@ void __rt_print_init(void)
>>>>>  	spawn_printer_thread();
>>>>>  	pthread_atfork(NULL, NULL, forked_child_init);
>>>>>  }
>>>>> +
>>>>> +void __rt_print_exit(void)
>>>>> +{
>>>>> +	if (buffers) {
>>>>> +		/* Flush the buffers. Do not call print_buffers here
>>>>> +		 * since we do not know if our stack is big enough. */
>>>>> +		nanosleep(&print_period, NULL);
>>>>> +		nanosleep(&print_period, NULL);
>>>>> +	}
>>>>> +}
>>>> IIRC, that's what cleanup_buffer is supposed to do, triggered by the
>>>> pthread key destruction. Can you explain why it failed in your scenario.
>>> pthread_key destruction is called when a thread is cancelled. My
>>> testcase was calling exit.
>> exit() does not trigger these cleanup hooks? Too bad.
>>
>>> Besides, cleanup_buffer flushes the buffers
>>> from the context of the calling thread, which is a bad idea, due to the
>>> stack sizes situation.
>> If a thread requesting rt_printk services is not capable of performing
>> this work during it's cleanup, when there is no significant stack load,
>> I would say it's misconfigured /wrt stack size.
> 
> Do you agree with this rebased patch?
> 
> diff --git a/src/rtdk/init.c b/src/rtdk/init.c
> index 2084c73..b864175 100644
> --- a/src/rtdk/init.c
> +++ b/src/rtdk/init.c
> @@ -22,3 +22,8 @@ static __attribute__ ((constructor)) void __init_rtdk(void)
>  {
>  	__rt_print_init();
>  }
> +
> +static __attribute__ ((destructor)) void __exit_rtdk(void)
> +{
> +	__rt_print_exit();
> +}
> diff --git a/src/rtdk/internal.h b/src/rtdk/internal.h
> index bd15b2d..345d4fa 100644
> --- a/src/rtdk/internal.h
> +++ b/src/rtdk/internal.h
> @@ -23,6 +23,7 @@
>  #include <sys/time.h>
>  
>  void __rt_print_init(void);
> +void __rt_print_exit(void);
>  
>  void __real_free(void *ptr);
>  void *__real_malloc(size_t size);
> diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
> index 1d2b70a..fc170d0 100644
> --- a/src/rtdk/rt_print.c
> +++ b/src/rtdk/rt_print.c
> @@ -29,6 +29,7 @@
>  
>  #include <rtdk.h>
>  #include <asm/xenomai/system.h>
> +#include <asm-generic/stacksize.h>
>  
>  #define RT_PRINT_BUFFER_ENV		"RT_PRINT_BUFFER"
>  #define RT_PRINT_DEFAULT_BUFFER		16*1024
> @@ -464,6 +465,15 @@ static void forked_child_init(void)
>  	struct print_buffer *my_buffer = pthread_getspecific(buffer_key);
>  	struct print_buffer **pbuffer = &first_buffer;
>  
> +	if (my_buffer) {
> +		/* Any content of my_buffer should be printed by our parent,
> +		   not us. */
> +		memset(my_buffer->ring, 0, my_buffer->size);
> +
> +		my_buffer->read_pos  = 0;
> +		my_buffer->write_pos = 0;
> +	}
> +
>  	/* re-init to avoid finding it locked by some parent thread */
>  	pthread_mutex_init(&buffer_lock, NULL);
>  
> @@ -518,3 +528,13 @@ void __rt_print_init(void)
>  	spawn_printer_thread();
>  	pthread_atfork(NULL, NULL, forked_child_init);
>  }
> +
> +void __rt_print_exit(void)
> +{
> +	if (buffers) {
> +		/* Flush the buffers. Do not call print_buffers here
> +		 * since we do not know if our stack is big enough. */
> +		nanosleep(&print_period, NULL);
> +		nanosleep(&print_period, NULL);
> +	}
> +}
> 

Oh, this is still lacking an OK: Looks good to me!

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]