linux-kernel - Re: [PATCH 6/8] MIPS: call set_cpu_online() on the uping cpu with irq disabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120522062126.GB12098@zhy>
Date:	Tue, 22 May 2012 14:21:27 +0800
From:	Yong Zhang <yong.zhang0@...il.com>
To:	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc:	linux-mips@...ux-mips.org, linux-kernel@...r.kernel.org,
	ralf@...ux-mips.org, sshtylyov@...sta.com, david.daney@...ium.com
Subject: Re: [PATCH 6/8] MIPS: call set_cpu_online() on the uping cpu with
 irq disabled

On Mon, May 21, 2012 at 04:09:16PM +0530, Srivatsa S. Bhat wrote:
> On 05/21/2012 11:30 AM, Yong Zhang wrote:
> 
> > From: Yong Zhang <yong.zhang@...driver.com>
> > 
> > To prevent a problem as commit 5fbd036b [sched: Cleanup cpu_active madness]
> > and commit 2baab4e9 [sched: Fix select_fallback_rq() vs cpu_active/cpu_online]
> > try to resolve, move set_cpu_online() to the brought up CPU and with irq
> > disabled.
> > 
> > Signed-off-by: Yong Zhang <yong.zhang0@...il.com>
> > Acked-by: David Daney <david.daney@...ium.com>
> > ---
> >  arch/mips/kernel/smp.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
> > index 73a268a..042145f 100644
> > --- a/arch/mips/kernel/smp.c
> > +++ b/arch/mips/kernel/smp.c
> > @@ -122,6 +122,8 @@ asmlinkage __cpuinit void start_secondary(void)
> > 
> >  	notify_cpu_starting(cpu);
> > 
> > +	set_cpu_online(cpu, true);
> > +
> 
> 
> You will also need to use ipi_call_lock/unlock() around this.
> See how x86 does it. (MIPS also selects USE_GENERIC_SMP_HELPERS).

Hmm... But look at the comments in arch/x86/kernel/smpboot.c::start_secondary()

start_secondary()
{
	...
        /*   
         * We need to hold call_lock, so there is no inconsistency
         * between the time smp_call_function() determines number of
         * IPI recipients, and the time when the determination is made
         * for which cpus receive the IPI. Holding this
         * lock helps us to not include this cpu in a currently in progress
         * smp_call_function().
         *
         * We need to hold vector_lock so there the set of online cpus
         * does not change while we are assigning vectors to cpus.  Holding
         * this lock ensures we don't half assign or remove an irq from a cpu.
         */
        ipi_call_lock();
        lock_vector_lock();
        set_cpu_online(smp_processor_id(), true);
        unlock_vector_lock();
        ipi_call_unlock();

	...
}

which ipi_call_lock()/ipi_call_unlock() is to pretect race with concurrent 
smp_call_function(), but it seems that is already broken, because

1) The comments is alread there before we switch to generic smp helper(commit
   3b16cf87), and at that time the comments is true because
   smp_call_function_interrupt() doesn't test if a cpu should handle the
   IPI interrupt.
   But in the gereric smp helper, we have checked if a cpu should handle the IPI
   in generic_smp_call_function_interrupt():
   	if (!cpumask_test_cpu(cpu, data->cpumask))
		continue;

2) call_function.lock used in smp_call_function_many() is just to protect
   call_function.queue and &data->refs, cpu_online_mask is outside of the
   lock. And I don't think it's necessary to protect cpu_online_mask,
   because data->cpumask is pre-calculate and even if a cpu is brougt up
   when calling arch_send_call_function_ipi_mask(), it's harmless because
   validation test in generic_smp_call_function_interrupt() will take care
   of it.

3) For cpu down issue, stop_machine() will guarantee that no concurrent
   smp_call_fuction() is processing.

So it seems ipi_call_lock()/ipi_call_unlock() is not needed and could be
removed IMHO.
Or am I missing something?

Thanks,
Yong

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/