Hello,
on single processor systems the ISR disable/enable was the big hammer which
ensured system-wide mutual exclusion. On SMP configurations this no longer
works since other processors don't care about disable interrupts on this
processor and continue to execute freely.
On SMP in addition to ISR disable/enable an SMP lock must be used. For
compatibility with single processor configurations I introduced the ISR locks.
http://www.rtems.org/onlinedocs/doxygen/cpukit/html/group__ClassicINTRLocks.html
They are currently used for the TOD manager, Termios and the file system
support.
We need a general review of the RTEMS code base and evaluate every usage of ISR
disable/enable. It helps to add proper _Assert() statements to catch the hot
spots (attached is an example).
This is a high priority issue, since correctness of the system depends on this.
This makes SMP and RTEMS currently like playing Russian roulette.
One big issue are the RTEMS chains:
http://www.rtems.org/onlinedocs/doxygen/cpukit/html/group__ClassicChains.html
To make them SMP ready we have two options.
1. Instead of ISR disable/enable use ISR locks. Advantage: not interface
change. Disadvantage: space overhead for user which only use the unprotected
routines.
2. Add a new chain API, e.g. new structure rtems_chain_with_lock which provides
a chain + ISR lock. Advantage: you get what you need. Disadvantage: API
duplication.
--
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.hu...@embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>From 2907fa9f385add38988db4ead669b7822ab6ff08 Mon Sep 17 00:00:00 2001
From: Sebastian Huber <sebastian.hu...@embedded-brains.de>
Date: Wed, 14 Aug 2013 11:09:37 +0200
Subject: [PATCH] FIXME: Add and use _Assert_Owner_of_giant()
---
c/src/lib/libbsp/shared/bootcard.c | 2 +-
cpukit/rtems/include/rtems/rtems/intr.h | 6 ++++
cpukit/rtems/src/taskmode.c | 4 +-
cpukit/sapi/src/exinit.c | 9 +++++-
cpukit/score/include/rtems/score/assert.h | 9 ++++++
cpukit/score/include/rtems/score/isr.h | 4 +-
cpukit/score/include/rtems/score/isrlevel.h | 16 +++++++++++
cpukit/score/include/rtems/score/thread.h | 4 +-
cpukit/score/include/rtems/score/threaddispatch.h | 4 +-
cpukit/score/src/threaddispatch.c | 4 +-
cpukit/score/src/threaddispatchdisablelevel.c | 29 +++++++++++++-------
11 files changed, 68 insertions(+), 23 deletions(-)
diff --git a/c/src/lib/libbsp/shared/bootcard.c b/c/src/lib/libbsp/shared/bootcard.c
index 81ef70e..883058b 100644
--- a/c/src/lib/libbsp/shared/bootcard.c
+++ b/c/src/lib/libbsp/shared/bootcard.c
@@ -86,7 +86,7 @@ void boot_card(
* Make sure interrupts are disabled.
*/
(void) bsp_isr_level;
- rtems_interrupt_disable( bsp_isr_level );
+ rtems_interrupt_disable_without_giant( bsp_isr_level );
bsp_boot_cmdline = cmdline;
diff --git a/cpukit/rtems/include/rtems/rtems/intr.h b/cpukit/rtems/include/rtems/rtems/intr.h
index f80afc1..66a8a7c 100644
--- a/cpukit/rtems/include/rtems/rtems/intr.h
+++ b/cpukit/rtems/include/rtems/rtems/intr.h
@@ -88,6 +88,12 @@ rtems_status_code rtems_interrupt_catch(
);
#endif
+#define rtems_interrupt_disable_without_giant( _isr_cookie ) \
+ _ISR_Disable_without_giant(_isr_cookie)
+
+#define rtems_interrupt_enable_without_giant( _isr_cookie ) \
+ _ISR_Enable_without_giant(_isr_cookie)
+
/**
* @brief Disable RTEMS Interrupt
*
diff --git a/cpukit/rtems/src/taskmode.c b/cpukit/rtems/src/taskmode.c
index 606366b..8650ccc 100644
--- a/cpukit/rtems/src/taskmode.c
+++ b/cpukit/rtems/src/taskmode.c
@@ -39,7 +39,7 @@ static void _RTEMS_Tasks_Dispatch_if_necessary(
#if defined( RTEMS_SMP )
ISR_Level level;
- _ISR_Disable( level );
+ _ISR_Disable_without_giant( level );
#endif
if ( !_Thread_Is_heir( executing ) && executing->is_preemptible ) {
@@ -48,7 +48,7 @@ static void _RTEMS_Tasks_Dispatch_if_necessary(
}
#if defined( RTEMS_SMP )
- _ISR_Enable( level );
+ _ISR_Enable_without_giant( level );
#endif
if ( dispatch_necessary ) {
diff --git a/cpukit/sapi/src/exinit.c b/cpukit/sapi/src/exinit.c
index 10c1559..9cc64ad 100644
--- a/cpukit/sapi/src/exinit.c
+++ b/cpukit/sapi/src/exinit.c
@@ -112,10 +112,14 @@ void rtems_initialize_data_structures(void)
*/
_Debug_Manager_initialization();
- _API_extensions_Initialization();
-
_Thread_Dispatch_initialization();
+ #if defined(RTEMS_SMP)
+ _Giant_Acquire();
+ #endif
+
+ _API_extensions_Initialization();
+
_User_extensions_Handler_initialization();
_ISR_Handler_initialization();
@@ -220,6 +224,7 @@ void rtems_initialize_start_multitasking(void)
_System_state_Set( SYSTEM_STATE_UP );
#if defined(RTEMS_SMP)
+ _Giant_Release();
_SMP_Request_other_cores_to_perform_first_context_switch();
#endif
diff --git a/cpukit/score/include/rtems/score/assert.h b/cpukit/score/include/rtems/score/assert.h
index 174676e..4856eae 100644
--- a/cpukit/score/include/rtems/score/assert.h
+++ b/cpukit/score/include/rtems/score/assert.h
@@ -50,6 +50,15 @@ extern "C" {
#define _Assert_Thread_dispatching_repressed() ( ( void ) 0 )
#endif
+/**
+ * @brief Asserts that current thread of execution owns the giant lock.
+ */
+#if defined( RTEMS_DEBUG ) && defined( RTEMS_SMP )
+ void _Assert_Owner_of_giant( void );
+#else
+ #define _Assert_Owner_of_giant() ( ( void ) 0 )
+#endif
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
diff --git a/cpukit/score/include/rtems/score/isr.h b/cpukit/score/include/rtems/score/isr.h
index 4d4a5f3..fa0052f 100644
--- a/cpukit/score/include/rtems/score/isr.h
+++ b/cpukit/score/include/rtems/score/isr.h
@@ -163,13 +163,13 @@ RTEMS_INLINE_ROUTINE uint32_t _ISR_Get_nest_level( void )
#if defined( RTEMS_SMP )
ISR_Level level;
- _ISR_Disable( level );
+ _ISR_Disable_without_giant( level );
#endif
isr_nest_level = _ISR_Nest_level;
#if defined( RTEMS_SMP )
- _ISR_Enable( level );
+ _ISR_Enable_without_giant( level );
#endif
return isr_nest_level;
diff --git a/cpukit/score/include/rtems/score/isrlevel.h b/cpukit/score/include/rtems/score/isrlevel.h
index 35d3d03..bfa7ab5 100644
--- a/cpukit/score/include/rtems/score/isrlevel.h
+++ b/cpukit/score/include/rtems/score/isrlevel.h
@@ -20,6 +20,7 @@
#define _RTEMS_SCORE_ISR_LEVEL_h
#include <rtems/score/cpu.h>
+#include <rtems/score/assert.h>
#ifdef __cplusplus
extern "C" {
@@ -40,6 +41,18 @@ extern "C" {
*/
typedef uint32_t ISR_Level;
+#define _ISR_Disable_without_giant( _level ) \
+ do { \
+ _CPU_ISR_Disable( _level ); \
+ RTEMS_COMPILER_MEMORY_BARRIER(); \
+ } while (0)
+
+#define _ISR_Enable_without_giant( _level ) \
+ do { \
+ RTEMS_COMPILER_MEMORY_BARRIER(); \
+ _CPU_ISR_Enable( _level ); \
+ } while (0)
+
/**
* @brief Disables interrupts on this processor.
*
@@ -57,6 +70,7 @@ typedef uint32_t ISR_Level;
#define _ISR_Disable( _level ) \
do { \
_CPU_ISR_Disable( _level ); \
+ _Assert_Owner_of_giant(); \
RTEMS_COMPILER_MEMORY_BARRIER(); \
} while (0)
@@ -74,6 +88,7 @@ typedef uint32_t ISR_Level;
#define _ISR_Enable( _level ) \
do { \
RTEMS_COMPILER_MEMORY_BARRIER(); \
+ _Assert_Owner_of_giant(); \
_CPU_ISR_Enable( _level ); \
} while (0)
@@ -99,6 +114,7 @@ typedef uint32_t ISR_Level;
#define _ISR_Flash( _level ) \
do { \
RTEMS_COMPILER_MEMORY_BARRIER(); \
+ _Assert_Owner_of_giant(); \
_CPU_ISR_Flash( _level ); \
RTEMS_COMPILER_MEMORY_BARRIER(); \
} while (0)
diff --git a/cpukit/score/include/rtems/score/thread.h b/cpukit/score/include/rtems/score/thread.h
index 77f1d7a..33b5244 100644
--- a/cpukit/score/include/rtems/score/thread.h
+++ b/cpukit/score/include/rtems/score/thread.h
@@ -495,13 +495,13 @@ RTEMS_INLINE_ROUTINE Thread_Control *_Thread_Get_executing( void )
#if defined( RTEMS_SMP )
ISR_Level level;
- _ISR_Disable( level );
+ _ISR_Disable_without_giant( level );
#endif
executing = _Thread_Executing;
#if defined( RTEMS_SMP )
- _ISR_Enable( level );
+ _ISR_Enable_without_giant( level );
#endif
return executing;
diff --git a/cpukit/score/include/rtems/score/threaddispatch.h b/cpukit/score/include/rtems/score/threaddispatch.h
index 9cd1287..2211662 100644
--- a/cpukit/score/include/rtems/score/threaddispatch.h
+++ b/cpukit/score/include/rtems/score/threaddispatch.h
@@ -54,13 +54,13 @@ RTEMS_INLINE_ROUTINE bool _Thread_Dispatch_is_enabled(void)
#if defined(RTEMS_SMP)
ISR_Level level;
- _ISR_Disable( level );
+ _ISR_Disable_without_giant( level );
#endif
enabled = _Thread_Dispatch_disable_level == 0;
#if defined(RTEMS_SMP)
- _ISR_Enable( level );
+ _ISR_Enable_without_giant( level );
#endif
return enabled;
diff --git a/cpukit/score/src/threaddispatch.c b/cpukit/score/src/threaddispatch.c
index 3b5fb42..0c8298f 100644
--- a/cpukit/score/src/threaddispatch.c
+++ b/cpukit/score/src/threaddispatch.c
@@ -35,7 +35,7 @@ void _Thread_Dispatch( void )
ISR_Level level;
#if defined( RTEMS_SMP )
- _ISR_Disable( level );
+ _ISR_Disable_without_giant( level );
#endif
per_cpu = _Per_CPU_Get();
@@ -43,7 +43,7 @@ void _Thread_Dispatch( void )
per_cpu->thread_dispatch_disable_level = 1;
#if defined( RTEMS_SMP )
- _ISR_Enable( level );
+ _ISR_Enable_without_giant( level );
#endif
/*
diff --git a/cpukit/score/src/threaddispatchdisablelevel.c b/cpukit/score/src/threaddispatchdisablelevel.c
index 50af367..95888e1 100644
--- a/cpukit/score/src/threaddispatchdisablelevel.c
+++ b/cpukit/score/src/threaddispatchdisablelevel.c
@@ -64,7 +64,7 @@ uint32_t _Thread_Dispatch_increment_disable_level( void )
uint32_t disable_level;
Per_CPU_Control *self_cpu;
- _ISR_Disable( isr_level );
+ _ISR_Disable_without_giant( isr_level );
/*
* We must obtain the processor ID after interrupts are disabled to prevent
@@ -79,7 +79,7 @@ uint32_t _Thread_Dispatch_increment_disable_level( void )
++disable_level;
self_cpu->thread_dispatch_disable_level = disable_level;
- _ISR_Enable( isr_level );
+ _ISR_Enable_without_giant( isr_level );
return disable_level;
}
@@ -90,7 +90,7 @@ uint32_t _Thread_Dispatch_decrement_disable_level( void )
uint32_t disable_level;
Per_CPU_Control *self_cpu;
- _ISR_Disable( isr_level );
+ _ISR_Disable_without_giant( isr_level );
self_cpu = _Per_CPU_Get();
disable_level = self_cpu->thread_dispatch_disable_level;
@@ -99,7 +99,7 @@ uint32_t _Thread_Dispatch_decrement_disable_level( void )
_Giant_Do_release();
- _ISR_Enable( isr_level );
+ _ISR_Enable_without_giant( isr_level );
return disable_level;
}
@@ -119,9 +119,9 @@ uint32_t _Thread_Dispatch_set_disable_level(uint32_t value)
ISR_Level isr_level;
uint32_t disable_level;
- _ISR_Disable( isr_level );
+ _ISR_Disable_without_giant( isr_level );
disable_level = _Thread_Dispatch_disable_level;
- _ISR_Enable( isr_level );
+ _ISR_Enable_without_giant( isr_level );
/*
* If we need the dispatch level to go higher
@@ -148,18 +148,27 @@ void _Giant_Acquire( void )
{
ISR_Level isr_level;
- _ISR_Disable( isr_level );
+ _ISR_Disable_without_giant( isr_level );
_Assert( _Thread_Dispatch_disable_level != 0 );
_Giant_Do_acquire( _SMP_Get_current_processor() );
- _ISR_Enable( isr_level );
+ _ISR_Enable_without_giant( isr_level );
}
void _Giant_Release( void )
{
ISR_Level isr_level;
- _ISR_Disable( isr_level );
+ _ISR_Disable_without_giant( isr_level );
_Assert( _Thread_Dispatch_disable_level != 0 );
_Giant_Do_release();
- _ISR_Enable( isr_level );
+ _ISR_Enable_without_giant( isr_level );
}
+
+#if defined( RTEMS_DEBUG )
+void _Assert_Owner_of_giant( void )
+{
+ Giant_Control *giant = &_Giant;
+
+ _Assert( giant->owner_cpu == _SMP_Get_current_processor() );
+}
+#endif
--
1.7.7
_______________________________________________
rtems-devel mailing list
rtems-devel@rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
