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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git 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	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67247 bytes --]

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

[Andrew Morton]

On Fri, 8 Feb 2019 02:29:33 +0800 kbuild test robot <lkp@intel.com> wrote:

> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git 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?

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]

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

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

[Andrew Morton]

> > 
> > 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).  :-/

Is this documented anywhere (apart from here?)

> 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.

Oh geeze.

Good luck, Suren ;)

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

[Matthew Wilcox]

On Fri, Feb 08, 2019 at 11:44:07PM -0800, Paul E. McKenney wrote:
> 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);

So the problem here is the opposite of what we think it is -- seq->private
is not marked as being RCU protected.

> 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.

radix_tree_iter_resume is, happily, gone from my xarray-conv tree.
__radix_tree_lookup, __radix_tree_replace, radix_tree_iter_replace and
radix_tree_iter_init are still present, but hopefully not for too much
longer.  For example, __radix_tree_replace() is (now) called only from
idr_replace(), and there are only 12 remaining callers of idr_replace().

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]

On Mon, Feb 11, 2019 at 05:00:37PM -0800, Andrew Morton wrote:
> > > 
> > > 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).  :-/
> 
> Is this documented anywhere (apart from here?)

In the docbook headers for these functions, apart from rcu_dereference_raw(),
whose use I am not encouraging.

But having it in one place with examples might be helpful.  Does the
patch at the end of this email seem reasonable?

> > 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.
> 
> Oh geeze.
> 
> Good luck, Suren ;)

Ummm...  OK...

							Thanx, Paul

------------------------------------------------------------------------

commit abf0d8830a2885af9d17c41cfb7fe32321df94cb
Author: Paul E. McKenney <paulmck@linux.ibm.com>
Date:   Tue Feb 12 07:51:24 2019 -0800

    doc: Describe choice of rcu_dereference() APIs and __rcu usage
    
    Reported-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>

diff --git a/Documentation/RCU/rcu_dereference.txt b/Documentation/RCU/rcu_dereference.txt
index ab96227bad42..bf699e8cfc75 100644
--- a/Documentation/RCU/rcu_dereference.txt
+++ b/Documentation/RCU/rcu_dereference.txt
@@ -351,3 +351,106 @@ garbage values.
 
 In short, rcu_dereference() is -not- optional when you are going to
 dereference the resulting pointer.
+
+
+WHICH MEMBER OF THE rcu_dereference() FAMILY SHOULD YOU USE?
+
+First, please avoid using rcu_dereference_raw() and also please avoid
+using rcu_dereference_check() and rcu_dereference_protected() with a
+second argument with a constant value of 1 (or true, for that matter).
+With that caution out of the way, here is some guidance for which
+member of the rcu_dereference() to use in various situations:
+
+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 the rare case when rcu_dereference_raw()
+	is appropriate.  In addition, rcu_dereference_raw() might be
+	appropriate 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_dereference_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_check( ... , 1), and
+	its close relative, rcu_dereference_protected(... , 1).
+
+
+SPARSE CHECKING OF RCU-PROTECTED POINTERS
+
+The sparse static-analysis tool checks for direct access to RCU-protected
+pointers, which can result in "interesting" bugs due to compiler
+optimizations involving invented loads and perhaps also load tearing.
+For example, suppose someone mistakenly does something like this:
+
+	p = q->rcu_protected_pointer;
+	do_something_with(p->a);
+	do_something_else_with(p->b);
+
+If register pressure is high, the compiler might optimize "p" out
+of existence, transforming the code to something like this:
+
+	do_something_with(q->rcu_protected_pointer->a);
+	do_something_else_with(q->rcu_protected_pointer->b);
+
+This could fatally disappoint your code if q->rcu_protected_pointer
+changed in the meantime.  Nor is this a theoretical problem:  Exactly
+this sort of bug cost Paul E. McKenney (and several of his innocent
+colleagues) a three-day weekend back in the early 1990s.
+
+Load tearing could of course result in dereferencing a mashup of a pair
+of pointers, which also might fatally disappoint your code.
+
+These problems could have been avoided simply by making the code instead
+read as follows:
+
+	p = rcu_dereference(q->rcu_protected_pointer);
+	do_something_with(p->a);
+	do_something_else_with(p->b);
+
+Unfortunately, these sorts of bugs can be extremely hard to spot during
+review.  This is where the sparse tool comes into play, along with the
+"__rcu" marker.  If you mark a pointer declaration, whether in a structure
+or as a formal parameter, with "__rcu", which tells sparse to complain if
+this pointer is accessed directly.  It will also cause sparse to complain
+if a pointer not marked with "__rcu" is accessed using rcu_dereference()
+and friends.  For example, ->rcu_protected_pointer might be declared as
+follows:
+
+	struct foo __rcu *rcu_protected_pointer;
+
+Use of "__rcu" is opt-in.  If you choose not to use it, then you should
+ignore the sparse warnings.

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]

