Re: [PATCH] hotplug: Optimize {get,put}_online_cpus()

[Oleg Nesterov <oleg@redhat.com>]

On 09/23, Peter Zijlstra wrote:
>
> On Sat, Sep 21, 2013 at 06:34:04PM +0200, Oleg Nesterov wrote:
> > > So the slow path is still per-cpu and mostly uncontended even in the
> > > pending writer case.
> >
> > Is it really important? I mean, per-cpu/uncontended even if the writer
> > is pending?
>
> I think so, once we make {get,put}_online_cpus() really cheap they'll
> get in more and more places, and the global count with pending writer
> will make things crawl on bigger machines.

Hmm. But the writers should be rare.

> > But. We already have percpu_rw_semaphore,
>
> Oh urgh, forgot about that one. /me goes read.
>
> /me curses loudly.. that thing has an _expedited() call in it, those
> should die.

Probably yes, the original reason for _expedited() has gone away.

> I'd dread to think what would happen if a 4k cpu machine were to land in
> the slow path on that global mutex. Readers would never go-away and
> progress would make a glacier seem fast.

Another problem is that write-lock can never succeed unless it
prevents the new readers, but this needs the per-task counter.

> > Note also that percpu_down_write/percpu_up_write can be improved wrt
> > synchronize_sched(). We can turn the 2nd one into call_rcu(), and the
> > 1nd one can be avoided if another percpu_down_write() comes "soon after"
> > percpu_down_up().
>
> Write side be damned ;-)

Suppose that a 4k cpu machine does disable_nonboot_cpus(), every
_cpu_down() does synchronize_sched()... OK, perhaps the locking can be
changed so that cpu_hotplug_begin/end is called only once in this case.

> > 	- The writer calls cpuph_wait_refcount()
> >
> > 	- cpuph_wait_refcount() does refcnt += __cpuhp_refcount[0].
> > 	  refcnt == 0.
> >
> > 	- another reader comes on CPU_0, increments __cpuhp_refcount[0].
> >
> > 	- this reader migrates to CPU_1 and does put_online_cpus(),
> > 	  this decrements __cpuhp_refcount[1] which becomes zero.
> >
> > 	- cpuph_wait_refcount() continues and reads __cpuhp_refcount[1]
> > 	  which is zero. refcnt == 0, return.
>
> Ah indeed..
>
> The best I can come up with is something like:
>
> static unsigned int cpuhp_refcount(void)
> {
> 	unsigned int refcount = 0;
> 	int cpu;
>
> 	for_each_possible_cpu(cpu)
> 		refcount += per_cpu(__cpuhp_refcount, cpu);
> }
>
> static void cpuhp_wait_refcount(void)
> {
> 	for (;;) {
> 		unsigned int rc1, rc2;
>
> 		rc1 = cpuhp_refcount();
> 		set_current_state(TASK_UNINTERRUPTIBLE); /* MB */
> 		rc2 = cpuhp_refcount();
>
> 		if (rc1 == rc2 && !rc1)

But this only makes the race above "theoretical ** 2". Both
cpuhp_refcount()'s can be equally fooled.

Looks like, cpuhp_refcount() should take all per-cpu cpuhp_lock's
before it reads __cpuhp_refcount.

Oleg.