Re: [tip:x86/pat] generic-ipi: Allow cpus not yet online to call smp_call_function with irqs disabled

Re: [tip:x86/pat] generic-ipi: Allow cpus not yet online to call smp_call_function with irqs disabled

[Nick Piggin]

On Fri, Aug 21, 2009 at 11:51:50PM +0000, Suresh B wrote:
> Commit-ID:  269c861baa2fe7c114c3bc7831292758d29eb336
> Gitweb:     http://git.kernel.org/tip/269c861baa2fe7c114c3bc7831292758d29eb336
> Author:     Suresh Siddha <suresh.b.siddha@intel.com>
> AuthorDate: Wed, 19 Aug 2009 18:05:35 -0700
> Committer:  H. Peter Anvin <hpa@zytor.com>
> CommitDate: Fri, 21 Aug 2009 16:25:43 -0700
> 
> generic-ipi: Allow cpus not yet online to call smp_call_function with irqs disabled
> 
> Because of deadlock possiblities smp_call_function() is not allowed to
> be called with interrupts disabled. Add an exception for the cpu not
> yet online, as no one else can send smp call function interrupt to this
> cpu that is not yet online and as such deadlock condition is not possible.
> 
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Acked-by: Nick Piggin <npiggin@suse.de>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>

I don't know if we should allow the use of smp_call_function here --
only call_function_single. CPU hotplug code is required to set up
some call_function data and if the cpu is offline then it might not
be set up correctly.

Also, I would say that we should just restrict this to wait==1 case
because in that case the stack can trivially be used for data. In
the wait==0 case, it is more complex. In the current implementation
it should be OK (it uses per-cpu data), but we've used kmalloc
there in the past, which probably wouldn't work either.


> ---
>  kernel/smp.c |   40 ++++++++++++++++++++++++++++++++++------
>  1 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index ad63d85..2accdf6 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -177,6 +177,11 @@ void generic_smp_call_function_interrupt(void)
>  	int cpu = get_cpu();
>  
>  	/*
> +	 * Shouldn't receive this interrupt on a cpu that is not yet online.
> +	 */
> +	WARN_ON_ONCE(!cpu_online(cpu));
> +
> +	/*
>  	 * Ensure entry is visible on call_function_queue after we have
>  	 * entered the IPI. See comment in smp_call_function_many.
>  	 * If we don't have this, then we may miss an entry on the list
> @@ -230,6 +235,11 @@ void generic_smp_call_function_single_interrupt(void)
>  	unsigned int data_flags;
>  	LIST_HEAD(list);
>  
> +	/*
> +	 * Shouldn't receive this interrupt on a cpu that is not yet online.
> +	 */
> +	WARN_ON_ONCE(!cpu_online(smp_processor_id()));
> +
>  	spin_lock(&q->lock);
>  	list_replace_init(&q->list, &list);
>  	spin_unlock(&q->lock);
> @@ -285,8 +295,14 @@ int smp_call_function_single(int cpu, void (*func) (void *info), void *info,
>  	 */
>  	this_cpu = get_cpu();
>  
> -	/* Can deadlock when called with interrupts disabled */
> -	WARN_ON_ONCE(irqs_disabled() && !oops_in_progress);
> +	/*
> +	 * Can deadlock when called with interrupts disabled.
> +	 * We allow cpu's that are not yet online though, as no one else can
> +	 * send smp call function interrupt to this cpu and as such deadlocks
> +	 * can't happen.
> +	 */
> +	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
> +		     && !oops_in_progress);
>  
>  	if (cpu == this_cpu) {
>  		local_irq_save(flags);
> @@ -329,8 +345,14 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
>  {
>  	csd_lock(data);
>  
> -	/* Can deadlock when called with interrupts disabled */
> -	WARN_ON_ONCE(wait && irqs_disabled() && !oops_in_progress);
> +	/*
> +	 * Can deadlock when called with interrupts disabled.
> +	 * We allow cpu's that are not yet online though, as no one else can
> +	 * send smp call function interrupt to this cpu and as such deadlocks
> +	 * can't happen.
> +	 */
> +	WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
> +		     && !oops_in_progress);
>  
>  	generic_exec_single(cpu, data, wait);
>  }
> @@ -365,8 +387,14 @@ void smp_call_function_many(const struct cpumask *mask,
>  	unsigned long flags;
>  	int cpu, next_cpu, this_cpu = smp_processor_id();
>  
> -	/* Can deadlock when called with interrupts disabled */
> -	WARN_ON_ONCE(irqs_disabled() && !oops_in_progress);
> +	/*
> +	 * Can deadlock when called with interrupts disabled.
> +	 * We allow cpu's that are not yet online though, as no one else can
> +	 * send smp call function interrupt to this cpu and as such deadlocks
> +	 * can't happen.
> +	 */
> +	WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
> +		     && !oops_in_progress);
>  
>  	/* So, what's a CPU they want? Ignoring this one. */
>  	cpu = cpumask_first_and(mask, cpu_online_mask);

Re: [tip:x86/pat] generic-ipi: Allow cpus not yet online to call smp_call_function with irqs disabled

[Suresh Siddha]

