[Bug 1729] SMP Step #2

[bugzilla-daemon at rtems.org]

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

--- Comment #5 from Jennifer Averett <jennifer.averett at oarcorp.com> 2011-01-26 13:12:41 CST ---
I'm not finished with all the mods, but have or will do most of them so I'm
going ahead and replying.  A new patch will follow.

(In reply to comment #4)
> 1. The include file
> #include <bsp/apic.h>
> is missing in the patch and CVS head?

This file is needed when you have real SMP and I should not have included it.

> I disagree with several names in this patch.  The SMP support belongs to the
> Score.  Thus we should use the Score naming conventions.
> 2. rtems_bsp_smp_get_current():  This should be renamed to
> _BSP_smp_processor_id() or simply bsp_smp_processor_id().  That this function
> is placed in the BSP domain is a pity.  Linux manages to determine the
> processor ID on a per architecture basis.
> 3. CONFIGURE_SMP_CPUS: Rename to CONFIGURE_SMP_PROCESSOR_COUNT.
> 4. rtems_smp_maximum_cpus: Rename to _SMP_Processor_count.
> 5. per_cpu_initialize(): Rename to _SMP_Handler_initialize().

Fixed 

> 6. Why do we need the RTEMS_BSP_SMP_CPU_INITIAL_STATE,
> RTEMS_BSP_SMP_CPU_INITIALIZED, and RTEMS_BSP_SMP_CPU_SHUTDOWN states in
> addition to System_state_Codes?  Why are this defines and not an enum?

They reflect the state of secondary CPU's during initilization and shutdown. 
The secondary cores start in reset and need to be brought out of reset in an
orderly manner.  These constants are used so cpu 0 knows what is happening on
cpus 1 .. n.  

System state reflects overall system state not just the state of the secondary
cores. 

This technically may not be necessary as part of this patch, but this or
something similar will be needed in the next patch.

> 7. rtems_lock_t: Where does this come from?  Why this name?  The Score
> generally has no _t names.

It is in score/lock.h.  It is the type for the simplest smp spin lock.  This is
an area that needs improvement as we move forward.  We will have to address
locking at the dispatcher, interrupt, and per cpu data structure level.

> 8. volatile uint32_t state and volatile uint32_t message in Per_CPU_Control:
> Usage of volatile is probably wrong here.  Why do we use uint32_t here?

They are uint32_t's so that the size is easier to calculate in assembly and
don't vary as much across ports.  At least some of the fields may be modified
by one cpu without another knowing.

> 9. int rtems_bsp_smp_get_current(void)  __attribute__ ((pure)): FreeBSD, Linux,
> and RTEMS (sometimes) use defines to avoid hard coded compiler dependencies. 
> We should really consider to avoid all these __attribute__ stuff throughout the
> code.

Still working to fix this.

> 10. _Per_CPU_Information = _Workspace_Allocate_or_fatal_error(): We should
> consider to define this array in confdefs.h and place it in the BSS or
> architecture dependent section (e.g. small data area on PowerPC).

Will look at fixing this.

> 11. We should consider to avoid a lot of these #if defined(RTEMS_SMP) and
> simply define _SMP_Processor_count (rtems_smp_maximum_cpus) to 0 in the non-SMP
> case.  The code size overhead is not huge.

The code size may not be huge but it is too much for low speed and low memory
processors.  We still really don't know the full impact of adding SMP.  True
SMP will have an alternate scheduler which will be even more bulk.

Independient of that the code is incomplete and under development.  By keeping
it to defines it redeces impact on users not interested in SMP.  When finished
this issue is worth revisiting, since we will know what the full impact will
be.

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