Re: [linux-next:master 6618/6917] kernel/sched/psi.c:1230:13: sparse: error: incompatible types in comparison expression (different address spaces)

["Paul E. McKenney" <paulmck@linux.ibm.com>]

On Fri, Feb 08, 2019 at 03:14:41PM -0800, Andrew Morton wrote:
> On Fri, 8 Feb 2019 02:29:33 +0800 kbuild test robot <lkp@intel.com> wrote:
> 
> > tree:   https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_next_linux-2Dnext.git&d=DwICAg&c=jf_iaSHvJObTbx-siA1ZOg&r=q4hkQkeaNH3IlTsPvEwkaUALMqf7y6jCMwT5b6lVQbQ&m=myIJaLgovNwHx7SqCW_p1sQx2YvRlmVbShFnuZEFqxY&s=0Y32d-tVCGOq6Vu_VAGgVgbEplhfvOSJ5evHbXTtyBI&e= master
> > head:   1bd831d68d5521c01d783af0275439ac645f5027
> > commit: e7acbba0d6f7a24c8d24280089030eb9a0eb7522 [6618/6917] psi: introduce psi monitor
> > reproduce:
> >         # apt-get install sparse
> >         git checkout e7acbba0d6f7a24c8d24280089030eb9a0eb7522
> >         make ARCH=x86_64 allmodconfig
> >         make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> > 
> > All errors (new ones prefixed by >>):
> > 
> >    kernel/sched/psi.c:151:6: sparse: warning: symbol 'psi_enable' was not declared. Should it be static?
> > >> kernel/sched/psi.c:1230:13: sparse: error: incompatible types in comparison expression (different address spaces)
> >    kernel/sched/psi.c:774:30: sparse: warning: dereference of noderef expression
> > 
> > vim +1230 kernel/sched/psi.c
> > 
> >   1222	
> >   1223	static __poll_t psi_fop_poll(struct file *file, poll_table *wait)
> >   1224	{
> >   1225		struct seq_file *seq = file->private_data;
> >   1226		struct psi_trigger *t;
> >   1227		__poll_t ret;
> >   1228	
> >   1229		rcu_read_lock();
> > > 1230		t = rcu_dereference(seq->private);
> >   1231		if (t)
> >   1232			ret = psi_trigger_poll(t, file, wait);
> >   1233		else
> >   1234			ret = DEFAULT_POLLMASK | EPOLLERR | EPOLLPRI;
> >   1235		rcu_read_unlock();
> >   1236	
> >   1237		return ret;
> >   1238	
> 
> Well a bit of googling led me to this fix:
> 
> --- a/kernel/sched/psi.c~psi-introduce-psi-monitor-fix-fix
> +++ a/kernel/sched/psi.c
> @@ -1227,7 +1227,7 @@ static __poll_t psi_fop_poll(struct file
>  	__poll_t ret;
>  
>  	rcu_read_lock();
> -	t = rcu_dereference(seq->private);
> +	t = rcu_dereference_raw(seq->private);
>  	if (t)
>  		ret = psi_trigger_poll(t, file, wait);
>  	else
> 
> But I have no idea why this works, nor what's going on in there. 
> rcu_dereference_raw() documentation is scant.
> 
> Paul, can you please shed light?

First, please avoid using rcu_dereference_raw() where possible.  It is
intended for situations where the developer cannot easily state what
is to be protecting access to an RCU-protected data structure.  So...

1.	If the access needs to be within an RCU read-side critical
	section, use rcu_dereference().  With the new consolidated
	RCU flavors, an RCU read-side critical section is entered
	using rcu_read_lock(), anything that disables bottom halves,
	anything that disables interrupts, or anything that disables
	preemption.

2.	If the access might be within an RCU read-side critical section
	on the one hand, or protected by (say) my_lock on the other,
	use rcu_dereference_check(), for example:
	
		p1 = rcu_dereference_check(p->rcu_protected_pointer,
					   lockdep_is_held(&my_lock));


3.	If the access might be within an RCU read-side critical section
	on the one hand, or protected by either my_lock or your_lock on
	the other, again use rcu_dereference_check(), for example:

		p1 = rcu_dereference_check(p->rcu_protected_pointer,
					   lockdep_is_held(&my_lock) ||
					   lockdep_is_held(&your_lock));

4.	If the access is on the update side, so that it is always protected
	by my_lock, use rcu_dereference_protected():

		p1 = rcu_dereference_protected(p->rcu_protected_pointer,
					       lockdep_is_held(&my_lock));

	This can be extended to handle multiple locks as in #3 above,
	and both can be extended to check other conditions as well.

5.	If the protection is supplied by the caller, and is thus unknown
	to this code, that is when you use rcu_dereference_raw().  Or
	I suppose you could use it when the lockdep expression would be
	excessively complex, except that a better approach in that case
	might be to take a long hard look at your synchronization design.
	Still, there are data-locking cases where any one of a very
	large number of locks or reference counters suffices to protect the
	pointer, so rcu_derefernce_raw() does have its place.

	However, its place is probably quite a bit smaller than one
	might expect given the number of uses in the current kernel.
	Ditto for its synonym, rcu_dereference_protected( ... , 1).  :-/

Now on to this sparse checking and what the point of it is.  This sparse
checking is opt-in.  Its purpose is to catch cases where someone
mistakenly does something like:

	p = q->rcu_protected_pointer;

When they should have done this instead:

	p = rcu_dereference(q->rcu_protected_pointer);

If you wish to opt into this checking, you need to mark the pointer
definitions (in this case ->private) with __rcu.  It may also
be necessary to mark function parameters as well, as is done for
radix_tree_iter_resume().  If you do not wish to use this checking,
you should ignore these sparse warnings.

Unfortunately, I don't know of a way to inform 0-day test robot of
the various maintainers' opt-in/out choices.

							Thanx, Paul