Date: Sat, 30 Jul 2011 20:29:40 +0000 (UTC) From: Andriy Gapon <avg@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r224527 - head/sys/kern Message-ID: <201107302029.p6UKTeGc014409@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: avg Date: Sat Jul 30 20:29:39 2011 New Revision: 224527 URL: http://svn.freebsd.org/changeset/base/224527 Log: smp_rendezvous: master cpu should wait until all slaves are fully done This is a followup to r222032 and a reimplementation of it. While that revision fixed the race for the smp_rv_waiters[2] exit sentinel, it still left a possibility for a target CPU to access stale or wrong smp_rv_func_arg in smp_rv_teardown_func. To fix this race the slave CPUs signal when they are really fully done with the rendezvous and the master CPU waits until all slaves are done. Diagnosed by: kib Reviewed by: jhb, mlaier, neel Approved by: re (kib) MFC after: 2 weeks Modified: head/sys/kern/subr_smp.c Modified: head/sys/kern/subr_smp.c ============================================================================== --- head/sys/kern/subr_smp.c Sat Jul 30 19:00:31 2011 (r224526) +++ head/sys/kern/subr_smp.c Sat Jul 30 20:29:39 2011 (r224527) @@ -109,8 +109,7 @@ static void (*volatile smp_rv_setup_func 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_generation; +static volatile int smp_rv_waiters[4]; /* * Shared mutex to restrict busywaits between smp_rendezvous() and @@ -321,7 +320,6 @@ smp_rendezvous_action(void) void (*local_setup_func)(void*); void (*local_action_func)(void*); void (*local_teardown_func)(void*); - int generation; #ifdef INVARIANTS int owepreempt; #endif @@ -336,7 +334,6 @@ smp_rendezvous_action(void) local_setup_func = smp_rv_setup_func; local_action_func = smp_rv_action_func; local_teardown_func = smp_rv_teardown_func; - generation = smp_rv_generation; /* * Use a nested critical section to prevent any preemptions @@ -382,32 +379,28 @@ smp_rendezvous_action(void) if (local_action_func != NULL) local_action_func(local_func_arg); - /* - * Signal that the main action has been completed. If a - * full exit rendezvous is requested, then all CPUs will - * wait here until all CPUs have finished the main action. - * - * Note that the write by the last CPU to finish the action - * may become visible to different CPUs at different times. - * As a result, the CPU that initiated the rendezvous may - * exit the rendezvous and drop the lock allowing another - * rendezvous to be initiated on the same CPU or a different - * CPU. In that case the exit sentinel may be cleared before - * all CPUs have noticed causing those CPUs to hang forever. - * Workaround this by using a generation count to notice when - * this race occurs and to exit the rendezvous in that case. - */ - MPASS(generation == smp_rv_generation); - atomic_add_int(&smp_rv_waiters[2], 1); if (local_teardown_func != smp_no_rendevous_barrier) { - while (smp_rv_waiters[2] < smp_rv_ncpus && - generation == smp_rv_generation) + /* + * Signal that the main action has been completed. If a + * full exit rendezvous is requested, then all CPUs will + * wait here until all CPUs have finished the main action. + */ + atomic_add_int(&smp_rv_waiters[2], 1); + while (smp_rv_waiters[2] < smp_rv_ncpus) cpu_spinwait(); if (local_teardown_func != NULL) local_teardown_func(local_func_arg); } + /* + * Signal that the rendezvous is fully completed by this CPU. + * This means that no member of smp_rv_* pseudo-structure will be + * accessed by this target CPU after this point; in particular, + * memory pointed by smp_rv_func_arg. + */ + atomic_add_int(&smp_rv_waiters[3], 1); + td->td_critnest--; KASSERT(owepreempt == td->td_owepreempt, ("rendezvous action changed td_owepreempt")); @@ -441,8 +434,6 @@ smp_rendezvous_cpus(cpuset_t map, mtx_lock_spin(&smp_ipi_mtx); - atomic_add_acq_int(&smp_rv_generation, 1); - /* Pass rendezvous parameters via global variables. */ smp_rv_ncpus = ncpus; smp_rv_setup_func = setup_func; @@ -451,6 +442,7 @@ smp_rendezvous_cpus(cpuset_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); /* @@ -466,13 +458,13 @@ smp_rendezvous_cpus(cpuset_t map, smp_rendezvous_action(); /* - * If the caller did not request an exit barrier to be enforced - * on each CPU, ensure that this CPU waits for all the other - * CPUs to finish the rendezvous. + * Ensure that the master CPU waits for all the other + * CPUs to finish the rendezvous, so that smp_rv_* + * pseudo-structure and the arg are guaranteed to not + * be in use. */ - if (teardown_func == smp_no_rendevous_barrier) - while (atomic_load_acq_int(&smp_rv_waiters[2]) < ncpus) - cpu_spinwait(); + while (atomic_load_acq_int(&smp_rv_waiters[3]) < ncpus) + cpu_spinwait(); mtx_unlock_spin(&smp_ipi_mtx); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201107302029.p6UKTeGc014409>