[Bug 1729] SMP Step #2

[bugzilla-daemon at rtems.org]

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

Gedare <giddyup44 at yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |giddyup44 at yahoo.com

--- Comment #6 from Gedare <giddyup44 at yahoo.com> 2011-01-26 14:37:41 CST ---
(In reply to comment #5)
> 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.
Agree that these functions are probably mis-placed.  It seems like multilib'd
score/cpu should work in theory. Putting features in BSP should be justified by
claims that the features rely on board features/quirks and not
architecture-family generic functionality.  Granted, it may be easier to hack
the support into the BSP, the right thing to do seems to be to make the score
SMP-aware and try to keep the BSPs simple.

> > 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.  
> 
This didn't answer the question of why not an enum, and why not to extend
System_state_Codes.

> 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.
> 
I can't find the definition of rtems_lock_t -- is the file missing from the
patch / tar?

IF there will be multiple types of locks, this should be more concretely named
and also named consistent to score naming conventions. Perhaps an SMP handler?
Then naming would be something like SMP_Spin_lock() for example. 

Is there a hard-and-fast rule available for use of _t in typedefs? There are a
few instances in the score (Internal_errors_t, Thread_CPU_usage_t), if they
aren't wanted they should probably be fixed.

> > 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.
>
Will the fields by written/read by CPUs other than the 'owning' CPU of the
array indexed by the CPU ID?  If so, I think this violates the intent of "per
cpu", and further shouldn't be "guarded" by volatile but instead should be
protected by appropriate locking mechanism (which should already have a
compiler barrier included to avoid optimization-related issues).

> > 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
Can you quantify this statement (how much is too much)?

> 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.
>
Yes, but that scheduler will be in isolated files and only add to confdefs.h,
which is allowed liberal use of #ifdefs.  Score files shouldn't be littered
with #ifdefs, if possible.

> 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.
The only justification I can think to avoid reworking these #ifdefs is to
assert that uniprocessor performance not be affected at all by RTEMS support
for SMP. Even then, all of these ifdefs make code less readable.

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