[Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Jan Kiszka]

xenomai-git-request@domain.hid wrote:
> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
> index f0e569c..b9ac680 100644
> --- a/include/asm-generic/bits/current.h
> +++ b/include/asm-generic/bits/current.h

...

> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>  {
>  	void *val = pthread_getspecific(xeno_current_key);
>  
> -	if (!val)
> -		return XN_NO_HANDLE;
> -
> -	return (xnhandle_t)val;
> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>  }

So when used with normal Linux threads, this will always trigger the
syscall of xeno_slow_get_current()?

> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
> index bad19f3..f67bcd8 100644
> --- a/src/rtdk/assert_context.c
> +++ b/src/rtdk/assert_context.c
> @@ -30,12 +30,9 @@ void assert_nrt(void)
>  	xnthread_info_t info;
>  	int err;
>  
> -	if (xeno_get_current() != XN_NO_HANDLE)
> -		return;
> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
> +		     !(xeno_get_current_mode() & XNRELAX))) {

Then please provide a different API for assert_nrt as this makes the
service too heavy for common use.

The current_mode slow path is no problem as it only triggers during cleanup.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Xenomai-core] [Xenomai-git] common: modify leak warning

[Jan Kiszka]

xenomai-git-request@domain.hid wrote:
> diff --git a/src/skins/common/current.c b/src/skins/common/current.c
> index 7277189..5d418a4 100644
> --- a/src/skins/common/current.c
> +++ b/src/skins/common/current.c
> @@ -29,8 +29,6 @@ static inline unsigned long *create_current_mode(void)
>  
>  static inline void free_current_mode(unsigned long *mode) { }
>  
> -#define XENO_MODE_LEAK_WARNING ""
> -
>  #else /* !HAVE___THREAD */
>  
>  pthread_key_t xeno_current_key;
> @@ -56,8 +54,9 @@ static inline void free_current_mode(unsigned long *mode)
>  }
>  
>  #define XENO_MODE_LEAK_WARNING \
> -	"         To reduce the probality, we leak a few bytes of heap " \
> -	"per thread.\n"
> +	"Xenomai: WARNING, this version of Xenomai kernel is anterior to" \
> +	" 2.5.2.\nIn order to avoid getting memory corruption, we leak 4" \
> +	" bytes per thread.\nUpgrade is recommended.\n"
>  
>  #endif /* !HAVE___THREAD */
>  
> @@ -70,20 +69,18 @@ static void cleanup_current_mode(void *key)
>  
>  	err = XENOMAI_SYSCALL0(__xn_sys_drop_u_mode);
>  
> -	if (err) {
> +	if (!err)
> +		free_current_mode(mode);
> +#ifdef XENO_MODE_LEAK_WARNING
> +	else {
>  		static int warned;
>  
>  		if (!warned) {
>  			warned = 1;
> -			fprintf(stderr,
> -				"\nXenomai: WARNING, this Xenomai kernel can "
> -					"cause spurious application\n"
> -				"         crashes on thread termination. "
> -					"Upgrade is highly recommended.\n%s\n",
> -				XENO_MODE_LEAK_WARNING);
> +			fprintf(stderr, XENO_MODE_LEAK_WARNING);
>  		}
> -	} else
> -		free_current_mode(mode);
> +	}
> +#endif /* XENO_MODE_LEAK_WARNING */
>  }
>  
>  static void init_current_keys(void)
> 

