[Bug 1729] SMP Step #2

[bugzilla-daemon at rtems.org]

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

--- Comment #15 from Gedare <giddyup44 at yahoo.com> 2011-02-02 13:18:47 CST ---
Some more detailed comments.

* c/src/lib/libbsp/shared/smp/smp_stub.c
+#include <rtems/bspsmp.h>
+#include <stdlib.h>
+
+#include <rtems/bspsmp.h>
double include.

* Why move sparc/cpu_irq.S into libbsp/sparc/shared?

* cpukit/score/cpu/sparc/cpu_asm.S: _CPU_Context_switch_to_first_task_smp

The comment states it may not be safe to flush windows, but proceeds to issue a
window save. Is this considered safe, or should the window save be done after
setting up the WIM and before branching to done_flushing?

* cpukit/score/include/rtems/bspsmp.h

why are some functions prefaced with rtems_bsp_smp_xx, others with bsp_smp_xxx

typos: will reschedule on this this CPU -- "this" repeated twice. Happens in
multiple lines in bspsmp.h

void bsp_smp_broadcast_message(): should this function issue IPIs to EVERY cpu?
 including the originating cpu?

What is the difference between bsp_smp_broadcast_message() and
bsp_smp_interrupt_cpu()?

Is it usual to have such an interface defined in the score that relies so
heavily on BSP functions? Can we extend the existing uni-processor interfaces
to support the notion of cpu id?  Are these functions implemented per-BSP, or
generically at the SCORE level? If generically, they should probably belong to
an SMP handler?


* cpukit/score/include/rtems/score/percpu.h

+    /** This element is used to lock this structure */
+    SMP_lock_control  Lock;
Why capital L in Lock? This is inconsistent with structure field names.

Do we know that uint32_t is the best choice of data size for IPI payloads
(state, message) and lock_control? Or should these be BSP-dependent types?

+/**
+ *  @brief Initialize SMP Handler
+ *
+ *  This method initialize the SMP Handler.
+ */
+void _SMP_Handler_initialize(void);
+

Are there multiple handlers defined in this header file? Should there be a
separate SMP handler that instantiates all of these SMP-related files?

It seems a lot of the #ifdef's can be eliminated by defining the uni-processor
RTEMS as an SMP system with 1 CPU having id=0. Then the same arrays can be used
and identical initializing and updating code. This simplify a lot of other
files, such as confdefs.h, threadcreateidle.c, and probably others.


* cpukit/score/include/rtems/score/smplock.h

Should it be SMP_Lock_control, or SMP_lock_Control, or SMP_lock_control?  And I
previously mentioned the volatile keyword.

* cpukit/score/src/smp.c
+  /*
+   *  This is definitely a hack until we have SMP scheduling.  Since there
+   *  is only one executing and heir right now, we have to fake this out.
+   */
Can you identify what will go here when there is a proper scheduler?

+  /*
+   *  HACK: Should not have to enable interrupts in real system here.
+   *        It should happen as part of switching to the first task.
+   */
Will this be moved somewhere else then?

rtems_smp_process_interrupt(): 
+    while(1)
+      ;
+    /* does not continue past here */
Is there a better way to do this, like a BSP-defined "halt"? This will make
sense for example on platforms with the ability to shutdown cores for reducing
power.

+    printk( "switch needed\n" );
Debug statement?

+    if ( cpu == dest_cpu )
+      continue;
I guess this answers the question about broadcasting to the source. This should
be clearly documented.

+  bsp_smp_broadcast_interrupt();
Might be better to notify while sending the message? Acquiring locks can be
quite expensive.

* cpukit/score/src/smplock.c
This implements a spin lock. There are many types of locks (e.g. R/W locks,
RCU, and so on). The naming should be more precise and forward compatible in
case additional locks are implemented.

* cpukit/score/src/thread.c:
-/*PAGE
- *
...
-#if defined(RTEMS_MULTIPROCESSING)
-  _Thread_MP_Handler_initialization( maximum_proxies );
-#endif
+  #if defined(RTEMS_MULTIPROCESSING)
+    _Thread_MP_Handler_initialization( maximum_proxies );
+  #endif
These syntactical fixes are distracting. Just a nit for the future.

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