get_online_cpus() from a preemptible() context (bug?)

get_online_cpus() from a preemptible() context (bug?)

[James Morse]

Hi Thomas, Peter,

I'm trying to work out what stops a thread being pre-empted and migrated between
calling get_online_cpus() and put_online_cpus().

According to __percpu_down_read(), its the pre-empt count:
>  * Due to having preemption disabled the decrement happens on
>  * the same CPU as the increment, avoiding the
>  * increment-on-one-CPU-and-decrement-on-another problem.


So this:
> void cpus_read_lock(void)
> {
>        percpu_down_read(&cpu_hotplug_lock);
> +
> +       /* Can we migrated before we release this per-cpu lock? */
> +       WARN_ON(preemptible());
>  }

should never fire?

It does, some of the offenders:
> kmem_cache_create
> apply_workqueue_attrs
> stop_machine
> static_key_enable
> lru_add_drain_all
> __cpuhp_setup_state
> kmem_cache_shrink
> vmstat_shepherd
> __cpuhp_state_add_instance


Trying to leave preempt disabled between the down/up leads to
scheduling-while-atomic instead.

Can you point out what I've missed here?


Thanks,

James

Re: get_online_cpus() from a preemptible() context (bug?)

[Peter Zijlstra]

On Fri, Nov 03, 2017 at 02:45:45PM +0000, James Morse wrote:
> Hi Thomas, Peter,
> 
> I'm trying to work out what stops a thread being pre-empted and migrated between
> calling get_online_cpus() and put_online_cpus().
> 
> According to __percpu_down_read(), its the pre-empt count:
> >  * Due to having preemption disabled the decrement happens on
> >  * the same CPU as the increment, avoiding the
> >  * increment-on-one-CPU-and-decrement-on-another problem.
> 
> 
> So this:
> > void cpus_read_lock(void)
> > {
> >        percpu_down_read(&cpu_hotplug_lock);
> > +
> > +       /* Can we migrated before we release this per-cpu lock? */
> > +       WARN_ON(preemptible());
> >  }
> 
> should never fire?

It should.. You're reading a comment on __percpu_down_read() and using
percpu_down_read(), _not_ the same function ;-)

If you look at percpu_down_read(), you'll note it'll disable preemption
before calling __percpu_down_read().

And yes, that whole percpu-rwsem code is fairly magical :-)

Re: get_online_cpus() from a preemptible() context (bug?)

[Peter Zijlstra]

On Mon, Nov 06, 2017 at 11:32:12AM +0100, Peter Zijlstra wrote:
> On Fri, Nov 03, 2017 at 02:45:45PM +0000, James Morse wrote:
> > Hi Thomas, Peter,
> > 
> > I'm trying to work out what stops a thread being pre-empted and migrated between
> > calling get_online_cpus() and put_online_cpus().

Nothing; why would you think it would? All those functions guarantee is
that any CPU observed as being online says online (and its converse,
that a CPU observed as being offline, says offline, although less people
care about that one).

That is; it serializes against CPU hotplug, nothing else.

Re: get_online_cpus() from a preemptible() context (bug?)

[James Morse]

Hi Peter,

(combining your replies)

On 06/11/17 10:32, Peter Zijlstra wrote:
> On Fri, Nov 03, 2017 at 02:45:45PM +0000, James Morse wrote:
>> I'm trying to work out what stops a thread being pre-empted and migrated between
>> calling get_online_cpus() and put_online_cpus().

> Nothing; why would you think it would?

To stop the this_cpu_*() operations in down/up being applied on different CPUs,
affecting a different percpu:read_count.


> All those functions guarantee is
> that any CPU observed as being online says online (and its converse,
> that a CPU observed as being offline, says offline, although less people
> care about that one).


>> According to __percpu_down_read(), its the pre-empt count:
>>>  * Due to having preemption disabled the decrement happens on
>>>  * the same CPU as the increment, avoiding the
>>>  * increment-on-one-CPU-and-decrement-on-another problem.
>>
>>
>> So this:
>>> void cpus_read_lock(void)
>>> {
>>>        percpu_down_read(&cpu_hotplug_lock);
>>> +
>>> +       /* Can we migrated before we release this per-cpu lock? */
>>> +       WARN_ON(preemptible());
>>>  }
>>
>> should never fire?

