[Bug 1729] SMP Step #2

Wed Jan 26 21:14:14 UTC 2011

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

--- Comment #7 from Joel Sherrill <joel.sherrill at oarcorp.com> 2011-01-26 15:14:13 CST ---
(In reply to comment #6)
> (In reply to comment #5)

> > > 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.

We have only investigated two architectures -- SPARC and x86.  For x86, it is
part of the APIC and might be able to be in cpukit EXCEPT that it is completely
orthogonal to the CPU core features multilib is allowed to test.  If you turn
on SMP and build say i386ex, the read will likely fail.

On the SPARC, cpukit only knows v7/v8 with and without FPU.  I am pretty sure
the SMP register is LEON specific -- not defined by the SPARC architecture.

That's really the rub -- how to read the CPU core number  so far is defined
outside the definition of what features the CPU core itself has.

> > > 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.

It probably could be an enum if only used from C.

It doesn't fit into the System_state_Codes because it is separate from that.
Those are for the system as a whole -- not the individual state of a core
as the primary CPU initializes the system.  The primary CPU uses that value to
know when to wait for each secondary core to reach a certain point of
initialization.  

The system is initializing but the individual CPU cores are in different
states.

> > 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. 

That's a better name.

> 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.

Just old and new styles getting mixed. You are right we should be consistent.
If you want to open another PR and submit a patch, it is welcomed.

> > > 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).

At this point, we anticipate heir will written by whichever CPU decides
a switch is necessary on that core.

You are right.  If the SMP spin lock guards this and is a memory barrier, that
should be sufficient.

> > > 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.

Agreed.  Size and speed appear to be negatively impacted by enabling SMP.

I would like to revisit this when the code is more complete.  If size and
speed are negatively impacted in the complete version, then SMP stays 
conditional.

On the readability.  We haven't looked at providing empty macros for SMP
support calls when SMP is disabled.  This would probably clean up some of
the conditional.  

I haven't looked recently but there was a configuration option on the Linux
kernel to disable SMP at one point.  Is it still there?

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