Re: [PATCH 1/10] Add generic helpers for arch IPI function calls

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Tue, Jun 10, 2008 at 03:51:25PM +0100, Catalin Marinas wrote:
> Hi,
> 
> On Thu, 2008-05-29 at 10:58 +0200, Jens Axboe wrote:
> > This adds kernel/smp.c which contains helpers for IPI function calls. In
> > addition to supporting the existing smp_call_function() in a more efficient
> > manner, it also adds a more scalable variant called smp_call_function_single()
> > for calling a given function on a single CPU only.
> [...]
> > + * You must not call this function with disabled interrupts or from a
> > + * hardware interrupt handler or from a bottom half handler.
> > + */
> > +int smp_call_function_mask(cpumask_t mask, void (*func)(void *), void *info,
> > +			   int wait)
> > +{
> > +	struct call_function_data d;
> > +	struct call_function_data *data = NULL;
> > +	cpumask_t allbutself;
> > +	unsigned long flags;
> > +	int cpu, num_cpus;
> > +
> > +	/* Can deadlock when called with interrupts disabled */
> > +	WARN_ON(irqs_disabled());
> 
> I was thinking whether this condition can be removed and allow the
> smp_call_function*() to be called with IRQs disabled. At a quick look,
> it seems to be possible if the csd_flag_wait() function calls the IPI
> handlers directly when the IRQs are disabled (see the patch below).
> 
> This would be useful on ARM11MPCore based systems where the cache
> maintenance operations are not detected by the snoop control unit and
> this affects the DMA calls like dma_map_single(). There doesn't seem to
> be any restriction on calls to dma_map_single() and hence we cannot
> broadcast the cache operation to the other CPUs. I could implement this
> in the ARM specific code using spin_try_lock (on an IPI-specific lock
> held during the cross-call) and polling for an IPI if a lock cannot be
> acquired (meaning that a different CPU is issuing an IPI) but I was
> wondering whether this would be possible in a more generic way.
> 
> Please let me know what you think or whether deadlocks are still
> possible (or any other solution apart from hardware fixes :-)). Thanks.

There were objections last month:  http://lkml.org/lkml/2008/5/3/167

The issue was that this permits some interrupts to arrive despite
interrupts being disabled.  There seemed to be less resistance to
doing this in the wait==1 case, however.

						Thanx, Paul

> diff --git a/kernel/smp.c b/kernel/smp.c
> index ef6de3d..2c63e81 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -54,6 +54,10 @@ static void csd_flag_wait(struct call_single_data *data)
>  		smp_mb();
>  		if (!(data->flags & CSD_FLAG_WAIT))
>  			break;
> +		if (irqs_disabled()) {
> +			generic_smp_call_function_single_interrupt();
> +			generic_smp_call_function_interrupt();
> +		}
>  		cpu_relax();
>  	} while (1);
>  }
> @@ -208,9 +212,6 @@ int smp_call_function_single(int cpu, void (*func) (void *info), void *info,
>  	/* prevent preemption and reschedule on another processor */
>  	int me = get_cpu();
> 
> -	/* Can deadlock when called with interrupts disabled */
> -	WARN_ON(irqs_disabled());
> -
>  	if (cpu == me) {
>  		local_irq_save(flags);
>  		func(info);
> @@ -250,9 +251,6 @@ EXPORT_SYMBOL(smp_call_function_single);
>   */
>  void __smp_call_function_single(int cpu, struct call_single_data *data)
>  {
> -	/* Can deadlock when called with interrupts disabled */
> -	WARN_ON((data->flags & CSD_FLAG_WAIT) && irqs_disabled());
> -
>  	generic_exec_single(cpu, data);
>  }
> 
> @@ -279,9 +277,6 @@ int smp_call_function_mask(cpumask_t mask, void (*func)(void *), void *info,
>  	unsigned long flags;
>  	int cpu, num_cpus;
> 
> -	/* Can deadlock when called with interrupts disabled */
> -	WARN_ON(irqs_disabled());
> -
>  	cpu = smp_processor_id();
>  	allbutself = cpu_online_map;
>  	cpu_clear(cpu, allbutself);
> 
> 
> -- 
> Catalin
>