[RFC][PATCH] rcu: Clean up TASKS_RCU() abuse

[RFC][PATCH] rcu: Clean up TASKS_RCU() abuse

[Peter Zijlstra]

Hi

I recently ran into TASKS_RCU() and wondered why we can't use normal
coding patterns to do the same.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/rcupdate.h | 33 +++++++++++++++++++++++++++++++--
 kernel/exit.c            |  9 +++------
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index a0189ba67fde..15a82372b372 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -389,7 +389,7 @@ static inline void rcu_init_nohz(void)
  * macro rather than an inline function to avoid #include hell.
  */
 #ifdef CONFIG_TASKS_RCU
-#define TASKS_RCU(x) x
+
 extern struct srcu_struct tasks_rcu_exit_srcu;
 #define rcu_note_voluntary_context_switch(t) \
 	do { \
@@ -397,9 +397,38 @@ extern struct srcu_struct tasks_rcu_exit_srcu;
 		if (READ_ONCE((t)->rcu_tasks_holdout)) \
 			WRITE_ONCE((t)->rcu_tasks_holdout, false); \
 	} while (0)
+
+static inline int tasks_rcu_read_lock(void)
+{
+	int idx;
+
+	preempt_disable();
+	idx = __srcu_read_lock(&tasks_rcu_exit_srcu);
+	preempt_enable();
+
+	return idx;
+}
+
+static inline void tasks_rcu_read_unlock(int idx)
+{
+	preempt_disable();
+	__srcu_read_unlock(&tasks_rcu_exit_srcu, idx);
+	preempt_enable();
+}
+
 #else /* #ifdef CONFIG_TASKS_RCU */
-#define TASKS_RCU(x) do { } while (0)
+
 #define rcu_note_voluntary_context_switch(t)	rcu_all_qs()
+
+static inline int tasks_rcu_read_lock(void)
+{
+	return 0;
+}
+
+static inline void tasks_rcu_read_unlock(int idx)
+{
+}
+
 #endif /* #else #ifdef CONFIG_TASKS_RCU */
 
 /**
diff --git a/kernel/exit.c b/kernel/exit.c
index 07110c6020a0..cd5644baeb22 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -653,8 +653,7 @@ static inline void check_stack_usage(void) {}
 void do_exit(long code)
 {
 	struct task_struct *tsk = current;
-	int group_dead;
-	TASKS_RCU(int tasks_rcu_i);
+	int group_dead, tasks_rcu_i;
 
 	profile_task_exit(tsk);
 
@@ -763,9 +762,7 @@ void do_exit(long code)
 	 */
 	flush_ptrace_hw_breakpoint(tsk);
 
-	TASKS_RCU(preempt_disable());
-	TASKS_RCU(tasks_rcu_i = __srcu_read_lock(&tasks_rcu_exit_srcu));
-	TASKS_RCU(preempt_enable());
+	tasks_rcu_i = tasks_rcu_read_lock();
 	exit_notify(tsk, group_dead);
 	proc_exit_connector(tsk);
 #ifdef CONFIG_NUMA
@@ -805,7 +802,7 @@ void do_exit(long code)
 	if (tsk->nr_dirtied)
 		__this_cpu_add(dirty_throttle_leaks, tsk->nr_dirtied);
 	exit_rcu();
