Re: proposed smp_rendezvous change

On Sunday, May 15, 2011 12:36:45 pm Andriy Gapon wrote:
> on 15/05/2011 18:16 John Baldwin said the following:
> > On 5/15/11 10:53 AM, Andriy Gapon wrote:
> >> on 15/05/2011 10:12 Andriy Gapon said the following:
> >>> on 14/05/2011 18:25 John Baldwin said the following:
> >>>> Hmmm, so this is not actually sufficient.  NetApp ran into a very similar race
> >>>> with virtual CPUs in BHyVe.  In their case because virtual CPUs are threads
> >>>> that
> >>>> can be preempted, they have a chance at a longer race.
> >>>>
> >>>> The problem that they see is that even though the values have been updated, the
> >>>> next CPU to start a rendezvous can clear smp_rv_waiters[2] to zero before
> >>>> one of
> >>>> the other CPUs notices that it has finished.
> >>>
> >>> As a follow up to my previous question.  Have you noticed that in my patch no
> >>> slave CPU actually waits/spins on smp_rv_waiters[2]?  It's always only master
> >>> CPU (and under smp_ipi_mtx).
> >>>
> >>
> >> Here's a cleaner version of my approach to the fix.
> >> This one does not remove the initial wait on smp_rv_waiters[0] in
> >> smp_rendezvous_action() and thus does not renumber all smp_rv_waiters[] members
> >> and thus hopefully should be clearer.
> >>
> >> Index: sys/kern/subr_smp.c
> >> ===================================================================
> >> --- sys/kern/subr_smp.c    (revision 221943)
> >> +++ sys/kern/subr_smp.c    (working copy)
> >> _at__at_ -110,7 +110,7 _at__at_ static void (*volatile smp_rv_setup_func)(void *ar
> >>   static void (*volatile smp_rv_action_func)(void *arg);
> >>   static void (*volatile smp_rv_teardown_func)(void *arg);
> >>   static void *volatile smp_rv_func_arg;
> >> -static volatile int smp_rv_waiters[3];
> >> +static volatile int smp_rv_waiters[4];
> >>
> >>   /*
> >>    * Shared mutex to restrict busywaits between smp_rendezvous() and
> >> _at__at_ -338,11 +338,15 _at__at_ smp_rendezvous_action(void)
> >>
> >>       /* spin on exit rendezvous */
> >>       atomic_add_int(&smp_rv_waiters[2], 1);
> >> -    if (local_teardown_func == smp_no_rendevous_barrier)
> >> +    if (local_teardown_func == smp_no_rendevous_barrier) {
> >> +        atomic_add_int(&smp_rv_waiters[3], 1);
> >>                   return;
> >> +    }
> >>       while (smp_rv_waiters[2]<  smp_rv_ncpus)
> >>           cpu_spinwait();
> >>
> >> +    atomic_add_int(&smp_rv_waiters[3], 1);
> >> +
> >>       /* teardown function */
> >>       if (local_teardown_func != NULL)
> >>           local_teardown_func(local_func_arg);
> >> _at__at_ -377,6 +381,9 _at__at_ smp_rendezvous_cpus(cpumask_t map,
> >>       /* obtain rendezvous lock */
> >>       mtx_lock_spin(&smp_ipi_mtx);
> >>
> >> +    while (smp_rv_waiters[3]<  smp_rv_ncpus)
> >> +        cpu_spinwait();
> >> +
> >>       /* set static function pointers */
> >>       smp_rv_ncpus = ncpus;
> >>       smp_rv_setup_func = setup_func;
> >> _at__at_ -385,6 +392,7 _at__at_ smp_rendezvous_cpus(cpumask_t map,
> >>       smp_rv_func_arg = arg;
> >>       smp_rv_waiters[1] = 0;
> >>       smp_rv_waiters[2] = 0;
> >> +    smp_rv_waiters[3] = 0;
> >>       atomic_store_rel_int(&smp_rv_waiters[0], 0);
> >>
> >>       /* signal other processors, which will enter the IPI with interrupts off */
> > 
> > Ahh, so the bump is after the change.  I do think this will still be ok
> > and I probably just didn't explain it well to Neel.  I wonder though
> > if the bump shouldn't happen until after the call of the local teardown
> > function?
> 
> That should not be necessary according to my understanding unless we have a
> reason to care about potentially disappearing function text.  Teardown function
> pointer and argument are locally cached and I don't see any problems with a new
> rendezvous being setup and run while teardowns of the previous one are still
> running.
> 
> On the other hand, you might be onto something here.  If the argument is
> allocated in temporary storage (e.g. on stack) and the teardown function
> actually uses the argument, then it's not safe to deallocate the argument until
> all of the teardowns are done.  In this case the master CPU should wait not only
> until all actions are done, but also until all teardowns are done.
> The current code doesn't do that.  And I am not sure if we actually would want
> to add this safety net or just document the behavior and put a burden on
> smp_rendezvous API users.

As we chatted about on IRC, I think we should err on the side of making this
API less complex (so that all of the actions for a given rendezvous finish
before the next rendezvous begins) as this stuff is already fairly complex.
We should only add more complexity if we really need it.

> BTW, I don't think that we ever use smp_rendezvous() in its full glory, i.e.
> with non-trivial setup, action and teardown (except perhaps for jkim's stress
> test).  OpenSolaris seems to be doing just fine with the following three types
> of simpler CPU cross-calls (as they call them):
> 1. async - a master CPU requests an action to be executed on target CPUs but
> doesn't wait for anything
> - we have no explicit equivalent for this
> 
> 2. call - a master CPU requests an action to be executed on target CPUs and
> waits until all the target CPUs have finished executing it
> - this would be equivalent smp_rendezvous_cpus(no_barrier, action, no_barrier)
> 
> 3. synch - a master CPU requests an action to be executed on target CPUs and
> waits until all the target CPUs have finished executing it and the slave CPUs
> are also waiting for their siblings to be done
> - this would be equivalent smp_rendezvous_cpus(no_barrier, action, NULL)
> 
> Seems that our current code doesn't use/need more than that as well.
> But I really like the capability that smp_rendezvous_cpus() potentially provides.

I would be fine with stripping down our rendezvous API a bit.  They could all
still use a single IPI and handler but perhaps prevent a few wrapper APIs that
provide simpler semantics like this.  Back to my statement above, we've had
this uber-generic rendezvous API for a while, but if we don't really need it to
be quite so complex I'd be happy to dumb down the API to what we really need so
the implementation and use can be simpler.

-- 
John Baldwin