This no longer issues a warning for the __thread case. We might have
been lucky that there was no issue in practice, but are we sure? That
was my motivation to warn about both scenarios.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> xenomai-git-request@domain.hid wrote:
>> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
>> index f0e569c..b9ac680 100644
>> --- a/include/asm-generic/bits/current.h
>> +++ b/include/asm-generic/bits/current.h
> 
> ...
> 
>> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>>  {
>>  	void *val = pthread_getspecific(xeno_current_key);
>>  
>> -	if (!val)
>> -		return XN_NO_HANDLE;
>> -
>> -	return (xnhandle_t)val;
>> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>>  }
> 
> So when used with normal Linux threads, this will always trigger the
> syscall of xeno_slow_get_current()?
> 
>> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
>> index bad19f3..f67bcd8 100644
>> --- a/src/rtdk/assert_context.c
>> +++ b/src/rtdk/assert_context.c
>> @@ -30,12 +30,9 @@ void assert_nrt(void)
>>  	xnthread_info_t info;
>>  	int err;
>>  
>> -	if (xeno_get_current() != XN_NO_HANDLE)
>> -		return;
>> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
>> +		     !(xeno_get_current_mode() & XNRELAX))) {
> 
> Then please provide a different API for assert_nrt as this makes the
> service too heavy for common use.

It is a non real-time thread. Its performances are not critical. A
non-blocking syscall is not that heavy. Especially compared to the
execution time of malloc. Ok, will not argue on this any more, as
obviously our opinions differ in that area.

The point is that NULL (XN_NO_HANDLE) means at the same time a freed
mode and a mode after the TSD cleanup was run. So, emitting a syscall in
that case is the simplest thing we can do. And no, I did not find it
expensive. But in fact, using an additional syscall, assert_nrt could be
implemented as a simple dummy syscall which returns 0 and asks the
caller to be put in primary mode (and it would be even lighter). So, I
guess if you did not implemented that way, it means that you already
wanted to avoid the syscall.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] common: modify leak warning

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> xenomai-git-request@domain.hid wrote:
>> diff --git a/src/skins/common/current.c b/src/skins/common/current.c
>> index 7277189..5d418a4 100644
>> --- a/src/skins/common/current.c
>> +++ b/src/skins/common/current.c
>> @@ -29,8 +29,6 @@ static inline unsigned long *create_current_mode(void)
>>  
>>  static inline void free_current_mode(unsigned long *mode) { }
>>  
>> -#define XENO_MODE_LEAK_WARNING ""
>> -
>>  #else /* !HAVE___THREAD */
>>  
>>  pthread_key_t xeno_current_key;
>> @@ -56,8 +54,9 @@ static inline void free_current_mode(unsigned long *mode)
>>  }
>>  
>>  #define XENO_MODE_LEAK_WARNING \
>> -	"         To reduce the probality, we leak a few bytes of heap " \
>> -	"per thread.\n"
>> +	"Xenomai: WARNING, this version of Xenomai kernel is anterior to" \
>> +	" 2.5.2.\nIn order to avoid getting memory corruption, we leak 4" \
>> +	" bytes per thread.\nUpgrade is recommended.\n"
>>  
>>  #endif /* !HAVE___THREAD */
>>  
>> @@ -70,20 +69,18 @@ static void cleanup_current_mode(void *key)
>>  
>>  	err = XENOMAI_SYSCALL0(__xn_sys_drop_u_mode);
>>  
>> -	if (err) {
>> +	if (!err)
>> +		free_current_mode(mode);
>> +#ifdef XENO_MODE_LEAK_WARNING
>> +	else {
>>  		static int warned;
>>  
>>  		if (!warned) {
>>  			warned = 1;
>> -			fprintf(stderr,
>> -				"\nXenomai: WARNING, this Xenomai kernel can "
>> -					"cause spurious application\n"
>> -				"         crashes on thread termination. "
>> -					"Upgrade is highly recommended.\n%s\n",
>> -				XENO_MODE_LEAK_WARNING);
>> +			fprintf(stderr, XENO_MODE_LEAK_WARNING);
>>  		}
>> -	} else
>> -		free_current_mode(mode);
>> +	}
>> +#endif /* XENO_MODE_LEAK_WARNING */
>>  }
>>  
>>  static void init_current_keys(void)
>>
> 
> This no longer issues a warning for the __thread case. We might have
> been lucky that there was no issue in practice, but are we sure? That
> was my motivation to warn about both scenarios.

