Date: Tue, 17 May 2011 11:03:37 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: John Baldwin <jhb@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: <4DD22BD9.6070504@FreeBSD.org> In-Reply-To: <201105161609.21898.jhb@freebsd.org> References: <4DCD357D.6000109@FreeBSD.org> <201105161421.27665.jhb@freebsd.org> <4DD17AB3.1070606@FreeBSD.org> <201105161609.21898.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
>>> @@ -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.
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4DD22BD9.6070504>