Re: proposed smp_rendezvous change

on 16/05/2011 23:09 John Baldwin said the following:
> On Monday, May 16, 2011 3:27:47 pm Andriy Gapon wrote:
>> on 16/05/2011 21:21 John Baldwin said the following:
>>> How about this:
>> ...
>>>  /* 
>>>   * Shared mutex to restrict busywaits between smp_rendezvous() and
>>> _at__at_ -311,39 +312,62 _at__at_ restart_cpus(cpumask_t map)
>>>  void
>>>  smp_rendezvous_action(void)
>>>  {
>>> -	void* local_func_arg = smp_rv_func_arg;
>>> -	void (*local_setup_func)(void*)   = smp_rv_setup_func;
>>> -	void (*local_action_func)(void*)   = smp_rv_action_func;
>>> -	void (*local_teardown_func)(void*) = smp_rv_teardown_func;
>>> +	void *local_func_arg;
>>> +	void (*local_setup_func)(void*);
>>> +	void (*local_action_func)(void*);
>>> +	void (*local_teardown_func)(void*);
>>> +	int generation;
>>>  
>>>  	/* Ensure we have up-to-date values. */
>>>  	atomic_add_acq_int(&smp_rv_waiters[0], 1);
>>>  	while (smp_rv_waiters[0] < smp_rv_ncpus)
>>>  		cpu_spinwait();
>>>  
>>> -	/* setup function */
>>> +	/* Fetch rendezvous parameters after acquire barrier. */
>>> +	local_func_arg = smp_rv_func_arg;
>>> +	local_setup_func = smp_rv_setup_func;
>>> +	local_action_func = smp_rv_action_func;
>>> +	local_teardown_func = smp_rv_teardown_func;
>>
>> I want to ask once again - please pretty please convince me that the above
>> cpu_spinwait() loop is really needed and, by extension, that the assignments
>> should be moved behind it.
>> Please :)
> 
> Well, moving the assignments down is a style fix for one, and we can always 
> remove the first rendezvous as a follow up if desired.

OK, makes sense.

> However, suppose you have an arch where sending an IPI is not a barrier
> (this seems unlikely).  In that case, the atomic_add_acq_int() will not
> succeed (and return) until it has seen the earlier write by the CPU
> initiating the rendezvous to clear smp_rv_waiters[0] to zero.  The actual
> spin on the smp_rv_waiters[] value is not strictly necessary however and

On this below.

> is probably just cut and pasted to match the other uses of values in
> the smp_rv_waiters[] array.
> 
> (atomic_add_acq_int() could spin on architectures where it is implemented
> using compare-and-swap (e.g. sparc64) or locked-load conditional-store (e.g. 
> Alpha).)


When you say "not strictly necessary", do you mean "not necessary"?
If you do not mean that, then when could it be (non-strictly) necessary? :)

Couldn't [Shouldn't] the whole:

>>>  	/* Ensure we have up-to-date values. */
>>>  	atomic_add_acq_int(&smp_rv_waiters[0], 1);
>>>  	while (smp_rv_waiters[0] < smp_rv_ncpus)
>>>  		cpu_spinwait();

be just replaced with:

rmb();

Or a proper MI function that does just a read memory barrier, if rmb() is not that.

-- 
Andriy Gapon