Re: [PATCH 2/2] consolidate writes in smp_call_funtion_interrupt

[Peter Zijlstra <peterz@infradead.org>]

On Tue, 2011-01-18 at 15:06 -0600, Milton Miller wrote:

> Index: common/kernel/smp.c
> ===================================================================
> --- common.orig/kernel/smp.c	2011-01-17 20:16:18.000000000 -0600
> +++ common/kernel/smp.c	2011-01-17 20:17:50.000000000 -0600
> @@ -193,6 +193,7 @@ void generic_smp_call_function_interrupt
>  	 */
>  	list_for_each_entry_rcu(data, &call_function.queue, csd.list) {
>  		int refs;
> +		void (*func) (void *info);
>  
>  		/*
>  		 * Since we walk the list without any locks, we might
> @@ -212,24 +213,32 @@ void generic_smp_call_function_interrupt
>  		if (atomic_read(&data->refs) == 0)
>  			continue;
>  
> +		func = data->csd.func;			/* for later warn */
>  		data->csd.func(data->csd.info);
>  
> +		/*
> +		 * If the cpu mask is not still set then it enabled interrupts,
> +		 * we took another smp interrupt, and executed the function
> +		 * twice on this cpu.  In theory that copy decremented refs.
> +		 */
> +		if (!cpumask_test_and_clear_cpu(cpu, data->cpumask)) {
> +			WARN(1, "%pS enabled interrupts and double executed\n",
> +			     func);
> +			continue;
> +		}
> +
>  		refs = atomic_dec_return(&data->refs);
>  		WARN_ON(refs < 0);
>  
>  		if (refs)
>  			continue;
>  
> +		WARN_ON(!cpumask_empty(data->cpumask));
> +
> +		raw_spin_lock(&call_function.lock);
> +		list_del_rcu(&data->csd.list);
> +		raw_spin_unlock(&call_function.lock);
> +
>  		csd_unlock(&data->csd);
>  	}
>  


So after this we have:

list_for_each_entry_rcu()
rbd
  !->cpumask			  ->cpumask = 
rmb				wmb
  !->refs			  ->refs =
  ->func()			wmb
mb				  list_add_rcu()
  ->refs--
  if (!->refs)
    list_del_rcu()

So even if we see it as an old-ref, when we see a valid cpumask, valid
ref, we execute the function clear our cpumask bit and decrement the ref
and delete the entry, even though it might not yet be added?


(old-ref)
				->cpumask =
  if (!->cpumask)
				->refs = 
  if (!->refs)

  ->func()
  ->refs--
  if (!->refs)
    list_del_rcu()
				list_add_rcu()

Then what happens?