Re: [PATCH v2] percpu-refcount: Use normal instead of RCU-sched"

["Paul E. McKenney" <paulmck@kernel.org>]

On Mon, Nov 11, 2019 at 11:44:03AM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-11-08 18:17:47 [+0000], Christopher Lameter wrote:
> > On Fri, 8 Nov 2019, Sebastian Andrzej Siewior wrote:
> > 
> > > diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h
> > > index 7aef0abc194a2..390031e816dcd 100644
> > > --- a/include/linux/percpu-refcount.h
> > > +++ b/include/linux/percpu-refcount.h
> > > @@ -186,14 +186,14 @@ static inline void percpu_ref_get_many(struct percpu_ref *ref, unsigned long nr)
> > >  {
> > >  	unsigned long __percpu *percpu_count;
> > >
> > > -	rcu_read_lock_sched();
> > > +	rcu_read_lock();
> > >
> > >  	if (__ref_is_percpu(ref, &percpu_count))
> > >  		this_cpu_add(*percpu_count, nr);
> > 
> > You can use
> > 
> > 	__this_cpu_add()
> > 
> > instead since rcu_read_lock implies preempt disable.
> 
> Paul may correct me but rcu_read_lock() does not imply
> preempt_disable().  rcu_read_lock() does preempt_disable() on preemption
> models other than "Low-Latency Desktop". You can be preempted in a
> RCU-read section.

Sebastian is quite correct, rcu_read_lock() definitely does not imply
preempt_disable() when CONFIG_PREEMPT=y.  So the splat below is expected
behavior, and not just on x86.

							Thanx, Paul

> > This will not change the code for x86 but other platforms that do not use
> > atomic operation here will be able to avoid including to code to disable
> > preempt for the per cpu operations.
> 
> x86 triggers this warning with the suggested change:
> 
> | BUG: using __this_cpu_add() in preemptible [00000000] code: login/2370
> | caller is blk_mq_get_request+0x74/0x4c0
> | CPU: 0 PID: 2370 Comm: login Not tainted 5.4.0-rc7+ #82
> | Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.12.0-1 04/01/2014
> | Call Trace:
> |  dump_stack+0x7a/0xaa
> |  __this_cpu_preempt_check.cold+0x49/0x4e
> |  blk_mq_get_request+0x74/0x4c0
> |  blk_mq_make_request+0x111/0x890
> |  generic_make_request+0xd3/0x3f0
> |  submit_bio+0x42/0x140
> |  submit_bh_wbc.isra.0+0x13f/0x170
> |  ll_rw_block+0xa0/0xb0
> |  __breadahead+0x3f/0x70
> |  __ext4_get_inode_loc+0x40a/0x520
> |  __ext4_iget+0x10a/0xcf0
> |  ext4_lookup+0x106/0x1f0
> |  lookup_open+0x267/0x8e0
> |  path_openat+0x7c8/0xcb0
> |  do_filp_open+0x8c/0x100
> |  do_sys_open+0x17a/0x230
> |  __x64_sys_openat+0x1b/0x20
> |  do_syscall_64+0x5a/0x230
> |  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> > Same is valid for all other per cpu operations in the patch.
> 
> Sebastian