On Mon, Feb 11, 2019 at 05:36:06PM -0800, Matthew Wilcox wrote:
> On Fri, Feb 08, 2019 at 11:44:07PM -0800, Paul E. McKenney wrote:
> > 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);
> 
> So the problem here is the opposite of what we think it is -- seq->private
> is not marked as being RCU protected.

Glad to have helped, then.  ;-)

> > 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.
> 
> radix_tree_iter_resume is, happily, gone from my xarray-conv tree.
> __radix_tree_lookup, __radix_tree_replace, radix_tree_iter_replace and
> radix_tree_iter_init are still present, but hopefully not for too much
> longer.  For example, __radix_tree_replace() is (now) called only from
> idr_replace(), and there are only 12 remaining callers of idr_replace().

Will this reduce the number of uses of rcu_dereference_raw()?  Or do they
simply migrate into Xarray?

							Thanx, Paul

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

[Matthew Wilcox]

On Tue, Feb 12, 2019 at 07:56:10AM -0800, Paul E. McKenney wrote:
> On Mon, Feb 11, 2019 at 05:36:06PM -0800, Matthew Wilcox wrote:
> > radix_tree_iter_resume is, happily, gone from my xarray-conv tree.
> > __radix_tree_lookup, __radix_tree_replace, radix_tree_iter_replace and
> > radix_tree_iter_init are still present, but hopefully not for too much
> > longer.  For example, __radix_tree_replace() is (now) called only from
> > idr_replace(), and there are only 12 remaining callers of idr_replace().
> 
> Will this reduce the number of uses of rcu_dereference_raw()?  Or do they
> simply migrate into Xarray?

Unlike the radix tree (which let you do whatever awful locking scheme you
wanted), the XArray requires that you use the spinlock embedded in the
root of the data structure to protect against simultaneous modification.
So all dereferences within the XArray code look like this:

(if either under lock, or rcu lock held):
        return rcu_dereference_check(node->slots[offset],
                                                lockdep_is_held(&xa->xa_lock));

(if we know the lock is held):
        return rcu_dereference_protected(node->slots[offset],
                                                lockdep_is_held(&xa->xa_lock));

The XArray API doesn't expose slot pointers to its clients.  It hides them
inside the xa_state's pointer to the current xa_node.

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]

On Tue, Feb 12, 2019 at 08:25:18AM -0800, Matthew Wilcox wrote:
> On Tue, Feb 12, 2019 at 07:56:10AM -0800, Paul E. McKenney wrote:
> > On Mon, Feb 11, 2019 at 05:36:06PM -0800, Matthew Wilcox wrote:
> > > radix_tree_iter_resume is, happily, gone from my xarray-conv tree.
> > > __radix_tree_lookup, __radix_tree_replace, radix_tree_iter_replace and
> > > radix_tree_iter_init are still present, but hopefully not for too much
> > > longer.  For example, __radix_tree_replace() is (now) called only from
> > > idr_replace(), and there are only 12 remaining callers of idr_replace().
> > 
> > Will this reduce the number of uses of rcu_dereference_raw()?  Or do they
> > simply migrate into Xarray?
> 
> Unlike the radix tree (which let you do whatever awful locking scheme you
> wanted), the XArray requires that you use the spinlock embedded in the
> root of the data structure to protect against simultaneous modification.
> So all dereferences within the XArray code look like this:
> 
> (if either under lock, or rcu lock held):
>         return rcu_dereference_check(node->slots[offset],
>                                                 lockdep_is_held(&xa->xa_lock));
> 
> (if we know the lock is held):
>         return rcu_dereference_protected(node->slots[offset],
>                                                 lockdep_is_held(&xa->xa_lock));
> 
> The XArray API doesn't expose slot pointers to its clients.  It hides them
> inside the xa_state's pointer to the current xa_node.

