[Bug 1791] DispatchDisableLevel is not smp safe

[bugzilla-daemon at rtems.org]

https://www.rtems.org/bugzilla/show_bug.cgi?id=1791

--- Comment #4 from Gedare <giddyup44 at yahoo.com> 2011-05-19 09:35:36 CDT ---
(In reply to comment #2)
> I'll catch some of these and let jennifer catch the rest.
> 
> (In reply to comment #1)
> > +   * The following declares the a smp spinlock to be used to control
> > typo: s/the a/a
> > 
fix this :)

> > +  SCORE_EXTERN SMP_lock_spinlock_nested_Control
> > _Thread_Dispatch_smp_spin_lock;
> > I'm not sold on the name, unless this is the only lock that will be associated
> > with any _Thread_Dispatch handler.  We should develop a consistent convention
> > for lock names.
> 
> Agree on naming convention.  But for now, there are only two planned.  One for
> dispatch disable critical sections and one for interrupt disable critical
> sections.
> 
> I am implementing the latter now.  So suggestions appreciated.
> 
I suggest the name "_Thread_Dispatch_disable_level_lock". _lock makes it clear
what the lock is intended to protect.

> > cpukit/score/inline/rtems/score/thread.inl:
> > Function prototypes should be in .h files. Why aren't these functions inline in
> > the SMP version?
> 
> Too complicated to inline.
> 
OK. Should the function prototypes be moved to the appropriate .h file?

> > _Thread_Dispatch_set_disable_level(): This function confuses me. Why looping up
> > and then down?
> 
> Since you lock/unlock as you go up and down, Jennifer thought this was the
> safest level.  I don't know how many places it is actually used and would like
> to analyse each one.  It is a replacement for direct assignments of a disable
> level to the variable.  Makes one wonder if it could be enable/disable or
> initialize in places.
initialize is using the set routine. It seems heavy to use a loop. For set(n)
this code acquires a nested spinlock 'n' times. It would be more efficient to
acquire the lock once, set its nesting depth, and set the disable level. This
is an optimization that would need to be measured though, and the nested
spinlock API would need to support assigning its depth.

I would like some comments added to explain briefly the increment, decrement,
and set functions, specifically the assumptions on entry/exit about the state
of the lock and ISRs, and the reason for looping in set. It took me a few
minutes to figure these things out by inspection of code.

-- 
Configure bugmail: https://www.rtems.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.