No, you are right. I got all mixed up. I assumed that since we were
setting the current_mode to an invalid state, we were safe. But
obviously, this does not work with the older kernel-space support.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> xenomai-git-request@domain.hid wrote:
>>> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
>>> index f0e569c..b9ac680 100644
>>> --- a/include/asm-generic/bits/current.h
>>> +++ b/include/asm-generic/bits/current.h
>> ...
>>
>>> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>>>  {
>>>  	void *val = pthread_getspecific(xeno_current_key);
>>>  
>>> -	if (!val)
>>> -		return XN_NO_HANDLE;
>>> -
>>> -	return (xnhandle_t)val;
>>> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>>>  }
>> So when used with normal Linux threads, this will always trigger the
>> syscall of xeno_slow_get_current()?
>>
>>> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
>>> index bad19f3..f67bcd8 100644
>>> --- a/src/rtdk/assert_context.c
>>> +++ b/src/rtdk/assert_context.c
>>> @@ -30,12 +30,9 @@ void assert_nrt(void)
>>>  	xnthread_info_t info;
>>>  	int err;
>>>  
>>> -	if (xeno_get_current() != XN_NO_HANDLE)
>>> -		return;
>>> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
>>> +		     !(xeno_get_current_mode() & XNRELAX))) {
>> Then please provide a different API for assert_nrt as this makes the
>> service too heavy for common use.
> 
> It is a non real-time thread. Its performances are not critical. A
> non-blocking syscall is not that heavy. Especially compared to the
> execution time of malloc. Ok, will not argue on this any more, as
> obviously our opinions differ in that area.

Sorry, disagree. We are using it in mixed applications where both RT
latency as well as non-RT throughput matters. The assert_nrt fast was
designed to remain lightweight for both non-Xenomai threads as well as
migrated threads.

> 
> The point is that NULL (XN_NO_HANDLE) means at the same time a freed
> mode and a mode after the TSD cleanup was run. So, emitting a syscall in
> that case is the simplest thing we can do. And no, I did not find it
> expensive. But in fact, using an additional syscall, assert_nrt could be
> implemented as a simple dummy syscall which returns 0 and asks the
> caller to be put in primary mode (and it would be even lighter). So, I
> guess if you did not implemented that way, it means that you already
> wanted to avoid the syscall.

Can't follow on this yet.

The point of assert_nrt remains to issue no syscall in the fast path. We
have this in malloc, and issuing one syscall per allocation/release of
every chunk is unacceptable - my colleague will shot me!

I'm fine with changing the semantic of xeno_get_current, but please let
us add some special service that does not go beyond checking in user space.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Jan Kiszka]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> xenomai-git-request@domain.hid wrote:
>>>> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
>>>> index f0e569c..b9ac680 100644
>>>> --- a/include/asm-generic/bits/current.h
>>>> +++ b/include/asm-generic/bits/current.h
>>> ...
>>>
>>>> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>>>>  {
>>>>  	void *val = pthread_getspecific(xeno_current_key);
>>>>  
>>>> -	if (!val)
>>>> -		return XN_NO_HANDLE;
>>>> -
>>>> -	return (xnhandle_t)val;
>>>> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>>>>  }
>>> So when used with normal Linux threads, this will always trigger the
>>> syscall of xeno_slow_get_current()?
>>>
>>>> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
>>>> index bad19f3..f67bcd8 100644
>>>> --- a/src/rtdk/assert_context.c
>>>> +++ b/src/rtdk/assert_context.c
>>>> @@ -30,12 +30,9 @@ void assert_nrt(void)
>>>>  	xnthread_info_t info;
>>>>  	int err;
>>>>  
>>>> -	if (xeno_get_current() != XN_NO_HANDLE)
>>>> -		return;
>>>> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
>>>> +		     !(xeno_get_current_mode() & XNRELAX))) {
>>> Then please provide a different API for assert_nrt as this makes the
>>> service too heavy for common use.
>> It is a non real-time thread. Its performances are not critical. A
>> non-blocking syscall is not that heavy. Especially compared to the
>> execution time of malloc. Ok, will not argue on this any more, as
>> obviously our opinions differ in that area.
> 
> Sorry, disagree. We are using it in mixed applications where both RT
> latency as well as non-RT throughput matters. The assert_nrt fast was
> designed to remain lightweight for both non-Xenomai threads as well as
> migrated threads.
> 
>> The point is that NULL (XN_NO_HANDLE) means at the same time a freed
>> mode and a mode after the TSD cleanup was run. So, emitting a syscall in
>> that case is the simplest thing we can do. And no, I did not find it
>> expensive. But in fact, using an additional syscall, assert_nrt could be
>> implemented as a simple dummy syscall which returns 0 and asks the
>> caller to be put in primary mode (and it would be even lighter). So, I
>> guess if you did not implemented that way, it means that you already
>> wanted to avoid the syscall.
> 
> Can't follow on this yet.

...specifically as an unset current key is a bug for a Xenomai thread.
Every skin library is supposed to set it, so there should be no need for
falling back to a syscall for them, and there is no point in trying it
for non-Xenomai threads.

So why this fallback? Does it simplify something else that I miss ATM?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> xenomai-git-request@domain.hid wrote:
>>>>> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
>>>>> index f0e569c..b9ac680 100644
>>>>> --- a/include/asm-generic/bits/current.h
>>>>> +++ b/include/asm-generic/bits/current.h
>>>> ...
>>>>
>>>>> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>>>>>  {
>>>>>  	void *val = pthread_getspecific(xeno_current_key);
>>>>>  
>>>>> -	if (!val)
>>>>> -		return XN_NO_HANDLE;
>>>>> -
>>>>> -	return (xnhandle_t)val;
>>>>> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>>>>>  }
>>>> So when used with normal Linux threads, this will always trigger the
>>>> syscall of xeno_slow_get_current()?
>>>>
>>>>> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
>>>>> index bad19f3..f67bcd8 100644
>>>>> --- a/src/rtdk/assert_context.c
>>>>> +++ b/src/rtdk/assert_context.c
>>>>> @@ -30,12 +30,9 @@ void assert_nrt(void)
>>>>>  	xnthread_info_t info;
>>>>>  	int err;
>>>>>  
>>>>> -	if (xeno_get_current() != XN_NO_HANDLE)
>>>>> -		return;
>>>>> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
>>>>> +		     !(xeno_get_current_mode() & XNRELAX))) {
>>>> Then please provide a different API for assert_nrt as this makes the
>>>> service too heavy for common use.
>>> It is a non real-time thread. Its performances are not critical. A
>>> non-blocking syscall is not that heavy. Especially compared to the
>>> execution time of malloc. Ok, will not argue on this any more, as
>>> obviously our opinions differ in that area.
>> Sorry, disagree. We are using it in mixed applications where both RT
>> latency as well as non-RT throughput matters. The assert_nrt fast was
>> designed to remain lightweight for both non-Xenomai threads as well as
>> migrated threads.
>>
>>> The point is that NULL (XN_NO_HANDLE) means at the same time a freed
>>> mode and a mode after the TSD cleanup was run. So, emitting a syscall in
>>> that case is the simplest thing we can do. And no, I did not find it
>>> expensive. But in fact, using an additional syscall, assert_nrt could be
>>> implemented as a simple dummy syscall which returns 0 and asks the
>>> caller to be put in primary mode (and it would be even lighter). So, I
>>> guess if you did not implemented that way, it means that you already
>>> wanted to avoid the syscall.
>> Can't follow on this yet.
> 
> ...specifically as an unset current key is a bug for a Xenomai thread.
> Every skin library is supposed to set it, so there should be no need for
> falling back to a syscall for them, and there is no point in trying it
> for non-Xenomai threads.
> 
> So why this fallback? Does it simplify something else that I miss ATM?

After the TSD cleanup is run, the TSD are set to NULL. Actually, I
assumed that when doing this change. I need to check.

> 
> Jan
> 


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Gilles Chanteperdrix]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> xenomai-git-request@domain.hid wrote:
>>>>>> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
>>>>>> index f0e569c..b9ac680 100644
>>>>>> --- a/include/asm-generic/bits/current.h
>>>>>> +++ b/include/asm-generic/bits/current.h
>>>>> ...
>>>>>
>>>>>> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>>>>>>  {
>>>>>>  	void *val = pthread_getspecific(xeno_current_key);
>>>>>>  
>>>>>> -	if (!val)
>>>>>> -		return XN_NO_HANDLE;
>>>>>> -
>>>>>> -	return (xnhandle_t)val;
>>>>>> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>>>>>>  }
>>>>> So when used with normal Linux threads, this will always trigger the
>>>>> syscall of xeno_slow_get_current()?
>>>>>
>>>>>> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
>>>>>> index bad19f3..f67bcd8 100644
>>>>>> --- a/src/rtdk/assert_context.c
>>>>>> +++ b/src/rtdk/assert_context.c
>>>>>> @@ -30,12 +30,9 @@ void assert_nrt(void)
>>>>>>  	xnthread_info_t info;
>>>>>>  	int err;
>>>>>>  
>>>>>> -	if (xeno_get_current() != XN_NO_HANDLE)
>>>>>> -		return;
>>>>>> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
>>>>>> +		     !(xeno_get_current_mode() & XNRELAX))) {
>>>>> Then please provide a different API for assert_nrt as this makes the
>>>>> service too heavy for common use.
>>>> It is a non real-time thread. Its performances are not critical. A
>>>> non-blocking syscall is not that heavy. Especially compared to the
>>>> execution time of malloc. Ok, will not argue on this any more, as
>>>> obviously our opinions differ in that area.
>>> Sorry, disagree. We are using it in mixed applications where both RT
>>> latency as well as non-RT throughput matters. The assert_nrt fast was
>>> designed to remain lightweight for both non-Xenomai threads as well as
>>> migrated threads.
>>>
>>>> The point is that NULL (XN_NO_HANDLE) means at the same time a freed
>>>> mode and a mode after the TSD cleanup was run. So, emitting a syscall in
>>>> that case is the simplest thing we can do. And no, I did not find it
>>>> expensive. But in fact, using an additional syscall, assert_nrt could be
>>>> implemented as a simple dummy syscall which returns 0 and asks the
>>>> caller to be put in primary mode (and it would be even lighter). So, I
>>>> guess if you did not implemented that way, it means that you already
>>>> wanted to avoid the syscall.
>>> Can't follow on this yet.
>> ...specifically as an unset current key is a bug for a Xenomai thread.
>> Every skin library is supposed to set it, so there should be no need for
>> falling back to a syscall for them, and there is no point in trying it
>> for non-Xenomai threads.
>>
>> So why this fallback? Does it simplify something else that I miss ATM?
> 
> After the TSD cleanup is run, the TSD are set to NULL. Actually, I
> assumed that when doing this change. I need to check.

That is the way I implemented it in the kernel-space skin. But reading
the spec again, I may have been overzealous:
http://www.opengroup.org/onlinepubs/000095399/functions/pthread_key_create.html

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Gilles Chanteperdrix]

