proposed smp_rendezvous change
Andriy Gapon
avg at FreeBSD.org
Tue May 17 08:03:41 UTC 2011
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
More information about the freebsd-current
mailing list