Re: proposed smp_rendezvous change

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.

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
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).)

-- 
John Baldwin