Messages in this thread | | | Date | Fri, 05 Jun 2009 09:32:13 +0800 | From | Lai Jiangshan <> | Subject | Re: [PATCH] cpuhotplug: introduce try_get_online_cpus() take 2 |
| |
Oleg Nesterov wrote: > On 06/04, Lai Jiangshan wrote: >> - Lockless for get_online_cpus()'s fast path >> - Introduce try_get_online_cpus() > > I think this can work... > >> @@ -50,10 +57,20 @@ void get_online_cpus(void) >> might_sleep(); >> if (cpu_hotplug.active_writer == current) >> return; >> - mutex_lock(&cpu_hotplug.lock); >> - cpu_hotplug.refcount++; >> - mutex_unlock(&cpu_hotplug.lock); >> >> + if (unlikely(!atomic_inc_not_zero(&cpu_hotplug.refcount))) { >> + DEFINE_WAIT(wait); >> + >> + for (;;) { >> + prepare_to_wait(&cpu_hotplug.sleeping_readers, &wait, >> + TASK_UNINTERRUPTIBLE); >> + if (atomic_inc_not_zero(&cpu_hotplug.refcount)) >> + break; >> + schedule(); >> + } >> + >> + finish_wait(&cpu_hotplug.sleeping_readers, &wait); >> + } >> } > > Looks like the code above can be replaced with > > wait_event(atomic_inc_not_zero(&cpu_hotplug.refcount));
You are right, but with the atomic_inc_not_zero() has side-effect, I'm afraid that wait_event() will be changed in future, and it may increases the cpu_hotplug.refcount twice.
#define wait_event(wq, condition) ......
I consider that @condition should not have side-effect, it should be some thing like this:
some_number == 2, !some_condition, some_thing_has_done, ......
> >> static void cpu_hotplug_done(void) >> { >> cpu_hotplug.active_writer = NULL; >> - mutex_unlock(&cpu_hotplug.lock); >> + atomic_inc(&cpu_hotplug.refcount); >> + >> + if (waitqueue_active(&cpu_hotplug.sleeping_readers)) >> + wake_up(&cpu_hotplug.sleeping_readers); >> } > > This looks racy. > > Suppose that the new reader comes right before atomic_inc(). The first > inc_not_zero() fails, the readear does prepare_to_wait(), the 2nd > inc_not_zero() fails too. > > cpu_hotplug_done() does atomic_inc(). > > What guarantees we must see waitqueue_active() == T? > > I think cpu_hotplug_done() should do unconditional wake_up(). This path > is slow anyway, "if (waitqueue_active())" does not buy too much. In this > case .sleeping_readers->lock closes the race. > > Unless I missed something, of course.
You are definitely right, cpu_hotplug_done() should do unconditional wake_up(). waitqueue_active() has no synchronization codes.
> > > Minor, but I'd suggest to use wake_up_all(). This does not make any > difference because we do not have WQ_FLAG_EXCLUSIVE waiters, but imho > looks a bit cleaner. > > > Hmm. It seems to me that cpu_hotplug_done() needs mb__before_atomic_inc() > before atomic_inc. Otherwise, "active_writer = NULL" can be re-ordered with > atomic_inc(). If the new reader does get_online_cpus() + put_online_cpus() > quicky, it can see active_writer != NULL. > >
The lines "active_writer = NULL" and "atomic_inc()" can exchange, there is no code need to synchronize to them. get_online_cpus()/put_online_cpus() will see "active_writer != current", it just what get_online_cpus()/put_online_cpus() needs.
Lai
| |