Re: Stalled IPI processing on octeon

2015-07-15 Thread Visa Hankala 
On Wed, Jul 15, 2015 at 05:07:39AM +, Miod Vallat wrote:
> > The patch below solves stalled IPI processing on octeon. When IPIs are
> > finally enabled during boot, some kernel threads have already been
> > started. There seems to be no mechanism that would update interrupt
> > masks for a running thread, so the early threads run IPIs disabled.
> > This will lead to a deadlock quite soon after launching other cores.
> > The patch makes the registering of the IPI handler happen early enough
> > for the correct idle_mask to propagate to all threads.
> 
> This makes sense. However, your `ipi_enabled' new variable basically
> mimics an `if (ipi_handler != NULL)' test, so there is no need for an
> extra variable.

Here is a revised patch.


Index: arch/octeon/octeon/machdep.c
===
RCS file: src/sys/arch/octeon/octeon/machdep.c,v
retrieving revision 1.64
diff -u -p -r1.64 machdep.c
--- arch/octeon/octeon/machdep.c25 Jun 2015 10:56:00 -  1.64
+++ arch/octeon/octeon/machdep.c15 Jul 2015 13:21:02 -
@@ -128,6 +128,10 @@ intocteon_cpuspeed(int *);
 static voidprocess_bootargs(void);
 static uint64_tget_ncpusfound(void);
 
+#ifdef MULTIPROCESSOR
+uint32_t   ipi_intr(uint32_t, struct trap_frame *);
+#endif
+
 extern voidparse_uboot_root(void);
 
 cons_decl(cn30xxuart);
@@ -487,6 +491,10 @@ mips_init(__register_t a0, __register_t 
Debugger();
 #endif
 
+#ifdef MULTIPROCESSOR
+   set_intr(INTPRI_IPI, CR_INT_1, ipi_intr);
+#endif
+
/*
 * Return the new kernel stack pointer.
 */
@@ -771,8 +779,6 @@ uint32_t cpu_spinup_mask = 0;
 uint64_t cpu_spinup_a0, cpu_spinup_sp;
 static int (*ipi_handler)(void *);
 
-uint32_t ipi_intr(uint32_t, struct trap_frame *);
-
 extern bus_space_t iobus_tag;
 extern bus_space_handle_t iobus_h;
 
@@ -852,6 +858,9 @@ ipi_intr(uint32_t hwpend, struct trap_fr
 */
bus_space_write_8(&iobus_tag, iobus_h, CIU_IP3_EN0(cpuid), 0);
 
+   if (ipi_handler == NULL)
+   return hwpend;
+
ipi_handler((void *)cpuid);
 
/*
@@ -872,7 +881,6 @@ hw_ipi_intr_establish(int (*func)(void *
0x);
bus_space_write_8(&iobus_tag, iobus_h, CIU_IP3_EN0(cpuid),
(1ULL << CIU_INT_MBOX0)|(1ULL << CIU_INT_MBOX1));
-   set_intr(INTPRI_IPI, CR_INT_1, ipi_intr);
 
return 0;
 };

Re: Stalled IPI processing on octeon

2015-07-14 Thread Miod Vallat 
> The patch below solves stalled IPI processing on octeon. When IPIs are
> finally enabled during boot, some kernel threads have already been
> started. There seems to be no mechanism that would update interrupt
> masks for a running thread, so the early threads run IPIs disabled.
> This will lead to a deadlock quite soon after launching other cores.
> The patch makes the registering of the IPI handler happen early enough
> for the correct idle_mask to propagate to all threads.

This makes sense. However, your `ipi_enabled' new variable basically
mimics an `if (ipi_handler != NULL)' test, so there is no need for an
extra variable.

Miod

Stalled IPI processing on octeon

2015-07-14 Thread Visa Hankala 
The patch below solves stalled IPI processing on octeon. When IPIs are
finally enabled during boot, some kernel threads have already been
started. There seems to be no mechanism that would update interrupt
masks for a running thread, so the early threads run IPIs disabled.
This will lead to a deadlock quite soon after launching other cores.
The patch makes the registering of the IPI handler happen early enough
for the correct idle_mask to propagate to all threads.

With the patch, an EdgeRouter Lite boots quite nicely with two cores.
There are stability issues however. My test setup tends to panic or
hang after a while. I am trying to find out the cause. It seems
unrelated to the patch.


Index: arch/octeon/octeon/machdep.c
===
RCS file: src/sys/arch/octeon/octeon/machdep.c,v
retrieving revision 1.64
diff -u -p -r1.64 machdep.c
--- arch/octeon/octeon/machdep.c25 Jun 2015 10:56:00 -  1.64
+++ arch/octeon/octeon/machdep.c14 Jul 2015 14:11:31 -
@@ -128,6 +128,10 @@ intocteon_cpuspeed(int *);
 static voidprocess_bootargs(void);
 static uint64_tget_ncpusfound(void);
 
+#ifdef MULTIPROCESSOR
+uint32_t   ipi_intr(uint32_t, struct trap_frame *);
+#endif
+
 extern voidparse_uboot_root(void);
 
 cons_decl(cn30xxuart);
@@ -487,6 +491,10 @@ mips_init(__register_t a0, __register_t 
Debugger();
 #endif
 
+#ifdef MULTIPROCESSOR
+   set_intr(INTPRI_IPI, CR_INT_1, ipi_intr);
+#endif
+
/*
 * Return the new kernel stack pointer.
 */
@@ -770,8 +778,7 @@ is_memory_range(paddr_t pa, psize_t len,
 uint32_t cpu_spinup_mask = 0;
 uint64_t cpu_spinup_a0, cpu_spinup_sp;
 static int (*ipi_handler)(void *);
-
-uint32_t ipi_intr(uint32_t, struct trap_frame *);
+static int ipi_enabled = 0;
 
 extern bus_space_t iobus_tag;
 extern bus_space_handle_t iobus_h;
@@ -852,6 +859,9 @@ ipi_intr(uint32_t hwpend, struct trap_fr
 */
bus_space_write_8(&iobus_tag, iobus_h, CIU_IP3_EN0(cpuid), 0);
 
+   if (!ipi_enabled)
+   return hwpend;
+
ipi_handler((void *)cpuid);
 
/*
@@ -865,14 +875,16 @@ ipi_intr(uint32_t hwpend, struct trap_fr
 int
 hw_ipi_intr_establish(int (*func)(void *), u_long cpuid)
 {
-   if (cpuid == 0)
+   if (cpuid == 0) {
ipi_handler = func;
+   mips_sync();
+   ipi_enabled = 1;
+   }
 
bus_space_write_8(&iobus_tag, iobus_h, CIU_MBOX_CLR(cpuid),
0x);
bus_space_write_8(&iobus_tag, iobus_h, CIU_IP3_EN0(cpuid),
(1ULL << CIU_INT_MBOX0)|(1ULL << CIU_INT_MBOX1));
-   set_intr(INTPRI_IPI, CR_INT_1, ipi_intr);
 
return 0;
 };