Possible bug in _CORE_mutex_Seize()
joel.sherrill at OARcorp.com
Wed Oct 1 00:34:26 UTC 2003
Till Straumann wrote:
> Joel Sherrill wrote:
>> Phil Torre wrote:
>>> I'm running now with some of the changes suggested by Till and Joel:
>>> 1. "Paranoia" added to _CORE_mutex_Seize() which checks
>>> _Thread_Dispatch_disable_level and throws an internal error if
>>> its nonzero when it shouldn't be.
>> Be careful with this one. I don't think this is the right place to
>> check it. You can attempt to obtain a mutex in an ISR and
>> _Thread_Dispatch_disable_level will definitely be non-zero. You
>> can't use the Allocator Mutex when _Thread_Dispatch_disable_level
>> is non-zero but it is conceivable that you could use another mutex.
> ??? What is a legal scenario to obtain a CORE_mutex in an ISR -
> with _wait == TRUE -- and even if it is FALSE, I doubt it's OK. The ISR
> would lock a mutex for the interrupted task.
You are right in that it is only valid to call it for non-blocking
requests. But the
ISR disable part is OK since _ISR_Disable/_ISR_Enable pairs save the current
ISR mask level and restore it.
> Plus: CORE_mutex_Seize() is always executed with IRQs disabled - it's
> to imagine that it should be legal to call it from an ISR.
That isn't a problem since _ISR_Enable just restores the level from the
It doesn't arbitrarily enable them.
> The simplest scenario goes like this (summarizing):
> if ( !_CORE_mutex_Is_locked( the_mutex ) )
> the_mutex->holder = _Thread_Executing;
> /* set other fields in the_mutex and _Thread_Executing */
> return 0;
> It is *legal* however, to call CORE_mutex_Seize() from a dispatching
> disabled section (in a task context) provided that "_wait == FALSE", a
> which is tolerated by the paranoia check. Of course, the caller is
> to check/convert the ("indirect") return code before proceeding.
Exactly. Sounds like I thought your check was too tight and it isn't.
> I added the check at the top of CORE_mutex_Seize() just *because*
> I'd like to catch other possible misuses of mutexes.
OK. But in general we don't want too tight a check. I t hink you managed
to do that from what I am seeing.
> So far, I caught no other one running a rather complex EPICS application.
That's great to hear.
>> I would take this out.
>>> 2. The reent struct is now allocated from the workspace using
>>> _Workspace_Allocate() from libc_create_hook rather than the start
>>> hook. If _Workspace_Allocate() returns NULL, the create hook
>>> returns false.
>>> This seems to be working (given several minutes of exhaustive testing),
>>> but I notice a few things: Since I'm now calling _Workspace_Allocate()
>>> directly, _RTEMS_Allocator_mutex is not being locked. That should
>>> be OK, because _Thread_Dispatch_disable_level is nonzero, correct?
>> It should be right because _Workspace_Allocate() is a normal part of
>> rtems_task_create(). The stack, FP context, and API extensions
>> are allocated from the Workspace as part of task creation.
More information about the users