Nice!!!

							Thanx, Paul

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

[Johannes Weiner]

On Mon, Feb 11, 2019 at 05:36:06PM -0800, Matthew Wilcox wrote:
> On Fri, Feb 08, 2019 at 11:44:07PM -0800, Paul E. McKenney wrote:
> > 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:
> > > >   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);
> 
> So the problem here is the opposite of what we think it is -- seq->private
> is not marked as being RCU protected.
>
> > 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.

We cannot make struct seq_file->private generally __rcu, but the
cgroup code has a similar thing with kernfs, where it's doing rcu for
its particular use of struct kernfs_node->private. This is how it does
the dereference:

	cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv);

We could do this here as well.

It's ugly, though. I'd also be fine with ignoring the sparse warning.

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

[Matthew Wilcox]

On Tue, Feb 12, 2019 at 11:31:45AM -0500, Johannes Weiner wrote:
> On Mon, Feb 11, 2019 at 05:36:06PM -0800, Matthew Wilcox wrote:
> > On Fri, Feb 08, 2019 at 11:44:07PM -0800, Paul E. McKenney wrote:
> > > 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:
> > > > >   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);
> > 
> > So the problem here is the opposite of what we think it is -- seq->private
> > is not marked as being RCU protected.
> >
> > > 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.
> 
> We cannot make struct seq_file->private generally __rcu, but the
> cgroup code has a similar thing with kernfs, where it's doing rcu for
> its particular use of struct kernfs_node->private. This is how it does
> the dereference:
> 
> 	cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv);
> 
> We could do this here as well.
> 
> It's ugly, though. I'd also be fine with ignoring the sparse warning.

How about:

+++ b/include/linux/seq_file.h
@@ -26,7 +26,10 @@ struct seq_file {
        const struct seq_operations *op;
        int poll_event;
        const struct file *file;
-       void *private;
+       union {
+               void *private;
+               void __rcu *rcu_private;
+       };
 };
 
 struct seq_operations {

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

[Suren Baghdasaryan]

On Tue, Feb 12, 2019 at 8:35 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Feb 12, 2019 at 11:31:45AM -0500, Johannes Weiner wrote:
> > On Mon, Feb 11, 2019 at 05:36:06PM -0800, Matthew Wilcox wrote:
> > > On Fri, Feb 08, 2019 at 11:44:07PM -0800, Paul E. McKenney wrote:
> > > > 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:
> > > > > >   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);
> > >
> > > So the problem here is the opposite of what we think it is -- seq->private
> > > is not marked as being RCU protected.
> > >
> > > > 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.
> >
> > We cannot make struct seq_file->private generally __rcu, but the
> > cgroup code has a similar thing with kernfs, where it's doing rcu for
> > its particular use of struct kernfs_node->private. This is how it does
> > the dereference:
> >
> >       cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv);
> >
> > We could do this here as well.
> >
> > It's ugly, though. I'd also be fine with ignoring the sparse warning.
>
> How about:
>
> +++ b/include/linux/seq_file.h
> @@ -26,7 +26,10 @@ struct seq_file {
>         const struct seq_operations *op;
>         int poll_event;
>         const struct file *file;
> -       void *private;
> +       union {
> +               void *private;
> +               void __rcu *rcu_private;
> +       };
>  };
>
>  struct seq_operations {
>

Personally I would prefer cgrp = rcu_dereference(*(void __rcu __force
**)&kn->priv); as it's more localized change but if union would be
preferable I'll roll that into the next version of psi monitor.
Thanks,
Suren.