kernel: trace_syscalls: Replace rcu_assign_pointer() with RCU_INIT_POINTER()

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 <[email protected]>

---
 kernel/trace/trace_syscalls.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/trace/trace_syscalls.c +2 -2

index 759d5e0..4dc8b79 100644
@@ -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);

Quoted 6 lines · On Fri, 22 Aug 2014 17:28:22 +0300 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.

Quoted 7 lines

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

Quoted 28 lines

Signed-off-by Andreea-Cristina Bernat <[email protected]>

---
 kernel/trace/trace_syscalls.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/trace/trace_syscalls.c +2 -2

index 759d5e0..4dc8b79 100644
@@ -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);

Quoted 7 lines

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

Quoted 18 lines · On Mon, 25 Aug 2014 15:56:54 -0700 "Paul E. McKenney"…

Quoted 7 lines

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

Quoted 8 lines

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.