On Sun, 2009-08-23 at 22:40 -0700, Nick Piggin wrote:
> On Fri, Aug 21, 2009 at 11:51:50PM +0000, Suresh B wrote:
> > Commit-ID:  269c861baa2fe7c114c3bc7831292758d29eb336
> > Gitweb:     http://git.kernel.org/tip/269c861baa2fe7c114c3bc7831292758d29eb336
> > Author:     Suresh Siddha <suresh.b.siddha@intel.com>
> > AuthorDate: Wed, 19 Aug 2009 18:05:35 -0700
> > Committer:  H. Peter Anvin <hpa@zytor.com>
> > CommitDate: Fri, 21 Aug 2009 16:25:43 -0700
> > 
> > generic-ipi: Allow cpus not yet online to call smp_call_function with irqs disabled
> > 
> > Because of deadlock possiblities smp_call_function() is not allowed to
> > be called with interrupts disabled. Add an exception for the cpu not
> > yet online, as no one else can send smp call function interrupt to this
> > cpu that is not yet online and as such deadlock condition is not possible.
> > 
> > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> > Acked-by: Nick Piggin <npiggin@suse.de>
> > Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> 
> I don't know if we should allow the use of smp_call_function here --
> only call_function_single. CPU hotplug code is required to set up
> some call_function data and if the cpu is offline then it might not
> be set up correctly.

We are doing the required allocations in CPU_UP_PREPARE. So we should be
okay for any smp_call_function usage.

> Also, I would say that we should just restrict this to wait==1 case
> because in that case the stack can trivially be used for data. In
> the wait==0 case, it is more complex. In the current implementation
> it should be OK (it uses per-cpu data), but we've used kmalloc
> there in the past, which probably wouldn't work either.

In future if we add any kmalloc, we already have checks in kmalloc()
that can be easily caught. I would like to make this change as generic
as possible.

thanks,
suresh

Re: [tip:x86/pat] generic-ipi: Allow cpus not yet online to call smp_call_function with irqs disabled

[Nick Piggin]

On Mon, Aug 24, 2009 at 01:15:43PM -0700, Suresh B wrote:
> On Sun, 2009-08-23 at 22:40 -0700, Nick Piggin wrote:
> > On Fri, Aug 21, 2009 at 11:51:50PM +0000, Suresh B wrote:
> > > Commit-ID:  269c861baa2fe7c114c3bc7831292758d29eb336
> > > Gitweb:     http://git.kernel.org/tip/269c861baa2fe7c114c3bc7831292758d29eb336
> > > Author:     Suresh Siddha <suresh.b.siddha@intel.com>
> > > AuthorDate: Wed, 19 Aug 2009 18:05:35 -0700
> > > Committer:  H. Peter Anvin <hpa@zytor.com>
> > > CommitDate: Fri, 21 Aug 2009 16:25:43 -0700
> > > 
> > > generic-ipi: Allow cpus not yet online to call smp_call_function with irqs disabled
> > > 
> > > Because of deadlock possiblities smp_call_function() is not allowed to
> > > be called with interrupts disabled. Add an exception for the cpu not
> > > yet online, as no one else can send smp call function interrupt to this
> > > cpu that is not yet online and as such deadlock condition is not possible.
> > > 
> > > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> > > Acked-by: Nick Piggin <npiggin@suse.de>
> > > Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> > 
> > I don't know if we should allow the use of smp_call_function here --
> > only call_function_single. CPU hotplug code is required to set up
> > some call_function data and if the cpu is offline then it might not
> > be set up correctly.
> 
> We are doing the required allocations in CPU_UP_PREPARE. So we should be
> okay for any smp_call_function usage.
 
OK

> > Also, I would say that we should just restrict this to wait==1 case
> > because in that case the stack can trivially be used for data. In
> > the wait==0 case, it is more complex. In the current implementation
> > it should be OK (it uses per-cpu data), but we've used kmalloc
> > there in the past, which probably wouldn't work either.
> 
> In future if we add any kmalloc, we already have checks in kmalloc()
> that can be easily caught. I would like to make this change as generic
> as possible.

Why? You think there will be much demand for it?

Re: [tip:x86/pat] generic-ipi: Allow cpus not yet online to call smp_call_function with irqs disabled

[Suresh Siddha]

On Mon, 2009-08-24 at 21:29 -0700, Nick Piggin wrote:
> On Mon, Aug 24, 2009 at 01:15:43PM -0700, Suresh B wrote:
> > On Sun, 2009-08-23 at 22:40 -0700, Nick Piggin wrote:
> > > Also, I would say that we should just restrict this to wait==1 case
> > > because in that case the stack can trivially be used for data. In
> > > the wait==0 case, it is more complex. In the current implementation
> > > it should be OK (it uses per-cpu data), but we've used kmalloc
> > > there in the past, which probably wouldn't work either.
> > 
> > In future if we add any kmalloc, we already have checks in kmalloc()
> > that can be easily caught. I would like to make this change as generic
> > as possible.
> 
> Why? You think there will be much demand for it?

In the current mtrr case, we currently use wait==0

Also unless there are issues, I would like to keep it open. So that it
will encourage more usages and expose any other bugs that we perhaps
overlooked.

With all the recent generic-ipi changes, even for online cpu's, we
should be able to allow smp_call_function() with interrupts disabled for
wait==0 case, right?

thanks,
suresh