lkml.org 
[lkml]   [2013]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] hotplug: Optimize {get,put}_online_cpus()
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.



\
 
 \ /
  Last update: 2013-09-23 20:01    [W:0.236 / U:0.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site