-	TASKS_RCU(__srcu_read_unlock(&tasks_rcu_exit_srcu, tasks_rcu_i));
+	tasks_rcu_read_unlock(tasks_rcu_i);
 
 	/*
 	 * The setting of TASK_RUNNING by try_to_wake_up() may be delayed

Re: [RFC][PATCH] rcu: Clean up TASKS_RCU() abuse

[Paul E. McKenney]

On Wed, Nov 11, 2015 at 01:23:10PM +0100, Peter Zijlstra wrote:
> Hi
> 
> I recently ran into TASKS_RCU() and wondered why we can't use normal
> coding patterns to do the same.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Well, I cannot get too excited either way, but the diffstat for this
change is not particularly favorable.

							Thanx, Paul

> ---
>  include/linux/rcupdate.h | 33 +++++++++++++++++++++++++++++++--
>  kernel/exit.c            |  9 +++------
>  2 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index a0189ba67fde..15a82372b372 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -389,7 +389,7 @@ static inline void rcu_init_nohz(void)
>   * macro rather than an inline function to avoid #include hell.
>   */
>  #ifdef CONFIG_TASKS_RCU
> -#define TASKS_RCU(x) x
> +
>  extern struct srcu_struct tasks_rcu_exit_srcu;
>  #define rcu_note_voluntary_context_switch(t) \
>  	do { \
> @@ -397,9 +397,38 @@ extern struct srcu_struct tasks_rcu_exit_srcu;
>  		if (READ_ONCE((t)->rcu_tasks_holdout)) \
>  			WRITE_ONCE((t)->rcu_tasks_holdout, false); \
>  	} while (0)
> +
> +static inline int tasks_rcu_read_lock(void)
> +{
> +	int idx;
> +
> +	preempt_disable();
> +	idx = __srcu_read_lock(&tasks_rcu_exit_srcu);
> +	preempt_enable();
> +
> +	return idx;
> +}
> +
> +static inline void tasks_rcu_read_unlock(int idx)
> +{
> +	preempt_disable();
> +	__srcu_read_unlock(&tasks_rcu_exit_srcu, idx);
> +	preempt_enable();
> +}
> +
>  #else /* #ifdef CONFIG_TASKS_RCU */
> -#define TASKS_RCU(x) do { } while (0)
> +
>  #define rcu_note_voluntary_context_switch(t)	rcu_all_qs()
> +
> +static inline int tasks_rcu_read_lock(void)
> +{
> +	return 0;
> +}
> +
> +static inline void tasks_rcu_read_unlock(int idx)
> +{
> +}
> +
>  #endif /* #else #ifdef CONFIG_TASKS_RCU */
> 
>  /**
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 07110c6020a0..cd5644baeb22 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -653,8 +653,7 @@ static inline void check_stack_usage(void) {}
>  void do_exit(long code)
>  {
>  	struct task_struct *tsk = current;
> -	int group_dead;
> -	TASKS_RCU(int tasks_rcu_i);
> +	int group_dead, tasks_rcu_i;
> 
>  	profile_task_exit(tsk);
> 
> @@ -763,9 +762,7 @@ void do_exit(long code)
>  	 */
>  	flush_ptrace_hw_breakpoint(tsk);
> 
> -	TASKS_RCU(preempt_disable());
> -	TASKS_RCU(tasks_rcu_i = __srcu_read_lock(&tasks_rcu_exit_srcu));
> -	TASKS_RCU(preempt_enable());
> +	tasks_rcu_i = tasks_rcu_read_lock();
>  	exit_notify(tsk, group_dead);
>  	proc_exit_connector(tsk);
>  #ifdef CONFIG_NUMA
> @@ -805,7 +802,7 @@ void do_exit(long code)
>  	if (tsk->nr_dirtied)
>  		__this_cpu_add(dirty_throttle_leaks, tsk->nr_dirtied);
>  	exit_rcu();
> -	TASKS_RCU(__srcu_read_unlock(&tasks_rcu_exit_srcu, tasks_rcu_i));
> +	tasks_rcu_read_unlock(tasks_rcu_i);
> 
>  	/*
>  	 * The setting of TASK_RUNNING by try_to_wake_up() may be delayed
> 

Re: [RFC][PATCH] rcu: Clean up TASKS_RCU() abuse

[Peter Zijlstra]

On Wed, Nov 11, 2015 at 04:49:40AM -0800, Paul E. McKenney wrote:
> On Wed, Nov 11, 2015 at 01:23:10PM +0100, Peter Zijlstra wrote:
> > Hi
> > 
> > I recently ran into TASKS_RCU() and wondered why we can't use normal
> > coding patterns to do the same.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Well, I cannot get too excited either way, but the diffstat for this
> change is not particularly favorable.

It also doesn't build with TASKS_RCU enabled it turns out. But the point
is, nowhere else do we use this pattern. We always provide functions.

Re: [RFC][PATCH] rcu: Clean up TASKS_RCU() abuse

[Paul E. McKenney]

On Wed, Nov 11, 2015 at 02:23:04PM +0100, Peter Zijlstra wrote:
> On Wed, Nov 11, 2015 at 04:49:40AM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 11, 2015 at 01:23:10PM +0100, Peter Zijlstra wrote:
> > > Hi
> > > 
> > > I recently ran into TASKS_RCU() and wondered why we can't use normal
> > > coding patterns to do the same.
> > > 
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > 
> > Well, I cannot get too excited either way, but the diffstat for this
> > change is not particularly favorable.
> 
> It also doesn't build with TASKS_RCU enabled it turns out. But the point
> is, nowhere else do we use this pattern. We always provide functions.

RCU_TRACE() another very similar macro, and has been in place for quite
some time.  Still within RCU, admittedly, but it does exist.

							Thanx, Paul