linux-mips
[Top] [All Lists]

Re: [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary()

To: David Daney <ddaney.cavm@gmail.com>
Subject: Re: [PATCH] MIPS: cavium: Don't enable irq in ->init__secondary()
From: Yong Zhang <yong.zhang0@gmail.com>
Date: Wed, 18 Apr 2012 20:55:16 +0800
Cc: Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org, David Daney <ddaney@caviumnetworks.com>
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:reply-to:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=yAPmUgXfiumLfjVfiBfSM5FKNZG1ksrg7pa14NT5aJg=; b=AxGhrOoylE+n8qgoDulIGzaaFoG8AbF5eTcXciWi5MDg0btaJB/TQjZJCyChQlxHCQ goAaz0FzJ8As6YrAiKh92ndfvq8M8RqCKOy3AU1Ff96usXFPXQY8feuxaf4chn6pLURl ZejTkv18klCNAjEO7yJYLHQJtNdRsYkU8mj4CUuUjHBk7BFyRLKmDp9GJGGphubQ0OvT OkHWoCb/7mx2Y/ztz/l8b8boq9Kc423NIAjKz1ym/PyG2qBPdVhq/JVm1/9VWJoUnxg3 xbGKrxg3ORit5zdvjwrxzLBWzIwD8/HNwfzAwF7KA2eQxY9vTiwMzc4TP7zoZNYRNsot ZPcw==
In-reply-to: <4F8DD67E.3000600@gmail.com>
References: <1334561133-19139-1-git-send-email-yong.zhang0@gmail.com> <4F8C4D4E.4060900@gmail.com> <20120417030848.GA6377@zhy> <4F8DD67E.3000600@gmail.com>
Reply-to: Yong Zhang <yong.zhang0@gmail.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.20 (2009-06-14)
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();
> >>>  }
> >>>
> >>>  /**
> >

<Prev in Thread] Current Thread [Next in Thread>