Gilles Chanteperdrix wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> So why this fallback? Does it simplify something else that I miss ATM?
>> After the TSD cleanup is run, the TSD are set to NULL. Actually, I
>> assumed that when doing this change. I need to check.
> 
> That is the way I implemented it in the kernel-space skin. But reading
> the spec again, I may have been overzealous:
> http://www.opengroup.org/onlinepubs/000095399/functions/pthread_key_create.html

>From the results of the following test case:

#include <stdio.h>
#include <pthread.h>

pthread_key_t key;
pthread_key_t other_key;

void dest(void *foo)
{
	if ((long)foo == 1) {
		printf("other tsd, self == 0x%016lx, first pass: %p\n",
		       pthread_self(), pthread_getspecific(other_key));
		pthread_setspecific(key, (void *)2L);
	} else
		printf("other tsd, self == 0x%016lx, second pass: %p\n",
		       pthread_self(), pthread_getspecific(other_key));
}

void *thread(void *cookie)
{
	pthread_setspecific(key, (void *)1L);
	pthread_setspecific(other_key, (void *)1L);
	return cookie;
}

int main(void)
{
	pthread_t tid;

	pthread_key_create(&key, dest);
	pthread_key_create(&other_key, NULL);

	pthread_create(&tid, NULL, thread, NULL);
	pthread_join(tid, NULL);
	
	return 0;
}

