* [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary() @ 2012-04-16 7:25 Yong Zhang 2012-04-16 16:48 ` David Daney 0 siblings, 1 reply; 5+ messages in thread From: Yong Zhang @ 2012-04-16 7:25 UTC (permalink / raw) To: linux-mips; +Cc: Yong Zhang, David Daney, Ralf Baechle From: Yong Zhang <yong.zhang@windriver.com> Too early to enable irq will break some following action, such as notify_cpu_starting(). I don't get side effect with this patch. Signed-off-by: Yong Zhang <yong.zhang0@gmail.com> Cc: David Daney <ddaney@caviumnetworks.com> Cc: Ralf Baechle <ralf@linux-mips.org> --- arch/mips/cavium-octeon/smp.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c index 97e7ce9..7e65c88 100644 --- a/arch/mips/cavium-octeon/smp.c +++ b/arch/mips/cavium-octeon/smp.c @@ -185,7 +185,6 @@ static void __cpuinit octeon_init_secondary(void) octeon_init_cvmcount(); octeon_irq_setup_secondary(); - raw_local_irq_enable(); } /** -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary() 2012-04-16 7:25 [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary() Yong Zhang @ 2012-04-16 16:48 ` David Daney 2012-04-17 3:08 ` Yong Zhang 0 siblings, 1 reply; 5+ messages in thread From: David Daney @ 2012-04-16 16:48 UTC (permalink / raw) To: Yong Zhang; +Cc: linux-mips, Yong Zhang, David Daney, Ralf Baechle On 04/16/2012 12:25 AM, Yong Zhang wrote: > From: Yong Zhang<yong.zhang@windriver.com> > > Too early to enable irq will break some following action, > such as notify_cpu_starting(). Can you be more specific about what breaks? > > I don't get side effect with this patch. Without this, where do irqs get enabled on the secondary CPUs? > > Signed-off-by: Yong Zhang<yong.zhang0@gmail.com> > Cc: David Daney<ddaney@caviumnetworks.com> > Cc: Ralf Baechle<ralf@linux-mips.org> > --- > arch/mips/cavium-octeon/smp.c | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c > index 97e7ce9..7e65c88 100644 > --- a/arch/mips/cavium-octeon/smp.c > +++ b/arch/mips/cavium-octeon/smp.c > @@ -185,7 +185,6 @@ static void __cpuinit octeon_init_secondary(void) > octeon_init_cvmcount(); > > octeon_irq_setup_secondary(); > - raw_local_irq_enable(); > } > > /** ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary() 2012-04-16 16:48 ` David Daney @ 2012-04-17 3:08 ` Yong Zhang 2012-04-17 20:45 ` David Daney 0 siblings, 1 reply; 5+ messages in thread From: Yong Zhang @ 2012-04-17 3:08 UTC (permalink / raw) To: David Daney; +Cc: linux-mips, David Daney, Ralf Baechle On Mon, Apr 16, 2012 at 09:48:14AM -0700, David Daney wrote: > On 04/16/2012 12:25 AM, Yong Zhang wrote: > >From: Yong Zhang<yong.zhang@windriver.com> > > > >Too early to enable irq will break some following action, > >such as notify_cpu_starting(). > > Can you be more specific about what breaks? For example: CPU1 CPU2 __cpu_up(); mp_ops->boot_secondary(); start_secondary(); octeon_init_secondary(); raw_local_irq_enable(); <IRQ> do something; wake up softirqd; try_to_wake_up(); select_fallback_rq(); /* select wrong cpu */ set_cpu_online(); > > > > >I don't get side effect with this patch. > > Without this, where do irqs get enabled on the secondary CPUs? cpu_idle() will handle it. But in fact we should not depend on cpu_idle(). But it seems there is not suitable place to put local_irq_enable(), though ->smp_finish() looks like a more suitable place. When looking more at smp support on MIPS, there is more things I find. Such as set_cpu_online() is called on CPU1, so there will be another race window like above scenario. Please take a look at what commit 2baab4e9 intend to resolve. Thanks, Yong > > > > >Signed-off-by: Yong Zhang<yong.zhang0@gmail.com> > >Cc: David Daney<ddaney@caviumnetworks.com> > >Cc: Ralf Baechle<ralf@linux-mips.org> > >--- > > arch/mips/cavium-octeon/smp.c | 1 - > > 1 files changed, 0 insertions(+), 1 deletions(-) > > > >diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c > >index 97e7ce9..7e65c88 100644 > >--- a/arch/mips/cavium-octeon/smp.c > >+++ b/arch/mips/cavium-octeon/smp.c > >@@ -185,7 +185,6 @@ static void __cpuinit octeon_init_secondary(void) > > octeon_init_cvmcount(); > > > > octeon_irq_setup_secondary(); > >- raw_local_irq_enable(); > > } > > > > /** -- Only stand for myself ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary() 2012-04-17 3:08 ` Yong Zhang @ 2012-04-17 20:45 ` David Daney 2012-04-18 12:55 ` Yong Zhang 0 siblings, 1 reply; 5+ messages in thread From: David Daney @ 2012-04-17 20:45 UTC (permalink / raw) To: Yong Zhang, Ralf Baechle; +Cc: linux-mips, David Daney On 04/16/2012 08:08 PM, Yong Zhang wrote: > On Mon, Apr 16, 2012 at 09:48:14AM -0700, David Daney wrote: >> On 04/16/2012 12:25 AM, Yong Zhang wrote: >>> From: Yong Zhang<yong.zhang@windriver.com> >>> >>> Too early to enable irq will break some following action, >>> such as notify_cpu_starting(). >> >> Can you be more specific about what breaks? > > For example: > > CPU1 CPU2 > __cpu_up(); > mp_ops->boot_secondary(); > start_secondary(); > octeon_init_secondary(); > raw_local_irq_enable(); > <IRQ> > do something; > wake up softirqd; > try_to_wake_up(); > select_fallback_rq(); > /* select wrong cpu */ > set_cpu_online(); > Yeah, that looks broken to me too. >> >>> >>> I don't get side effect with this patch. >> >> Without this, where do irqs get enabled on the secondary CPUs? > > cpu_idle() will handle it. But in fact we should not depend on > cpu_idle(). It is not done in cpu_idle() itself. If irqs are disabled upon entry to cpu_idle() *and* need_resched(), then we call into schedule(). The irqs are then enabled by the raw_spin_unlock_irq(&rq->lock) at the end of kernel/sched/core.c: __schedule(). > > But it seems there is not suitable place to put local_irq_enable(), > though ->smp_finish() looks like a more suitable place. It would be better, but it seems like really it should be done in cpu_idle() immediately after rcu_idle_enter(). > > When looking more at smp support on MIPS, there is more things I find. > Such as set_cpu_online() is called on CPU1, so there will be another race > window like above scenario. Please take a look at what commit 2baab4e9 > intend to resolve. > > Thanks, > Yong > > > > >> >>> >>> Signed-off-by: Yong Zhang<yong.zhang0@gmail.com> >>> Cc: David Daney<ddaney@caviumnetworks.com> >>> Cc: Ralf Baechle<ralf@linux-mips.org> >>> --- >>> arch/mips/cavium-octeon/smp.c | 1 - >>> 1 files changed, 0 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c >>> index 97e7ce9..7e65c88 100644 >>> --- a/arch/mips/cavium-octeon/smp.c >>> +++ b/arch/mips/cavium-octeon/smp.c >>> @@ -185,7 +185,6 @@ static void __cpuinit octeon_init_secondary(void) >>> octeon_init_cvmcount(); >>> >>> octeon_irq_setup_secondary(); >>> - raw_local_irq_enable(); >>> } >>> >>> /** > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary() 2012-04-17 20:45 ` David Daney @ 2012-04-18 12:55 ` Yong Zhang 0 siblings, 0 replies; 5+ messages in thread From: Yong Zhang @ 2012-04-18 12:55 UTC (permalink / raw) To: David Daney; +Cc: Ralf Baechle, linux-mips, David Daney On Tue, Apr 17, 2012 at 01:45:50PM -0700, David Daney wrote: > On 04/16/2012 08:08 PM, Yong Zhang wrote: > >On Mon, Apr 16, 2012 at 09:48:14AM -0700, David Daney wrote: > >>On 04/16/2012 12:25 AM, Yong Zhang wrote: > >>>From: Yong Zhang<yong.zhang@windriver.com> > >>> > >>>Too early to enable irq will break some following action, > >>>such as notify_cpu_starting(). > >> > >>Can you be more specific about what breaks? > > > >For example: > > > > CPU1 CPU2 > >__cpu_up(); > > mp_ops->boot_secondary(); > > start_secondary(); > > octeon_init_secondary(); > > raw_local_irq_enable(); > > <IRQ> > > do something; > > wake up softirqd; > > try_to_wake_up(); > > select_fallback_rq(); > > /* select wrong cpu */ > > set_cpu_online(); > > > > Yeah, that looks broken to me too. > > >> > >>> > >>>I don't get side effect with this patch. > >> > >>Without this, where do irqs get enabled on the secondary CPUs? > > > >cpu_idle() will handle it. But in fact we should not depend on > >cpu_idle(). > > It is not done in cpu_idle() itself. If irqs are disabled upon > entry to cpu_idle() *and* need_resched(), then we call into > schedule(). The irqs are then enabled by the > raw_spin_unlock_irq(&rq->lock) at the end of kernel/sched/core.c: > __schedule(). > > > > >But it seems there is not suitable place to put local_irq_enable(), > >though ->smp_finish() looks like a more suitable place. > > It would be better, but it seems like really it should be done in > cpu_idle() immediately after rcu_idle_enter(). Hmm... seems we should put it before enter while (1) in cpu_idle(), otherwise tick_nohz_idle_enter() will be unhappy if NO_HZ enabled. But anyway, we still cann't survive from set_cpu_online() called on other cpu. I'll recheck smp support for MIPS to see if things could be improved ;-) Thanks, Yong > > > > >When looking more at smp support on MIPS, there is more things I find. > >Such as set_cpu_online() is called on CPU1, so there will be another race > >window like above scenario. Please take a look at what commit 2baab4e9 > >intend to resolve. > > > >Thanks, > >Yong > > > > > > > > > >> > >>> > >>>Signed-off-by: Yong Zhang<yong.zhang0@gmail.com> > >>>Cc: David Daney<ddaney@caviumnetworks.com> > >>>Cc: Ralf Baechle<ralf@linux-mips.org> > >>>--- > >>> arch/mips/cavium-octeon/smp.c | 1 - > >>> 1 files changed, 0 insertions(+), 1 deletions(-) > >>> > >>>diff --git a/arch/mips/cavium-octeon/smp.c b/arch/mips/cavium-octeon/smp.c > >>>index 97e7ce9..7e65c88 100644 > >>>--- a/arch/mips/cavium-octeon/smp.c > >>>+++ b/arch/mips/cavium-octeon/smp.c > >>>@@ -185,7 +185,6 @@ static void __cpuinit octeon_init_secondary(void) > >>> octeon_init_cvmcount(); > >>> > >>> octeon_irq_setup_secondary(); > >>>- raw_local_irq_enable(); > >>> } > >>> > >>> /** > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-04-18 12:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-16 7:25 [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary() Yong Zhang 2012-04-16 16:48 ` David Daney 2012-04-17 3:08 ` Yong Zhang 2012-04-17 20:45 ` David Daney 2012-04-18 12:55 ` Yong Zhang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.