[PATCH tip/core/rcu 09/12] fs: Substitute rcu_access_pointer() for rcu_dereference_raw()

[PATCH tip/core/rcu 09/12] fs: Substitute rcu_access_pointer() for rcu_dereference_raw()

[Paul E. McKenney]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

(Trivial patch.)

If the code is looking at the RCU-protected pointer itself, but not
dereferencing it, the rcu_dereference() functions can be downgraded to
rcu_access_pointer().  This commit makes this downgrade in __alloc_fd(),
which simply compares the RCU-protected pointer against NULL with no
dereferencing.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/file.c b/fs/file.c
index db25c2bdfe46..18f7d27855c4 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -497,7 +497,7 @@ repeat:
 	error = fd;
 #if 1
 	/* Sanity check */
-	if (rcu_dereference_raw(fdt->fd[fd]) != NULL) {
+	if (rcu_access_pointer(fdt->fd[fd]) != NULL) {
 		printk(KERN_WARNING "alloc_fd: slot %d not NULL!\n", fd);
 		rcu_assign_pointer(fdt->fd[fd], NULL);
 	}
-- 
1.8.1.5

Re: [PATCH tip/core/rcu 09/12] fs: Substitute rcu_access_pointer() for rcu_dereference_raw()

[Josh Triplett]

On Mon, Feb 17, 2014 at 01:35:56PM -0800, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> 
> (Trivial patch.)
> 
> If the code is looking at the RCU-protected pointer itself, but not
> dereferencing it, the rcu_dereference() functions can be downgraded to
> rcu_access_pointer().  This commit makes this downgrade in __alloc_fd(),
> which simply compares the RCU-protected pointer against NULL with no
> dereferencing.
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org

I'm beginning to wonder if this common pattern ought to have an
rcu_pointer_is_null(), which would not return the pointer, only the
boolean.

Regardless, for this patch:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  fs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index db25c2bdfe46..18f7d27855c4 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -497,7 +497,7 @@ repeat:
>  	error = fd;
>  #if 1
>  	/* Sanity check */
> -	if (rcu_dereference_raw(fdt->fd[fd]) != NULL) {
> +	if (rcu_access_pointer(fdt->fd[fd]) != NULL) {
>  		printk(KERN_WARNING "alloc_fd: slot %d not NULL!\n", fd);
>  		rcu_assign_pointer(fdt->fd[fd], NULL);
>  	}
> -- 
> 1.8.1.5
> 

Re: [PATCH tip/core/rcu 09/12] fs: Substitute rcu_access_pointer() for rcu_dereference_raw()

[Paul E. McKenney]

On Mon, Feb 17, 2014 at 02:00:15PM -0800, Josh Triplett wrote:
> On Mon, Feb 17, 2014 at 01:35:56PM -0800, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > (Trivial patch.)
> > 
> > If the code is looking at the RCU-protected pointer itself, but not
> > dereferencing it, the rcu_dereference() functions can be downgraded to
> > rcu_access_pointer().  This commit makes this downgrade in __alloc_fd(),
> > which simply compares the RCU-protected pointer against NULL with no
> > dereferencing.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: linux-fsdevel@vger.kernel.org
> 
> I'm beginning to wonder if this common pattern ought to have an
> rcu_pointer_is_null(), which would not return the pointer, only the
> boolean.

Or perhaps an rcu_compare_pointer() to also handle the various cases like:

	if (rcu_dereference_raw(foop) == barp) ...

I added the problem to the RCU cleanup list on the OPW site, and
your solution or my elaboration of it might be the right thing to do.
(Inspected all 1300 uses of members of the rcu_dereference() family of
functions last week, and was feeling a bit buggy-eyed at the end...)

							Thanx, Paul

> Regardless, for this patch:
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> 
> >  fs/file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/file.c b/fs/file.c
> > index db25c2bdfe46..18f7d27855c4 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -497,7 +497,7 @@ repeat:
> >  	error = fd;
> >  #if 1
> >  	/* Sanity check */
> > -	if (rcu_dereference_raw(fdt->fd[fd]) != NULL) {
> > +	if (rcu_access_pointer(fdt->fd[fd]) != NULL) {
> >  		printk(KERN_WARNING "alloc_fd: slot %d not NULL!\n", fd);
> >  		rcu_assign_pointer(fdt->fd[fd], NULL);
> >  	}
> > -- 
> > 1.8.1.5
> > 
> 

Re: [PATCH tip/core/rcu 09/12] fs: Substitute rcu_access_pointer() for rcu_dereference_raw()

[Josh Triplett]

On Mon, Feb 17, 2014 at 03:05:11PM -0800, Paul E. McKenney wrote:
> On Mon, Feb 17, 2014 at 02:00:15PM -0800, Josh Triplett wrote:
> > On Mon, Feb 17, 2014 at 01:35:56PM -0800, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > 
> > > (Trivial patch.)
> > > 
> > > If the code is looking at the RCU-protected pointer itself, but not
> > > dereferencing it, the rcu_dereference() functions can be downgraded to
> > > rcu_access_pointer().  This commit makes this downgrade in __alloc_fd(),
> > > which simply compares the RCU-protected pointer against NULL with no
> > > dereferencing.
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > Cc: linux-fsdevel@vger.kernel.org
> > 
> > I'm beginning to wonder if this common pattern ought to have an
> > rcu_pointer_is_null(), which would not return the pointer, only the
> > boolean.
> 
> Or perhaps an rcu_compare_pointer() to also handle the various cases like:
> 
> 	if (rcu_dereference_raw(foop) == barp) ...
> 
> I added the problem to the RCU cleanup list on the OPW site, and
> your solution or my elaboration of it might be the right thing to do.
> (Inspected all 1300 uses of members of the rcu_dereference() family of
> functions last week, and was feeling a bit buggy-eyed at the end...)

rcu_pointer_eq and/or rcu_pointer_neq might make sense, yeah, as
self-documenting versions of the most sensible way to do the operation,
to steer people away from rcu_dereference or rcu_dereference_raw.

- Jsoh Triplett

> 							Thanx, Paul
> 
> > Regardless, for this patch:
> > Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> > 
> > >  fs/file.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/file.c b/fs/file.c
> > > index db25c2bdfe46..18f7d27855c4 100644
> > > --- a/fs/file.c
> > > +++ b/fs/file.c
> > > @@ -497,7 +497,7 @@ repeat:
> > >  	error = fd;
> > >  #if 1
> > >  	/* Sanity check */
> > > -	if (rcu_dereference_raw(fdt->fd[fd]) != NULL) {
> > > +	if (rcu_access_pointer(fdt->fd[fd]) != NULL) {
> > >  		printk(KERN_WARNING "alloc_fd: slot %d not NULL!\n", fd);
> > >  		rcu_assign_pointer(fdt->fd[fd], NULL);
> > >  	}
> > > -- 
> > > 1.8.1.5
> > > 
> > 
> 

Re: [PATCH tip/core/rcu 09/12] fs: Substitute rcu_access_pointer() for rcu_dereference_raw()

[Paul E. McKenney]

On Mon, Feb 17, 2014 at 04:04:31PM -0800, Josh Triplett wrote:
> On Mon, Feb 17, 2014 at 03:05:11PM -0800, Paul E. McKenney wrote:
> > On Mon, Feb 17, 2014 at 02:00:15PM -0800, Josh Triplett wrote:
> > > On Mon, Feb 17, 2014 at 01:35:56PM -0800, Paul E. McKenney wrote:
> > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > > 
> > > > (Trivial patch.)
> > > > 
> > > > If the code is looking at the RCU-protected pointer itself, but not
> > > > dereferencing it, the rcu_dereference() functions can be downgraded to
> > > > rcu_access_pointer().  This commit makes this downgrade in __alloc_fd(),
> > > > which simply compares the RCU-protected pointer against NULL with no
> > > > dereferencing.
> > > > 
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > > Cc: linux-fsdevel@vger.kernel.org
> > > 
> > > I'm beginning to wonder if this common pattern ought to have an
> > > rcu_pointer_is_null(), which would not return the pointer, only the
> > > boolean.
> > 
> > Or perhaps an rcu_compare_pointer() to also handle the various cases like:
> > 
> > 	if (rcu_dereference_raw(foop) == barp) ...
> > 
> > I added the problem to the RCU cleanup list on the OPW site, and
> > your solution or my elaboration of it might be the right thing to do.
> > (Inspected all 1300 uses of members of the rcu_dereference() family of
> > functions last week, and was feeling a bit buggy-eyed at the end...)
> 
> rcu_pointer_eq and/or rcu_pointer_neq might make sense, yeah, as
> self-documenting versions of the most sensible way to do the operation,
> to steer people away from rcu_dereference or rcu_dereference_raw.

Good point!  I added this to http://kernelnewbies.org/OPWIntro-RCU?action=show,
crediting you for the idea.

							Thanx, Paul

> - Jsoh Triplett
> 
> > 							Thanx, Paul
> > 
> > > Regardless, for this patch:
> > > Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> > > 
> > > >  fs/file.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/file.c b/fs/file.c
> > > > index db25c2bdfe46..18f7d27855c4 100644
> > > > --- a/fs/file.c
> > > > +++ b/fs/file.c
> > > > @@ -497,7 +497,7 @@ repeat:
> > > >  	error = fd;
> > > >  #if 1
> > > >  	/* Sanity check */
> > > > -	if (rcu_dereference_raw(fdt->fd[fd]) != NULL) {
> > > > +	if (rcu_access_pointer(fdt->fd[fd]) != NULL) {
> > > >  		printk(KERN_WARNING "alloc_fd: slot %d not NULL!\n", fd);
> > > >  		rcu_assign_pointer(fdt->fd[fd], NULL);
> > > >  	}
> > > > -- 
> > > > 1.8.1.5
> > > > 
> > > 
> > 
>