Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 16 May 2011 16:09:21 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Andriy Gapon <avg@freebsd.org>
Cc:        Max Laier <max@love2party.net>, FreeBSD current <freebsd-current@freebsd.org>, neel@freebsd.org, Peter Grehan <grehan@freebsd.org>
Subject:   Re: proposed smp_rendezvous change
Message-ID:  <201105161609.21898.jhb@freebsd.org>
In-Reply-To: <4DD17AB3.1070606@FreeBSD.org>
References:  <4DCD357D.6000109@FreeBSD.org> <201105161421.27665.jhb@freebsd.org> <4DD17AB3.1070606@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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
> > @@ -311,39 +312,62 @@ 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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201105161609.21898.jhb>