[Bug 1876] Add SMP ISR support

[bugzilla-daemon at rtems.org]

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

--- Comment #3 from Jennifer Averett <jennifer.averett at oarcorp.com> 2011-08-03 12:50:59 CDT ---
Sorry, I made my comments before attaching the patch and didn't save them :(


> This is tricky stuff! I tried to make sense of it and have some comments.
Agreed, I have tried to break out anything that seemed reasonable but it
is still difficult to follow.

> src/isrsmp.c:
> Should the name be smpisr.c to be consistent with other smp files? The 
> question
> boils down to whether these new functions are part of the ISR package or the
> SMP package? If the latter, the functions and file should be renamed to 
> reverse
> the ordering. If the former, leave as-is.
It is part of the ISR package so I left it as-is.

> _ISR_Xxx_on_core: Xxx=Disable/Enable/Flash
> name implies you will pass a core to disable? I think add the word this:
> _ISR_Xxx_on_this_core. 
Modified

> _ISR_SMP_Xxx: Xxx=Disable/Enable/Flash
> how about _ISR_Xxx_all_cores ?
No, we don't want to do this modification until the USE_SPINLOCKS code is
fully implemented and debugged.

> honoured: British spelling. American spelling is honored.
Fixed

> The implementation of _ISR_SMP_Xxx (*Disable/Enable/Flash) is confusing,
> because it does not enforce across all cores unless the USE_SPINLOCKS is
> defined---should this define be configurable, or are we leaving it as a hack
> for now?  Since it is commented out, I don't see a reason to add these new
> functions: they do exactly the same thing as the original ones that have now
> been renamed to _ISR_Xxx_on_core.
I removed the USE_SPINLOCKS code and will apply it as a seperate patch.  We
just
didn't want to lose what was done on it so far and had reached a point where
things needed to be merged into the rtems head.

> In places you have switched to calling _ISR_Xxx_on_core explicitly. Please
> double-check these to make sure you intend that only the current core is
> involved (Granted, that is the only supported implementation at present, so 
> I'm
> not sure why the change to make these calls explicit.) and that every change
> to
> _ISR_Disable_on_core is matched with an explicit _ISR_Enable_on_core. If there
> is mismatch, bugs may appear if the USE_SPINLOCKS implementation is ever
> enabled.
Double check the calls.  The entry and exit code may not appear to match but it
is doing exactly what the assembly it was derived from is doing.  The code just
became too much to do in assembly once SMP was added.

> +int _ISR_SMP_Enter(void)
> _ISR_SMP_Exit
> These functions are not exported by a header file and they lack documentation
> or usage.
Exported and documentation added to the header file.

> Should the _ISR_Set_level function be also made per-core or SMP (for-all)?
No.  This would require a level message be passed into each core.  Which is not
supported at the moment.  We felt their was no gain in implementing this at
this time.

> + * Prototypes and structures used int the debug lock/unlock error log.
> s/int the/in the/
Fixed

> cpukit/score/src/threaddispatchdisablelevel.c
> @@ -96,6 +94,8 @@
>      isr_level
>    ); 
> 
> +  _ISR_Enable_on_core(isr_level);
> +
> Shouldn't need this ISR_Enable_on_core, it is already done in the call to
> _SMP_lock_spinlock_nested_Release?

Removed

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