> It should.. You're reading a comment on __percpu_down_read() and using
> percpu_down_read(), _not_ the same function ;-)

Yes, sorry, I thought you did a better job of describing the case I'm trying to
work-out.


> If you look at percpu_down_read(), you'll note it'll disable preemption
> before calling __percpu_down_read().

Yes, this is how __percpu_down_read() protects the combination of it's fast/slow
paths.

But next percpu_down_read() calls preempt_enable(), I can't see what stops us
migrating before percpu_up_read() preempt_disable()s to call __this_cpu_dec(),
which now affects a different variable.


> And yes, that whole percpu-rwsem code is fairly magical :-)

I think I'll file this under magical. That rcu_sync_is_idle() must know
something I don't!


Thanks!

James

Re: get_online_cpus() from a preemptible() context (bug?)

[Peter Zijlstra]

On Mon, Nov 06, 2017 at 06:51:35PM +0000, James Morse wrote:
> > If you look at percpu_down_read(), you'll note it'll disable preemption
> > before calling __percpu_down_read().
> 
> Yes, this is how __percpu_down_read() protects the combination of it's fast/slow
> paths.
> 
> But next percpu_down_read() calls preempt_enable(), I can't see what stops us
> migrating before percpu_up_read() preempt_disable()s to call __this_cpu_dec(),
> which now affects a different variable.
> 

Ah, so the two operations that comment talks about are:

    percpu_down_read_preempt_disable()
      preempt_disable();
1)    __this_cpu_inc(*sem->read_count);
      if (unlikely(!rcu_sync_is_idle(&sem->rss)))
	__percpu_down_read()
	  smp_mb()
	  if (likely(!smp_load_acquire(&sem->readers_block))) // false
	  __percpu_up_read()
	    smp_mb()
2)	   __this_cpu_dec(*sem->read_count);
	    rcuwait_wake_up(&sem->writer);
	  preempt_enable_no_resched();

If you want more detail on this, I'll actually have to go think :-)

Re: get_online_cpus() from a preemptible() context (bug?)

[James Morse]

Hi Peter,

On 06/11/17 21:07, Peter Zijlstra wrote:
> On Mon, Nov 06, 2017 at 06:51:35PM +0000, James Morse wrote:
>>> If you look at percpu_down_read(), you'll note it'll disable preemption
>>> before calling __percpu_down_read().
>>
>> Yes, this is how __percpu_down_read() protects the combination of it's fast/slow
>> paths.
>>
>> But next percpu_down_read() calls preempt_enable(), I can't see what stops us
>> migrating before percpu_up_read() preempt_disable()s to call __this_cpu_dec(),
>> which now affects a different variable.
>>
> 
> Ah, so the two operations that comment talks about are:
> 
>     percpu_down_read_preempt_disable()
>       preempt_disable();
> 1)    __this_cpu_inc(*sem->read_count);
>       if (unlikely(!rcu_sync_is_idle(&sem->rss)))
> 	__percpu_down_read()
> 	  smp_mb()
> 	  if (likely(!smp_load_acquire(&sem->readers_block))) // false
> 	  __percpu_up_read()
> 	    smp_mb()
> 2)	   __this_cpu_dec(*sem->read_count);
> 	    rcuwait_wake_up(&sem->writer);
> 	  preempt_enable_no_resched();
> 
> If you want more detail on this, I'll actually have to go think :-)

I think this was the answer to a much smarter question than mine!

I've tried (and failed) to break it instead. To answer my own question:

I thought this was potentially-broken because the __this_cpu_{add,dec}() out in
{get,put}_online_cpus() will operate on different per-cpu read_count variables
if we migrate. (not the pair above)

This isn't a problem as the only thing that reads the read_count is
readers_active_check(), which per_cpu_sum()s them all together before comparing
against zero. As they are all unsigned-ints it uses unsigned-overflow to do the
right thing. This even works if a CPU holding a vital part of the read_count is
offline, as per_cpu_sum() uses for_each_possible_cpu().


Thanks!

James