Messages in this thread | | | Date | Wed, 25 Sep 2013 17:55:15 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] hotplug: Optimize {get,put}_online_cpus() |
| |
On 09/24, Peter Zijlstra wrote: > > So now we drop from a no memory barriers fast path, into a memory > barrier 'slow' path into blocking.
Cough... can't understand the above ;) In fact I can't understand the patch... see below. But in any case, afaics the fast path needs mb() unless you add another synchronize_sched() into cpu_hotplug_done().
> +static inline void get_online_cpus(void) > +{ > + might_sleep(); > + > + /* Support reader-in-reader recursion */ > + if (current->cpuhp_ref++) { > + barrier(); > + return; > + } > + > + preempt_disable(); > + if (likely(!__cpuhp_writer)) > + __this_cpu_inc(__cpuhp_refcount);
mb() to ensure the reader can't miss, say, a STORE done inside the cpu_hotplug_begin/end section.
put_online_cpus() needs mb() as well.
> +void __get_online_cpus(void) > +{ > + if (__cpuhp_writer == 1) { > + /* See __srcu_read_lock() */ > + __this_cpu_inc(__cpuhp_refcount); > + smp_mb(); > + __this_cpu_inc(cpuhp_seq); > + return; > + }
OK, cpuhp_seq should guarantee cpuhp_readers_active_check() gets the "stable" numbers. Looks suspicious... but lets assume this works.
However, I do not see how "__cpuhp_writer == 1" can work, please see below.
> + /* > + * XXX list_empty_careful(&cpuhp_readers.task_list) ? > + */ > + if (atomic_dec_and_test(&cpuhp_waitcount)) > + wake_up_all(&cpuhp_writer);
Same problem as in previous version. __get_online_cpus() succeeds without incrementing __cpuhp_refcount. "goto start" can't help afaics.
> void cpu_hotplug_begin(void) > { > - cpu_hotplug.active_writer = current; > + unsigned int count = 0; > + int cpu; > > - for (;;) { > - mutex_lock(&cpu_hotplug.lock); > - if (likely(!cpu_hotplug.refcount)) > - break; > - __set_current_state(TASK_UNINTERRUPTIBLE); > - mutex_unlock(&cpu_hotplug.lock); > - schedule(); > - } > + lockdep_assert_held(&cpu_add_remove_lock); > + > + /* allow reader-in-writer recursion */ > + current->cpuhp_ref++; > + > + /* make readers take the slow path */ > + __cpuhp_writer = 1; > + > + /* See percpu_down_write() */ > + synchronize_sched();
Suppose there are no readers at this point,
> + > + /* make readers block */ > + __cpuhp_writer = 2; > + > + /* Wait for all readers to go away */ > + wait_event(cpuhp_writer, cpuhp_readers_active_check());
So wait_event() "quickly" returns.
Now. Why the new reader should see __cpuhp_writer = 2 ? It can still see it == 1, and take that "if (__cpuhp_writer == 1)" path above.
Oleg.
| |