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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/