[PATCH] kernel: trace_syscalls: Replace rcu_assign_pointer() with RCU_INIT_POINTER()

[PATCH] kernel: trace_syscalls: Replace rcu_assign_pointer() with RCU_INIT_POINTER()

[Andreea-Cristina Bernat]

The uses of "rcu_assign_pointer()" are NULLing out the pointers.
According to RCU_INIT_POINTER()'s block comment:
"1.   This use of RCU_INIT_POINTER() is NULLing out the pointer"
it is better to use it instead of rcu_assign_pointer() because it has a
smaller overhead.

The following Coccinelle semantic patch was used:
@@
@@

- rcu_assign_pointer
+ RCU_INIT_POINTER
  (..., NULL)

Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
---
 kernel/trace/trace_syscalls.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 759d5e0..4dc8b79 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -425,7 +425,7 @@ static void unreg_event_syscall_enter(struct ftrace_event_file *file,
 		return;
 	mutex_lock(&syscall_trace_lock);
 	tr->sys_refcount_enter--;
-	rcu_assign_pointer(tr->enter_syscall_files[num], NULL);
+	RCU_INIT_POINTER(tr->enter_syscall_files[num], NULL);
 	if (!tr->sys_refcount_enter)
 		unregister_trace_sys_enter(ftrace_syscall_enter, tr);
 	mutex_unlock(&syscall_trace_lock);
@@ -463,7 +463,7 @@ static void unreg_event_syscall_exit(struct ftrace_event_file *file,
 		return;
 	mutex_lock(&syscall_trace_lock);
 	tr->sys_refcount_exit--;
-	rcu_assign_pointer(tr->exit_syscall_files[num], NULL);
+	RCU_INIT_POINTER(tr->exit_syscall_files[num], NULL);
 	if (!tr->sys_refcount_exit)
 		unregister_trace_sys_exit(ftrace_syscall_exit, tr);
 	mutex_unlock(&syscall_trace_lock);
-- 
1.9.1

Re: [PATCH] kernel: trace_syscalls: Replace rcu_assign_pointer() with RCU_INIT_POINTER()

[Steven Rostedt]

On Fri, 22 Aug 2014 17:28:22 +0300
Andreea-Cristina Bernat <bernat.ada@gmail.com> wrote:

> The uses of "rcu_assign_pointer()" are NULLing out the pointers.
> According to RCU_INIT_POINTER()'s block comment:
> "1.   This use of RCU_INIT_POINTER() is NULLing out the pointer"
> it is better to use it instead of rcu_assign_pointer() because it has a
> smaller overhead.
> 
> The following Coccinelle semantic patch was used:
> @@
> @@
> 
> - rcu_assign_pointer
> + RCU_INIT_POINTER
>   (..., NULL)

I guess I can add this. It's a very slow path thus it isn't critical.

Although, I hate the name. Perhaps we should add another macro called
RCU_CLEAR_POINTER() or something that just nulls it. That way it
documents the use. To me, INIT means the pointer is being initialized,
where in reality it's just being cleared. I guess one could argue that
the pointer is being "re-initialized".

Thanks!

-- Steve

> 
> Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
> ---
>  kernel/trace/trace_syscalls.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 759d5e0..4dc8b79 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -425,7 +425,7 @@ static void unreg_event_syscall_enter(struct ftrace_event_file *file,
>  		return;
>  	mutex_lock(&syscall_trace_lock);
>  	tr->sys_refcount_enter--;
> -	rcu_assign_pointer(tr->enter_syscall_files[num], NULL);
> +	RCU_INIT_POINTER(tr->enter_syscall_files[num], NULL);
>  	if (!tr->sys_refcount_enter)
>  		unregister_trace_sys_enter(ftrace_syscall_enter, tr);
>  	mutex_unlock(&syscall_trace_lock);
> @@ -463,7 +463,7 @@ static void unreg_event_syscall_exit(struct ftrace_event_file *file,
>  		return;
>  	mutex_lock(&syscall_trace_lock);
>  	tr->sys_refcount_exit--;
> -	rcu_assign_pointer(tr->exit_syscall_files[num], NULL);
> +	RCU_INIT_POINTER(tr->exit_syscall_files[num], NULL);
>  	if (!tr->sys_refcount_exit)
>  		unregister_trace_sys_exit(ftrace_syscall_exit, tr);
>  	mutex_unlock(&syscall_trace_lock);

Re: [PATCH] kernel: trace_syscalls: Replace rcu_assign_pointer() with RCU_INIT_POINTER()

[Paul E. McKenney]

On Fri, Aug 22, 2014 at 11:37:58AM -0400, Steven Rostedt wrote:
> On Fri, 22 Aug 2014 17:28:22 +0300
> Andreea-Cristina Bernat <bernat.ada@gmail.com> wrote:
> 
> > The uses of "rcu_assign_pointer()" are NULLing out the pointers.
> > According to RCU_INIT_POINTER()'s block comment:
> > "1.   This use of RCU_INIT_POINTER() is NULLing out the pointer"
> > it is better to use it instead of rcu_assign_pointer() because it has a
> > smaller overhead.
> > 
> > The following Coccinelle semantic patch was used:
> > @@
> > @@
> > 
> > - rcu_assign_pointer
> > + RCU_INIT_POINTER
> >   (..., NULL)
> 
> I guess I can add this. It's a very slow path thus it isn't critical.
> 
> Although, I hate the name. Perhaps we should add another macro called
> RCU_CLEAR_POINTER() or something that just nulls it. That way it
> documents the use. To me, INIT means the pointer is being initialized,
> where in reality it's just being cleared. I guess one could argue that
> the pointer is being "re-initialized".

I considered that, but there end up being three separate use cases
for this thing:

1.	NULLing the pointer, as in this case.

2.	Initializing the pointer at a time when no readers have a
	reference to that pointer.  (In this case, there is presumably
	a later rcu_assign_pointer() that makes the whole thing visible
	to readers.)

3.	Rearranging data that is already visible to readers, the usual
	example being removing an element -- readers can already see
	the successor in this case.

Having three different APIs for identical macros seemed like overkill
to me.  Especially given that people already complain about the RCU
API being too big.  :-(

							Thanx, Paul

> Thanks!
> 
> -- Steve
> 
> > 
> > Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
> > ---
> >  kernel/trace/trace_syscalls.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> > index 759d5e0..4dc8b79 100644
> > --- a/kernel/trace/trace_syscalls.c
> > +++ b/kernel/trace/trace_syscalls.c
> > @@ -425,7 +425,7 @@ static void unreg_event_syscall_enter(struct ftrace_event_file *file,
> >  		return;
> >  	mutex_lock(&syscall_trace_lock);
> >  	tr->sys_refcount_enter--;
> > -	rcu_assign_pointer(tr->enter_syscall_files[num], NULL);
> > +	RCU_INIT_POINTER(tr->enter_syscall_files[num], NULL);
> >  	if (!tr->sys_refcount_enter)
> >  		unregister_trace_sys_enter(ftrace_syscall_enter, tr);
> >  	mutex_unlock(&syscall_trace_lock);
> > @@ -463,7 +463,7 @@ static void unreg_event_syscall_exit(struct ftrace_event_file *file,
> >  		return;
> >  	mutex_lock(&syscall_trace_lock);
> >  	tr->sys_refcount_exit--;
> > -	rcu_assign_pointer(tr->exit_syscall_files[num], NULL);
> > +	RCU_INIT_POINTER(tr->exit_syscall_files[num], NULL);
> >  	if (!tr->sys_refcount_exit)
> >  		unregister_trace_sys_exit(ftrace_syscall_exit, tr);
> >  	mutex_unlock(&syscall_trace_lock);
> 

Re: [PATCH] kernel: trace_syscalls: Replace rcu_assign_pointer() with RCU_INIT_POINTER()

[Steven Rostedt]

On Mon, 25 Aug 2014 15:56:54 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

 
> > I guess I can add this. It's a very slow path thus it isn't critical.
> > 
> > Although, I hate the name. Perhaps we should add another macro called
> > RCU_CLEAR_POINTER() or something that just nulls it. That way it
> > documents the use. To me, INIT means the pointer is being initialized,
> > where in reality it's just being cleared. I guess one could argue that
> > the pointer is being "re-initialized".
> 
> I considered that, but there end up being three separate use cases
> for this thing:
> 
> 1.	NULLing the pointer, as in this case.
> 
> 2.	Initializing the pointer at a time when no readers have a
> 	reference to that pointer.  (In this case, there is presumably
> 	a later rcu_assign_pointer() that makes the whole thing visible
> 	to readers.)
> 
> 3.	Rearranging data that is already visible to readers, the usual
> 	example being removing an element -- readers can already see
> 	the successor in this case.
> 
> Having three different APIs for identical macros seemed like overkill
> to me.  Especially given that people already complain about the RCU
> API being too big.  :-(
> 

Yeah, understood. But I think CLEAR is better than INIT as it says
what it's doing more than what it is for. In all three above, we want
to clear the pointer, but in only one case we want to initialize it.

But this is bikeshedding, and not worth the time of this dicussion.

No need to look further. Nothings going on here. Move along people or
I'll have to get my pepper spray out.

-- Steve

Re: [PATCH] kernel: trace_syscalls: Replace rcu_assign_pointer() with RCU_INIT_POINTER()

[Paul E. McKenney]

On Mon, Aug 25, 2014 at 07:05:54PM -0400, Steven Rostedt wrote:
> On Mon, 25 Aug 2014 15:56:54 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> 
> > > I guess I can add this. It's a very slow path thus it isn't critical.
> > > 
> > > Although, I hate the name. Perhaps we should add another macro called
> > > RCU_CLEAR_POINTER() or something that just nulls it. That way it
> > > documents the use. To me, INIT means the pointer is being initialized,
> > > where in reality it's just being cleared. I guess one could argue that
> > > the pointer is being "re-initialized".
> > 
> > I considered that, but there end up being three separate use cases
> > for this thing:
> > 
> > 1.	NULLing the pointer, as in this case.
> > 
> > 2.	Initializing the pointer at a time when no readers have a
> > 	reference to that pointer.  (In this case, there is presumably
> > 	a later rcu_assign_pointer() that makes the whole thing visible
> > 	to readers.)
> > 
> > 3.	Rearranging data that is already visible to readers, the usual
> > 	example being removing an element -- readers can already see
> > 	the successor in this case.
> > 
> > Having three different APIs for identical macros seemed like overkill
> > to me.  Especially given that people already complain about the RCU
> > API being too big.  :-(
> > 
> 
> Yeah, understood. But I think CLEAR is better than INIT as it says
> what it's doing more than what it is for. In all three above, we want
> to clear the pointer, but in only one case we want to initialize it.
> 
> But this is bikeshedding, and not worth the time of this dicussion.

PLAID!!!  We must paint the bikeshed plaid!

> No need to look further. Nothings going on here. Move along people or
> I'll have to get my pepper spray out.

;-) ;-) ;-)

							Thanx, Paul