It appears that the glibc I run here is implemented the same way as I
implemented it in xenomai kernel-space posix skin.

So, we still have this issue.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> xenomai-git-request@domain.hid wrote:
>>>>> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
>>>>> index f0e569c..b9ac680 100644
>>>>> --- a/include/asm-generic/bits/current.h
>>>>> +++ b/include/asm-generic/bits/current.h
>>>> ...
>>>>
>>>>> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>>>>>  {
>>>>>  	void *val = pthread_getspecific(xeno_current_key);
>>>>>  
>>>>> -	if (!val)
>>>>> -		return XN_NO_HANDLE;
>>>>> -
>>>>> -	return (xnhandle_t)val;
>>>>> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>>>>>  }
>>>> So when used with normal Linux threads, this will always trigger the
>>>> syscall of xeno_slow_get_current()?
>>>>
>>>>> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
>>>>> index bad19f3..f67bcd8 100644
>>>>> --- a/src/rtdk/assert_context.c
>>>>> +++ b/src/rtdk/assert_context.c
>>>>> @@ -30,12 +30,9 @@ void assert_nrt(void)
>>>>>  	xnthread_info_t info;
>>>>>  	int err;
>>>>>  
>>>>> -	if (xeno_get_current() != XN_NO_HANDLE)
>>>>> -		return;
>>>>> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
>>>>> +		     !(xeno_get_current_mode() & XNRELAX))) {
>>>> Then please provide a different API for assert_nrt as this makes the
>>>> service too heavy for common use.
>>> It is a non real-time thread. Its performances are not critical. A
>>> non-blocking syscall is not that heavy. Especially compared to the
>>> execution time of malloc. Ok, will not argue on this any more, as
>>> obviously our opinions differ in that area.
>> Sorry, disagree. We are using it in mixed applications where both RT
>> latency as well as non-RT throughput matters. The assert_nrt fast was
>> designed to remain lightweight for both non-Xenomai threads as well as
>> migrated threads.
>>
>>> The point is that NULL (XN_NO_HANDLE) means at the same time a freed
>>> mode and a mode after the TSD cleanup was run. So, emitting a syscall in
>>> that case is the simplest thing we can do. And no, I did not find it
>>> expensive. But in fact, using an additional syscall, assert_nrt could be
>>> implemented as a simple dummy syscall which returns 0 and asks the
>>> caller to be put in primary mode (and it would be even lighter). So, I
>>> guess if you did not implemented that way, it means that you already
>>> wanted to avoid the syscall.
>> Can't follow on this yet.
> 
> ...specifically as an unset current key is a bug for a Xenomai thread.
> Every skin library is supposed to set it, so there should be no need for
> falling back to a syscall for them, and there is no point in trying it
> for non-Xenomai threads.
> 
> So why this fallback? Does it simplify something else that I miss ATM?

So, to summarize, the problem here, is that current == XN_NO_HANDLE may
mean that the current thread is not a real-time thread, or that it is a
real-time thread, but that we are in the TSD cleanup, and the current
TSD was set to 0, as is done for TSD which have no destructor.

We need get_current() to be correct, for the implementation of mutexes,
and in that case we want the additional syscall. We need get_current()
to be correct for clock_gettime(), because as I understand, calling
linux' vdso clock_gettime may deadlock if we are not in secondary mode,
I do not know if you think that using a syscall for clock_gettime over
plain linux threads matters in that case.

For the wrapping of malloc and free, I would say we do not really care
to absolutely get the real current. I do not think that a syscall for
each call to malloc and free is that prohibitive, their execution time
may already been long anyway, and the current implementation is correct.
It is only sub-optimal for your very peculiar case, so, I will let you
sweat on this issue.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> xenomai-git-request@domain.hid wrote:
>>>>>> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
>>>>>> index f0e569c..b9ac680 100644
>>>>>> --- a/include/asm-generic/bits/current.h
>>>>>> +++ b/include/asm-generic/bits/current.h
>>>>> ...
>>>>>
>>>>>> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>>>>>>  {
>>>>>>  	void *val = pthread_getspecific(xeno_current_key);
>>>>>>  
>>>>>> -	if (!val)
>>>>>> -		return XN_NO_HANDLE;
>>>>>> -
>>>>>> -	return (xnhandle_t)val;
>>>>>> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>>>>>>  }
>>>>> So when used with normal Linux threads, this will always trigger the
>>>>> syscall of xeno_slow_get_current()?
>>>>>
>>>>>> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
>>>>>> index bad19f3..f67bcd8 100644
>>>>>> --- a/src/rtdk/assert_context.c
>>>>>> +++ b/src/rtdk/assert_context.c
>>>>>> @@ -30,12 +30,9 @@ void assert_nrt(void)
>>>>>>  	xnthread_info_t info;
>>>>>>  	int err;
>>>>>>  
>>>>>> -	if (xeno_get_current() != XN_NO_HANDLE)
>>>>>> -		return;
>>>>>> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
>>>>>> +		     !(xeno_get_current_mode() & XNRELAX))) {
>>>>> Then please provide a different API for assert_nrt as this makes the
>>>>> service too heavy for common use.
>>>> It is a non real-time thread. Its performances are not critical. A
>>>> non-blocking syscall is not that heavy. Especially compared to the
>>>> execution time of malloc. Ok, will not argue on this any more, as
>>>> obviously our opinions differ in that area.
>>> Sorry, disagree. We are using it in mixed applications where both RT
>>> latency as well as non-RT throughput matters. The assert_nrt fast was
>>> designed to remain lightweight for both non-Xenomai threads as well as
>>> migrated threads.
>>>
>>>> The point is that NULL (XN_NO_HANDLE) means at the same time a freed
>>>> mode and a mode after the TSD cleanup was run. So, emitting a syscall in
>>>> that case is the simplest thing we can do. And no, I did not find it
>>>> expensive. But in fact, using an additional syscall, assert_nrt could be
>>>> implemented as a simple dummy syscall which returns 0 and asks the
>>>> caller to be put in primary mode (and it would be even lighter). So, I
>>>> guess if you did not implemented that way, it means that you already
>>>> wanted to avoid the syscall.
>>> Can't follow on this yet.
>> ...specifically as an unset current key is a bug for a Xenomai thread.
>> Every skin library is supposed to set it, so there should be no need for
>> falling back to a syscall for them, and there is no point in trying it
>> for non-Xenomai threads.
>>
>> So why this fallback? Does it simplify something else that I miss ATM?
> 
> So, to summarize, the problem here, is that current == XN_NO_HANDLE may
> mean that the current thread is not a real-time thread, or that it is a
> real-time thread, but that we are in the TSD cleanup, and the current
> TSD was set to 0, as is done for TSD which have no destructor.
> 
> We need get_current() to be correct, for the implementation of mutexes,
> and in that case we want the additional syscall.

Right, at least for !HAVE___THREAD. For HAVE___THREAD, I don't see a
need for a syscall. Do you?

> We need get_current()
> to be correct for clock_gettime(), because as I understand, calling
> linux' vdso clock_gettime may deadlock if we are not in secondary mode,
> I do not know if you think that using a syscall for clock_gettime over
> plain linux threads matters in that case.
> 
> For the wrapping of malloc and free, I would say we do not really care
> to absolutely get the real current. I do not think that a syscall for
> each call to malloc and free is that prohibitive, their execution time
> may already been long anyway, and the current implementation is correct.
> It is only sub-optimal for your very peculiar case, so, I will let you
> sweat on this issue.

I will post a patch to add a "less-accurate" check for an
assert_nrt_fast variant. As this only reduces accuracy for TSD
destructor contexts, and that only under !HAVE___THREAD, I don't think
it justifies the additional syscall for the majority of use cases of
assert_nrt. But let's make the user choose the accuracy (s)he
explicitly: assert_nrt for always correct results, assert_nrt_fast for
syscall-less checks with known (and to-be-documented) limitations.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [Xenomai-git] common: do not let u_mode exceptional cases leak out of current.[ch]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> xenomai-git-request@domain.hid wrote:
>>>>>>> diff --git a/include/asm-generic/bits/current.h b/include/asm-generic/bits/current.h
>>>>>>> index f0e569c..b9ac680 100644
>>>>>>> --- a/include/asm-generic/bits/current.h
>>>>>>> +++ b/include/asm-generic/bits/current.h
>>>>>> ...
>>>>>>
>>>>>>> @@ -33,25 +33,16 @@ static inline xnhandle_t xeno_get_current(void)
>>>>>>>  {
>>>>>>>  	void *val = pthread_getspecific(xeno_current_key);
>>>>>>>  
>>>>>>> -	if (!val)
>>>>>>> -		return XN_NO_HANDLE;
>>>>>>> -
>>>>>>> -	return (xnhandle_t)val;
>>>>>>> +	return (xnhandle_t)val ?: xeno_slow_get_current();
>>>>>>>  }
>>>>>> So when used with normal Linux threads, this will always trigger the
>>>>>> syscall of xeno_slow_get_current()?
>>>>>>
>>>>>>> diff --git a/src/rtdk/assert_context.c b/src/rtdk/assert_context.c
>>>>>>> index bad19f3..f67bcd8 100644
>>>>>>> --- a/src/rtdk/assert_context.c
>>>>>>> +++ b/src/rtdk/assert_context.c
>>>>>>> @@ -30,12 +30,9 @@ void assert_nrt(void)
>>>>>>>  	xnthread_info_t info;
>>>>>>>  	int err;
>>>>>>>  
>>>>>>> -	if (xeno_get_current() != XN_NO_HANDLE)
>>>>>>> -		return;
>>>>>>> +	if (unlikely(xeno_get_current() != XN_NO_HANDLE &&
>>>>>>> +		     !(xeno_get_current_mode() & XNRELAX))) {
>>>>>> Then please provide a different API for assert_nrt as this makes the
>>>>>> service too heavy for common use.
>>>>> It is a non real-time thread. Its performances are not critical. A
>>>>> non-blocking syscall is not that heavy. Especially compared to the
>>>>> execution time of malloc. Ok, will not argue on this any more, as
>>>>> obviously our opinions differ in that area.
>>>> Sorry, disagree. We are using it in mixed applications where both RT
>>>> latency as well as non-RT throughput matters. The assert_nrt fast was
>>>> designed to remain lightweight for both non-Xenomai threads as well as
>>>> migrated threads.
>>>>
>>>>> The point is that NULL (XN_NO_HANDLE) means at the same time a freed
>>>>> mode and a mode after the TSD cleanup was run. So, emitting a syscall in
>>>>> that case is the simplest thing we can do. And no, I did not find it
>>>>> expensive. But in fact, using an additional syscall, assert_nrt could be
>>>>> implemented as a simple dummy syscall which returns 0 and asks the
>>>>> caller to be put in primary mode (and it would be even lighter). So, I
>>>>> guess if you did not implemented that way, it means that you already
>>>>> wanted to avoid the syscall.
>>>> Can't follow on this yet.
>>> ...specifically as an unset current key is a bug for a Xenomai thread.
>>> Every skin library is supposed to set it, so there should be no need for
>>> falling back to a syscall for them, and there is no point in trying it
>>> for non-Xenomai threads.
>>>
>>> So why this fallback? Does it simplify something else that I miss ATM?
>> So, to summarize, the problem here, is that current == XN_NO_HANDLE may
>> mean that the current thread is not a real-time thread, or that it is a
>> real-time thread, but that we are in the TSD cleanup, and the current
>> TSD was set to 0, as is done for TSD which have no destructor.
>>
>> We need get_current() to be correct, for the implementation of mutexes,
>> and in that case we want the additional syscall.
> 
> Right, at least for !HAVE___THREAD. For HAVE___THREAD, I don't see a
> need for a syscall. Do you?

No, I would think gcc/glibc probably guarantees that no user-space code
is ever executed in a context where the __thread variable has an
unexpected value, that would be silly.

> 
>> We need get_current()
>> to be correct for clock_gettime(), because as I understand, calling
>> linux' vdso clock_gettime may deadlock if we are not in secondary mode,
>> I do not know if you think that using a syscall for clock_gettime over
>> plain linux threads matters in that case.
>>
>> For the wrapping of malloc and free, I would say we do not really care
>> to absolutely get the real current. I do not think that a syscall for
>> each call to malloc and free is that prohibitive, their execution time
>> may already been long anyway, and the current implementation is correct.
>> It is only sub-optimal for your very peculiar case, so, I will let you
>> sweat on this issue.
> 
> I will post a patch to add a "less-accurate" check for an
> assert_nrt_fast variant. As this only reduces accuracy for TSD
> destructor contexts, and that only under !HAVE___THREAD, I don't think
> it justifies the additional syscall for the majority of use cases of
> assert_nrt. But let's make the user choose the accuracy (s)he
> explicitly: assert_nrt for always correct results, assert_nrt_fast for
> syscall-less checks with known (and to-be-documented) limitations.

Ok. But please hide the syscall in current.h, for the case where there
would be other users of the fast mode.

-- 
					    Gilles.