Fatal exceptions on context-switching for more than two isolated threads
Gedare Bloom
gedare at rtems.org
Wed Nov 11 17:12:37 UTC 2020
On Tue, Nov 10, 2020 at 8:53 AM Utkarsh Rai <utkarsh.rai60 at gmail.com> wrote:
>
>
>
> On Thu, Nov 5, 2020 at 11:45 AM Sebastian Huber <sebastian.huber at embedded-brains.de> wrote:
>>
>> On 04/11/2020 19:38, Gedare Bloom wrote:
>>
>> >> Based on the above suggestions I tried to move the User_extensions_Iterator storage to the TCB by adding a new field to the structure, but that did not compile(userextimpl.h is not included with thread.h because of cyclic dependency).
>> >> This made me try out a naive hack, I defined a structure similar to the User_extensions_Iterator and then added the field to the TCB. The next problem that I faced was during the creation of the idle thread. Since an idle thread is created during system
>> >> initialization, the 'executing' pointer pointing the TCB is null, and hence referencing to the iterator placed in the TCB for the idle thread fails. This was resolved by separating the cases for an idle thread and other threads. After that I was able to successfully isolate more than two thread stacks.
>> >> Can you please suggest a more acceptable approach for resolving this issue?
>> >>
>> > At a high level the approach you took makes sense. You have moved the
>> > user extensions to the TCB, and then avoid accessing it in case the
>> > TCB is null (i.e., the initial context switch). I'd need to see the
>> > code to make any further critical analysis. But it sounds acceptable
>> > to me.
>> The executing thread pointer can be NULL and there is already a check
>> for this in _User_extensions_Iterate(). In this case you can use a
>> member in the _Per_CPU_Information[].
>>
>> --
>> Sebastian Huber, embedded brains GmbH
>>
>> Address : Dornierstr. 4, D-82178 Puchheim, Germany
>> Phone : +49 89 189 47 41-16
>> Fax : +49 89 189 47 41-09
>> E-Mail : sebastian.huber at embedded-brains.de
>> PGP : Public key available on request.
>>
>> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>>
>
> Based on your comments I have made the following changes in the user extension iterator scheme. Some of the code is still hackish( if-else conditional for handling TCB pointer when it is NULL ) and I would request your suggestions on the same.
>
> diff --git a/cpukit/include/rtems/score/percpu.h b/cpukit/include/rtems/score/percpu.h
> index 31bc2b0bff..6e15f4b1ba 100644
> --- a/cpukit/include/rtems/score/percpu.h
> +++ b/cpukit/include/rtems/score/percpu.h
> @@ -25,7 +25,7 @@
> #include <rtems/asm.h>
> #else
> #include <rtems/score/assert.h>
> - #include <rtems/score/chain.h>
> + #include <rtems/score/chainimpl.h>
> #include <rtems/score/isrlock.h>
> #include <rtems/score/smp.h>
> #include <rtems/score/timestamp.h>
> @@ -339,6 +339,20 @@ typedef enum {
> PER_CPU_WATCHDOG_COUNT
> } Per_CPU_Watchdog_index;
>
> +#if defined ( RTEMS_THREAD_STACK_PROTECTION )
you don't need parens here
> + /**
> + * @brief Per CPU user extensions iterator structure
> + *
> + * This structure is used to refer to the user extensions iterator when
> + * thread stack protection is configured.
> + */
> +typedef struct Per_CPU_User_extensions_Iterator {
> + Chain_Iterator Iterator;
> + struct Per_CPU_User_extensions_Iterator *previous;
> +} Per_CPU_User_extensions_Iterator;
> +#endif
Does it make sense just to use this approach whether or not thread
stack protection is in place?
You might also be able to forward declare this type. I'm not sure if
the compiler will let you do that with a recursive structure though.
struct Per_CPU_User_extensions_Iterator;
> +
> +
> /**
> * @brief Per CPU Core Structure
> *
> @@ -595,6 +609,10 @@ typedef struct Per_CPU_Control {
> bool boot;
> #endif
>
> + #if defined (RTEMS_THREAD_STACK_PROTECTION)
Something like this is still hackish but might work:
struct Per_CPU_User_extensions_Iterator {
Chain_Iterator Iterator;
struct Per_CPU_User_extensions_Iterator *previous;
} Iter;
I'm not sure if the compiler will let you do this. You might instead
use an anonymous struct in the definition here, like this:
struct {
Chain_Iterator Iterator;
void *previous;
} Iter;
then you'll need to cast Iter.previous when you use it, but this
should at least be workable.
> + Per_CPU_User_extensions_Iterator iter;
}
Another minor note: a fully instantiated struct should have a capital
letter in the name, e.g.,
Per_CPU_User_extensions_Iterator Iter;
whereas a pointer is lower-case.
> + #endif
> +
> struct Record_Control *record;
>
> Per_CPU_Stats Stats;
> diff --git a/cpukit/include/rtems/score/thread.h b/cpukit/include/rtems/score/thread.h
> index ee0aee5b79..98b85a66af 100644
> --- a/cpukit/include/rtems/score/thread.h
> +++ b/cpukit/include/rtems/score/thread.h
> @@ -636,6 +636,13 @@ struct Thread_Action {
> Thread_Action_handler handler;
> };
>
> +#if defined (RTEMS_THREAD_STACK_PROTECTION)
> + typedef struct Thread_User_extensions_Iterator {
> + Chain_Iterator Iterator;
> + struct Thread_User_extensions_Iterator *previous;
> + } Thread_User_extensions_Iterator;
This type is almost identical to the Per_CPU one. You should try to
unify them. If you end up using void* instead, then they can be the
same struct.
You could still 'specialize' them in the Iterator code by casting the
struct type, if needed.
> +#endif
> +
> /**
> * @brief Per-thread information for POSIX Keys.
> */
> @@ -864,6 +871,10 @@ struct _Thread_Control {
> */
> struct _pthread_cleanup_context *last_cleanup_context;
>
> +#if defined (RTEMS_THREAD_STACK_PROTECTION)
> + Thread_User_extensions_Iterator iter;
> +#endif
> +
> /**
> * @brief LIFO list of user extensions iterators.
> */
> diff --git a/cpukit/score/src/userextiterate.c b/cpukit/score/src/userextiterate.c
> index 06665a2d7a..814e695018 100644
> --- a/cpukit/score/src/userextiterate.c
> +++ b/cpukit/score/src/userextiterate.c
> @@ -181,22 +181,47 @@ void _User_extensions_Iterate(
>
> _User_extensions_Acquire( &lock_context );
>
> - _Chain_Iterator_initialize(
> + if ( executing != NULL ) {
> + executing->iter.previous = executing->last_user_extensions_iterator;
> + executing->last_user_extensions_iterator = &executing->iter;
> +
> + _Chain_Iterator_initialize(
> &_User_extensions_List.Active,
> &_User_extensions_List.Iterators,
> - &iter.Iterator,
> + &executing->iter.Iterator,
> direction
> );
>
> - if ( executing != NULL ) {
> - iter.previous = executing->last_user_extensions_iterator;
> - executing->last_user_extensions_iterator = &iter;
> + while ( ( node = _Chain_Iterator_next( &executing->iter.Iterator ) ) != end ) {
> + const User_extensions_Control *extension;
> +
> + _Chain_Iterator_set_position( &executing->iter.Iterator, node );
> +
> + _User_extensions_Release( &lock_context );
> +
> + extension = (const User_extensions_Control *) node;
> + ( *visitor )( executing, arg, &extension->Callouts );
> +
> + _User_extensions_Acquire( &lock_context );
> }
>
> - while ( ( node = _Chain_Iterator_next( &iter.Iterator ) ) != end ) {
> + executing->last_user_extensions_iterator = executing->iter.previous;
> +
> + _Chain_Iterator_destroy( &executing->iter.Iterator );
> +
> + } else {
> +
> + _Chain_Iterator_initialize(
> + &_User_extensions_List.Active,
> + &_User_extensions_List.Iterators,
> + &_Per_CPU_Information[0].per_cpu.iter.Iterator,
> + direction
> + );
> +
> + while ( ( node = _Chain_Iterator_next( &_Per_CPU_Information[0].per_cpu.iter.Iterator ) ) != end ) {
> const User_extensions_Control *extension;
>
> - _Chain_Iterator_set_position( &iter.Iterator, node );
> + _Chain_Iterator_set_position( &_Per_CPU_Information[0].per_cpu.iter.Iterator, node );
>
> _User_extensions_Release( &lock_context );
>
> @@ -206,11 +231,9 @@ void _User_extensions_Iterate(
> _User_extensions_Acquire( &lock_context );
> }
>
> - if ( executing != NULL ) {
> - executing->last_user_extensions_iterator = iter.previous;
> - }
> + _Chain_Iterator_destroy( &_Per_CPU_Information->per_cpu.iter.Iterator );
>
> - _Chain_Iterator_destroy( &iter.Iterator );
> + }
>
> _User_extensions_Release( &lock_context );
More information about the devel
mailing list