Skip to contentSkip to conversation

cleanups, fixes for rcu/dev

PATCH In review Joel Fernandes 5 participants 24 messages May 23, 2018 – Jul 17, 2018 RCU

lore
Share & tools
lore

Joel Fernandes

author
#0 lore raw

cover cleanups, fixes for rcu/dev

Hi Paul,

Here's an updated series for rcu/dev addressing some of your comments along with the performance optimization. I also threw in the RCU-tasks speed up patch again, hopefully that doesn't annoy you :)

cheers,
- Joel

Joel Fernandes (Google) (4):
rcu: Speed up calling of RCU tasks callbacks
rcu: Add comment documenting how rcu_seq_snap works
rcu: Use better variable names in funnel locking loop
rcu: Identify grace period is in progress as we advance up the tree

 kernel/rcu/rcu.h    | 34 ++++++++++++++++++++++++++-
 kernel/rcu/tree.c   | 57 +++++++++++++++++++++++----------------------
 kernel/rcu/update.c | 12 +++++++++-
 3 files changed, 73 insertions(+), 30 deletions(-)
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
author
#1 lore raw

PATCH 1/4 rcu: Speed up calling of RCU tasks callbacks

From: "Joel Fernandes (Google)" <[email protected]>

RCU tasks callbacks can take at least 1 second before the callbacks are executed. This happens even if the hold-out tasks enter their quiescent states quickly. I noticed this when I was testing trampoline callback execution.

To test the trampoline freeing, I wrote a simple script:
cd /sys/kernel/debug/tracing/
echo '__schedule_bug:traceon' > set_ftrace_filter; echo '!__schedule_bug:traceon' > set_ftrace_filter;

In the background I had simple bash while loop:
while [ 1 ]; do x=1; done &

Total time of completion of above commands in seconds:

With this patch:

real    0m0.179s
user    0m0.000s
sys     0m0.054s

Without this patch:

real    0m1.098s
user    0m0.000s
sys     0m0.053s

That's a greater than 6X speed up in performance. In order to accomplish this, I am waiting for HZ/10 time before entering the hold-out checking loop. The loop still preserves its checking of held tasks every 1 second as before, in case this first test doesn't succeed.

Cc Steven Rostedt <[email protected]>
Cc Peter Zilstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Boqun Feng <[email protected]>
Cc Paul McKenney <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Joel Fernandes (Google) <[email protected]>
---
 kernel/rcu/update.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
kernel/rcu/update.c +11 -1
index 5783bdf86e5a..a28698e44b08 100644
@@ -743,6 +743,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 */
 		synchronize_srcu(&tasks_rcu_exit_srcu);
 
+		/*
+		 * Wait a little bit incase held tasks are released
+		 * during their next timer ticks.
+		 */
+		schedule_timeout_interruptible(HZ/10);
+
 		/*
 		 * Each pass through the following loop scans the list
 		 * of holdout tasks, removing any that are no longer

@@ -755,7 +761,6 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			int rtst;
 			struct task_struct *t1;
 
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);

@@ -768,6 +773,11 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 				check_holdout_task(t, needreport, &firstreport);
 				cond_resched();
 			}
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			schedule_timeout_interruptible(HZ);
 		}
 
 		/*
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
#2 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 26 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

RCU tasks callbacks can take at least 1 second before the callbacks are executed. This happens even if the hold-out tasks enter their quiescent states quickly. I noticed this when I was testing trampoline callback execution.

To test the trampoline freeing, I wrote a simple script:
cd /sys/kernel/debug/tracing/
echo '__schedule_bug:traceon' > set_ftrace_filter; echo '!__schedule_bug:traceon' > set_ftrace_filter;

In the background I had simple bash while loop:
while [ 1 ]; do x=1; done &

Total time of completion of above commands in seconds:

With this patch:

real    0m0.179s
user    0m0.000s
sys     0m0.054s

Without this patch:

real    0m1.098s
user    0m0.000s
sys     0m0.053s
Quoted 6 lines

That's a greater than 6X speed up in performance. In order to accomplish this, I am waiting for HZ/10 time before entering the hold-out checking loop. The loop still preserves its checking of held tasks every 1 second as before, in case this first test doesn't succeed.

Cc Steven Rostedt <[email protected]>

Given an ack from Steven, I would be happy to take this, give or take some nits below.

Thanx, Paul

Quoted 16 lines
Cc Peter Zilstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Boqun Feng <[email protected]>
Cc Paul McKenney <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Joel Fernandes (Google) <[email protected]>
---
 kernel/rcu/update.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
kernel/rcu/update.c +0 -0
index 5783bdf86e5a..a28698e44b08 100644

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 747

Quoted 5 lines
kernel/rcu/update.c +2 -0
@@ -743,6 +743,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 */
 		synchronize_srcu(&tasks_rcu_exit_srcu);
 
+		/*
+		 * Wait a little bit incase held tasks are released

in case

Quoted 16 lines
kernel/rcu/update.c +4 -1
+		 * during their next timer ticks.
+		 */
+		schedule_timeout_interruptible(HZ/10);
+
 		/*
 		 * Each pass through the following loop scans the list
 		 * of holdout tasks, removing any that are no longer

@@ -755,7 +761,6 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			int rtst;
 			struct task_struct *t1;
 
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 780

Quoted 8 lines
kernel/rcu/update.c +5 -0
@@ -768,6 +773,11 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 				check_holdout_task(t, needreport, &firstreport);
 				cond_resched();
 			}
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			schedule_timeout_interruptible(HZ);

Is there a better way to do this? Can this be converted into a for-loop? Alternatively, would it make sense to have a firsttime local variable initialized to true, to keep the schedule_timeout_interruptible() at the beginning of the loop, but skip it on the first pass through the loop?

Don't get me wrong, what you have looks functionally correct, but duplicating the condition might cause problems later on, for example, should a bug fix be needed in the condition.

Quoted 6 lines
kernel/rcu/update.c +0 -0
 		}
 
 		/*
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
reviewer
#3 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 32 lines · On Wed, 23 May 2018 08:57:34 -0700 "Paul E. McKenney"…
Quoted 31 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

RCU tasks callbacks can take at least 1 second before the callbacks are executed. This happens even if the hold-out tasks enter their quiescent states quickly. I noticed this when I was testing trampoline callback execution.

To test the trampoline freeing, I wrote a simple script:
cd /sys/kernel/debug/tracing/
echo '__schedule_bug:traceon' > set_ftrace_filter; echo '!__schedule_bug:traceon' > set_ftrace_filter;

In the background I had simple bash while loop:
while [ 1 ]; do x=1; done &

Total time of completion of above commands in seconds:

With this patch:

real    0m0.179s
user    0m0.000s
sys     0m0.054s

Without this patch:

real    0m1.098s
user    0m0.000s
sys     0m0.053s

That's a greater than 6X speed up in performance. In order to accomplish this, I am waiting for HZ/10 time before entering the hold-out checking loop. The loop still preserves its checking of held tasks every 1 second as before, in case this first test doesn't succeed.

Quoted 4 lines
Quoted 1 line
Cc Steven Rostedt <[email protected]>

Given an ack from Steven, I would be happy to take this, give or take some nits below.

I'm currently testing it, and trying to understand it better.

Quoted 38 lines

Thanx, Paul

Quoted 16 lines
Cc Peter Zilstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Boqun Feng <[email protected]>
Cc Paul McKenney <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Joel Fernandes (Google) <[email protected]>
---
 kernel/rcu/update.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
kernel/rcu/update.c +0 -0
index 5783bdf86e5a..a28698e44b08 100644

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 747

Quoted 5 lines
kernel/rcu/update.c +2 -0
@@ -743,6 +743,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 */
 		synchronize_srcu(&tasks_rcu_exit_srcu);
 
+		/*
+		 * Wait a little bit incase held tasks are released

in case

Quoted 11 lines
kernel/rcu/update.c +4 -0
+		 * during their next timer ticks.
+		 */
+		schedule_timeout_interruptible(HZ/10);
+
 		/*
 		 * Each pass through the following loop scans the list
 		 * of holdout tasks, removing any that are no longer

@@ -755,7 +761,6 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			int rtst;
 			struct task_struct *t1;
Quoted 13 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 780

Quoted 13 lines
kernel/rcu/update.c +5 -1
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);

@@ -768,6 +773,11 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 				check_holdout_task(t, needreport, &firstreport);
 				cond_resched();
 			}
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			schedule_timeout_interruptible(HZ);

Why is this a full second wait and not the HZ/10 like the others?

-- Steve

Quoted 16 lines

Is there a better way to do this? Can this be converted into a for-loop? Alternatively, would it make sense to have a firsttime local variable initialized to true, to keep the schedule_timeout_interruptible() at the beginning of the loop, but skip it on the first pass through the loop?

Don't get me wrong, what you have looks functionally correct, but duplicating the condition might cause problems later on, for example, should a bug fix be needed in the condition.

Quoted 6 lines
kernel/rcu/update.c +0 -0
 		}
 
 		/*
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
#4 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 35 lines · Steven Rostedt
Quoted 32 lines · On Wed, 23 May 2018 08:57:34 -0700 "Paul E. McKenney"…
Quoted 31 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

RCU tasks callbacks can take at least 1 second before the callbacks are executed. This happens even if the hold-out tasks enter their quiescent states quickly. I noticed this when I was testing trampoline callback execution.

To test the trampoline freeing, I wrote a simple script:
cd /sys/kernel/debug/tracing/
echo '__schedule_bug:traceon' > set_ftrace_filter; echo '!__schedule_bug:traceon' > set_ftrace_filter;

In the background I had simple bash while loop:
while [ 1 ]; do x=1; done &

Total time of completion of above commands in seconds:

With this patch:

real    0m0.179s
user    0m0.000s
sys     0m0.054s

Without this patch:

real    0m1.098s
user    0m0.000s
sys     0m0.053s

That's a greater than 6X speed up in performance. In order to accomplish this, I am waiting for HZ/10 time before entering the hold-out checking loop. The loop still preserves its checking of held tasks every 1 second as before, in case this first test doesn't succeed.

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Cc Steven Rostedt <[email protected]>

Given an ack from Steven, I would be happy to take this, give or take some nits below.

I'm currently testing it, and trying to understand it better.

Very good, thank you!

Quoted 35 lines
Quoted 35 lines
Quoted 16 lines
Cc Peter Zilstra <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Boqun Feng <[email protected]>
Cc Paul McKenney <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Joel Fernandes (Google) <[email protected]>
---
 kernel/rcu/update.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
kernel/rcu/update.c +0 -0
index 5783bdf86e5a..a28698e44b08 100644

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 747

Quoted 5 lines
kernel/rcu/update.c +2 -0
@@ -743,6 +743,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 */
 		synchronize_srcu(&tasks_rcu_exit_srcu);
 
+		/*
+		 * Wait a little bit incase held tasks are released

in case

Quoted 11 lines
kernel/rcu/update.c +4 -0
+		 * during their next timer ticks.
+		 */
+		schedule_timeout_interruptible(HZ/10);
+
 		/*
 		 * Each pass through the following loop scans the list
 		 * of holdout tasks, removing any that are no longer

@@ -755,7 +761,6 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			int rtst;
 			struct task_struct *t1;
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
kernel/rcu/update.c +0 -1
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);

@@ -768,6 +773,11 @@ static int __noreturn rcu_tasks_kthread(void *arg)
Quoted 8 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 780

Quoted 8 lines
kernel/rcu/update.c +5 -0
 				check_holdout_task(t, needreport, &firstreport);
 				cond_resched();
 			}
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			schedule_timeout_interruptible(HZ);

Why is this a full second wait and not the HZ/10 like the others?

The idea is to respond quickly on small idle systems and to reduce the number of possibly quite lengthy traversals of the task list otherwise. I actually considered exponential backoff, but decided to keep it simple, at least to start with.

Thanx, Paul

Quoted 19 lines

-- Steve

Quoted 16 lines

Is there a better way to do this? Can this be converted into a for-loop? Alternatively, would it make sense to have a firsttime local variable initialized to true, to keep the schedule_timeout_interruptible() at the beginning of the loop, but skip it on the first pass through the loop?

Don't get me wrong, what you have looks functionally correct, but duplicating the condition might cause problems later on, for example, should a bug fix be needed in the condition.

Quoted 6 lines
kernel/rcu/update.c +0 -0
 		}
 
 		/*
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
reviewer
#5 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 40 lines · On Wed, 23 May 2018 10:03:03 -0700 "Paul E. McKenney"…
Quoted 24 lines
Quoted 24 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 747

Quoted 10 lines
kernel/rcu/update.c +2 -0
index 5783bdf86e5a..a28698e44b08 100644
@@ -743,6 +743,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 */
 		synchronize_srcu(&tasks_rcu_exit_srcu);
 
+		/*
+		 * Wait a little bit incase held tasks are released

in case

Quoted 11 lines
kernel/rcu/update.c +4 -0
+		 * during their next timer ticks.
+		 */
+		schedule_timeout_interruptible(HZ/10);
+
 		/*
 		 * Each pass through the following loop scans the list
 		 * of holdout tasks, removing any that are no longer

@@ -755,7 +761,6 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			int rtst;
 			struct task_struct *t1;
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
kernel/rcu/update.c +0 -1
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);

@@ -768,6 +773,11 @@ static int __noreturn rcu_tasks_kthread(void *arg)
Quoted 8 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 780

Quoted 8 lines
kernel/rcu/update.c +5 -0
 				check_holdout_task(t, needreport, &firstreport);
 				cond_resched();
 			}
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			schedule_timeout_interruptible(HZ);

Why is this a full second wait and not the HZ/10 like the others?

Quoted 4 lines

The idea is to respond quickly on small idle systems and to reduce the number of possibly quite lengthy traversals of the task list otherwise. I actually considered exponential backoff, but decided to keep it simple, at least to start with.

Ah, now it makes sense. Reading what you wrote, we can still do a backoff and keep it simple. What about the patch below. It appears to have the same performance improvement as Joel's

-- Steve

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

Is there a better way to do this? Can this be converted into a for-loop? Alternatively, would it make sense to have a firsttime local variable initialized to true, to keep the schedule_timeout_interruptible() at the beginning of the loop, but skip it on the first pass through the loop?

Don't get me wrong, what you have looks functionally correct, but duplicating the condition might cause problems later on, for example, should a bug fix be needed in the condition.

kernel/rcu/update.c +11 -2
index 68fa19a5e7bd..c6df9fa916cf 100644
@@ -796,13 +796,22 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 * holdouts.  When the list is empty, we are done.
 		 */
 		lastreport = jiffies;
-		while (!list_empty(&rcu_tasks_holdouts)) {
+		for (;;) {
 			bool firstreport;
 			bool needreport;
 			int rtst;
 			struct task_struct *t1;
+			int fract = 15;
+
+			/* Slowly back off waiting for holdouts */
+			schedule_timeout_interruptible(HZ/fract);
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			if (fract > 1)
+				fract--;
 
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);
#6 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 43 lines · Steven Rostedt
Quoted 40 lines · On Wed, 23 May 2018 10:03:03 -0700 "Paul E. McKenney"…
Quoted 39 lines
Quoted 24 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 747

Quoted 10 lines
kernel/rcu/update.c +2 -0
index 5783bdf86e5a..a28698e44b08 100644
@@ -743,6 +743,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 */
 		synchronize_srcu(&tasks_rcu_exit_srcu);
 
+		/*
+		 * Wait a little bit incase held tasks are released

in case

Quoted 11 lines
kernel/rcu/update.c +4 -0
+		 * during their next timer ticks.
+		 */
+		schedule_timeout_interruptible(HZ/10);
+
 		/*
 		 * Each pass through the following loop scans the list
 		 * of holdout tasks, removing any that are no longer

@@ -755,7 +761,6 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			int rtst;
 			struct task_struct *t1;
Quoted 13 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 780

Quoted 13 lines
kernel/rcu/update.c +5 -1
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);

@@ -768,6 +773,11 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 				check_holdout_task(t, needreport, &firstreport);
 				cond_resched();
 			}
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			schedule_timeout_interruptible(HZ);

Why is this a full second wait and not the HZ/10 like the others?

Quoted 8 lines
Quoted 4 lines

The idea is to respond quickly on small idle systems and to reduce the number of possibly quite lengthy traversals of the task list otherwise. I actually considered exponential backoff, but decided to keep it simple, at least to start with.

Ah, now it makes sense. Reading what you wrote, we can still do a backoff and keep it simple. What about the patch below. It appears to have the same performance improvement as Joel's

Looks plausible to me!

Joel, do you see any gotchas in Steve's patch?

Thanx, Paul

Quoted 44 lines

-- Steve

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

Is there a better way to do this? Can this be converted into a for-loop? Alternatively, would it make sense to have a firsttime local variable initialized to true, to keep the schedule_timeout_interruptible() at the beginning of the loop, but skip it on the first pass through the loop?

Don't get me wrong, what you have looks functionally correct, but duplicating the condition might cause problems later on, for example, should a bug fix be needed in the condition.

kernel/rcu/update.c +11 -2
index 68fa19a5e7bd..c6df9fa916cf 100644
@@ -796,13 +796,22 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 * holdouts.  When the list is empty, we are done.
 		 */
 		lastreport = jiffies;
-		while (!list_empty(&rcu_tasks_holdouts)) {
+		for (;;) {
 			bool firstreport;
 			bool needreport;
 			int rtst;
 			struct task_struct *t1;
+			int fract = 15;
+
+			/* Slowly back off waiting for holdouts */
+			schedule_timeout_interruptible(HZ/fract);
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			if (fract > 1)
+				fract--;
 
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);
#7 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 49 lines · Paul E. McKenney
Quoted 48 lines · Steven Rostedt
Quoted 44 lines · On Wed, 23 May 2018 10:03:03 -0700 "Paul E. McKenney"…
Quoted 24 lines
Quoted 24 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 747

Quoted 10 lines
kernel/rcu/update.c +2 -0
index 5783bdf86e5a..a28698e44b08 100644
@@ -743,6 +743,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 */
 		synchronize_srcu(&tasks_rcu_exit_srcu);
 
+		/*
+		 * Wait a little bit incase held tasks are released

in case

Quoted 11 lines
kernel/rcu/update.c +4 -0
+		 * during their next timer ticks.
+		 */
+		schedule_timeout_interruptible(HZ/10);
+
 		/*
 		 * Each pass through the following loop scans the list
 		 * of holdout tasks, removing any that are no longer

@@ -755,7 +761,6 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			int rtst;
 			struct task_struct *t1;
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
kernel/rcu/update.c +0 -1
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);

@@ -768,6 +773,11 @@ static int __noreturn rcu_tasks_kthread(void *arg)
Quoted 8 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg)

Quoted 8 lines
kernel/rcu/update.c +5 -0
 				check_holdout_task(t, needreport, &firstreport);
 				cond_resched();
 			}
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			schedule_timeout_interruptible(HZ);

Why is this a full second wait and not the HZ/10 like the others?

The idea is to respond quickly on small idle systems and to reduce the number of possibly quite lengthy traversals of the task list otherwise. I actually considered exponential backoff, but decided to keep it simple, at least to start with.

Quoted 7 lines
Quoted 3 lines

Ah, now it makes sense. Reading what you wrote, we can still do a backoff and keep it simple. What about the patch below. It appears to have the same performance improvement as Joel's

Looks plausible to me!

Joel, do you see any gotchas in Steve's patch?

I see one but I hope I'm not day dreaming.. :D

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

Is there a better way to do this? Can this be converted into a for-loop? Alternatively, would it make sense to have a firsttime local variable initialized to true, to keep the schedule_timeout_interruptible() at the beginning of the loop, but skip it on the first pass through the loop?

Don't get me wrong, what you have looks functionally correct, but duplicating the condition might cause problems later on, for example, should a bug fix be needed in the condition.

I agree with your suggestions and Steven's patch is better.

Quoted 7 lines
Quoted 7 lines
kernel/rcu/update.c +0 -0
index 68fa19a5e7bd..c6df9fa916cf 100644
@@ -796,13 +796,22 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 * holdouts.  When the list is empty, we are done.
 		 */
Quoted 8 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 804

Quoted 8 lines
kernel/rcu/update.c +2 -1
 		lastreport = jiffies;
-		while (!list_empty(&rcu_tasks_holdouts)) {
+		for (;;) {
 			bool firstreport;
 			bool needreport;
 			int rtst;
 			struct task_struct *t1;
+			int fract = 15;

Shouldn't this assignment be done outside the loop? I believe the variable will be initialized on each iteration.

A program like this doesn't terminate:

#include<stdio.h>

int main() {
	for (;;) {
		int i = 10;
		if (!(i--))
			break;
	}

	return 0;
}

Otherwise looks good to me, I would initialize fract to 10 so its consistent with "HZ/10" in other parts of the code but I'm ok with either number.

thanks!

- Joel

#8 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

In reply to Paul E. McKenney
Quoted 49 lines · Paul E. McKenney
Quoted 48 lines · Steven Rostedt
Quoted 44 lines · On Wed, 23 May 2018 10:03:03 -0700 "Paul E. McKenney"…
Quoted 24 lines
Quoted 24 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 747

Quoted 10 lines
kernel/rcu/update.c +2 -0
index 5783bdf86e5a..a28698e44b08 100644
@@ -743,6 +743,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 */
 		synchronize_srcu(&tasks_rcu_exit_srcu);
 
+		/*
+		 * Wait a little bit incase held tasks are released

in case

Quoted 11 lines
kernel/rcu/update.c +4 -0
+		 * during their next timer ticks.
+		 */
+		schedule_timeout_interruptible(HZ/10);
+
 		/*
 		 * Each pass through the following loop scans the list
 		 * of holdout tasks, removing any that are no longer

@@ -755,7 +761,6 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			int rtst;
 			struct task_struct *t1;
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
kernel/rcu/update.c +0 -1
-			schedule_timeout_interruptible(HZ);
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);

@@ -768,6 +773,11 @@ static int __noreturn rcu_tasks_kthread(void *arg)
Quoted 8 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg)

Quoted 8 lines
kernel/rcu/update.c +5 -0
 				check_holdout_task(t, needreport, &firstreport);
 				cond_resched();
 			}
+
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			schedule_timeout_interruptible(HZ);

Why is this a full second wait and not the HZ/10 like the others?

The idea is to respond quickly on small idle systems and to reduce the number of possibly quite lengthy traversals of the task list otherwise. I actually considered exponential backoff, but decided to keep it simple, at least to start with.

Quoted 7 lines
Quoted 3 lines

Ah, now it makes sense. Reading what you wrote, we can still do a backoff and keep it simple. What about the patch below. It appears to have the same performance improvement as Joel's

Looks plausible to me!

Joel, do you see any gotchas in Steve's patch?

I see one but I hope I'm not day dreaming.. :D

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

Is there a better way to do this? Can this be converted into a for-loop? Alternatively, would it make sense to have a firsttime local variable initialized to true, to keep the schedule_timeout_interruptible() at the beginning of the loop, but skip it on the first pass through the loop?

Don't get me wrong, what you have looks functionally correct, but duplicating the condition might cause problems later on, for example, should a bug fix be needed in the condition.

I agree with your suggestions and Steven's patch is better.

Quoted 7 lines
Quoted 7 lines
kernel/rcu/update.c +0 -0
index 68fa19a5e7bd..c6df9fa916cf 100644
@@ -796,13 +796,22 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 * holdouts.  When the list is empty, we are done.
 		 */
Quoted 8 lines

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 804

Quoted 8 lines
kernel/rcu/update.c +2 -1
 		lastreport = jiffies;
-		while (!list_empty(&rcu_tasks_holdouts)) {
+		for (;;) {
 			bool firstreport;
 			bool needreport;
 			int rtst;
 			struct task_struct *t1;
+			int fract = 15;

Shouldn't this assignment be done outside the loop? I believe the variable will be initialized on each iteration.

A program like this doesn't terminate:

#include<stdio.h>

int main() {
	for (;;) {
		int i = 10;
		if (!(i--))
			break;
	}

	return 0;
}

Otherwise looks good to me, I would initialize fract to 10 so its consistent with "HZ/10" in other parts of the code but I'm ok with either number.

thanks!

- Joel

reviewer
#9 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 7 lines · On Wed, 23 May 2018 17:51:19 -0700 Joel Fernandes <[email protected]>…

Shouldn't this assignment be done outside the loop? I believe the variable will be initialized on each iteration.

A program like this doesn't terminate:

#include<stdio.h>

Quoted 9 lines
int main() {
	for (;;) {
		int i = 10;
		if (!(i--))
			break;
	}

	return 0;
}

Hey, it compiled, booted and tested. SHIP IT! ;-)

But yeah, a little egg on my face for that one. I blame it for the first code I wrote right off of coming back from vacation.

Quoted 3 lines

Otherwise looks good to me, I would initialize fract to 10 so its consistent with "HZ/10" in other parts of the code but I'm ok with either number.

I was thinking about that, but for some reason I thought 15. Not sure why. I'm fine with dropping it down to 10. I'll send out a more proper patch tomorrow.

Thanks!

-- Steve

reviewer
#10 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

In reply to Paul E. McKenney
Quoted 2 lines · On Wed, 23 May 2018 13:04:58 -0700 "Paul E. McKenney"…

Looks plausible to me!

BTW looking at the code:

Quoted 10 lines
		/* Invoke the callbacks. */
		while (list) {
			next = list->next;
			local_bh_disable();
			list->func(list);
			local_bh_enable();
			list = next;
			cond_resched();
		}
		schedule_timeout_uninterruptible(HZ/10);

What's the purpose of this final sleep?

-- Steve

Quoted 2 lines
	}
}
#11 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 19 lines · Steven Rostedt
Quoted 2 lines · On Wed, 23 May 2018 13:04:58 -0700 "Paul E. McKenney"…

Looks plausible to me!

BTW looking at the code:

Quoted 10 lines
		/* Invoke the callbacks. */
		while (list) {
			next = list->next;
			local_bh_disable();
			list->func(list);
			local_bh_enable();
			list = next;
			cond_resched();
		}
		schedule_timeout_uninterruptible(HZ/10);
Quoted 1 line

What's the purpose of this final sleep?

To avoid any possibility of rcu_tasks_kthread() getting into a tight loop.

Thanx, Paul

reviewer
#12 lore raw

Re: [PATCH 1/4] rcu: Speed up calling of RCU tasks callbacks

Quoted 20 lines · On Thu, 24 May 2018 15:38:39 -0700 "Paul E. McKenney"…
Quoted 19 lines · Steven Rostedt
Quoted 2 lines · On Wed, 23 May 2018 13:04:58 -0700 "Paul E. McKenney"…

Looks plausible to me!

BTW looking at the code:

Quoted 10 lines
		/* Invoke the callbacks. */
		while (list) {
			next = list->next;
			local_bh_disable();
			list->func(list);
			local_bh_enable();
			list = next;
			cond_resched();
		}
		schedule_timeout_uninterruptible(HZ/10);
Quoted 4 lines
Quoted 1 line

What's the purpose of this final sleep?

To avoid any possibility of rcu_tasks_kthread() getting into a tight loop.

OK, so I'll send out a v4 of what I just sent out ;-)

-- Steve

merge bot
#13 lore raw

[tip:core/rcu] rcu: Add comment to the last sleep in the rcu tasks loop

In reply to Paul E. McKenney
rcu: Add comment to the last sleep in the rcu tasks loop
Commit-ID:  cd23ac8ddb7be993f88bee893b89a8b4971c3651
Gitweb:     https://git.kernel.org/tip/cd23ac8ddb7be993f88bee893b89a8b4971c3651
Author:     Steven Rostedt (VMware) <[email protected]>
AuthorDate: Thu, 24 May 2018 18:58:16 -0400
Committer:  Paul E. McKenney <[email protected]>
CommitDate: Thu, 12 Jul 2018 15:39:21 -0700

rcu: Add comment to the last sleep in the rcu tasks loop

At the end of rcu_tasks_kthread() there's a lonely schedule_timeout_uninterruptible() call with no apparent rationale for its existence. But there is. It is to keep the thread from going into a tight loop if there's some anomaly. That really needs a comment.

Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Steven Rostedt (VMware) <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcu/update.c | 1 +
 1 file changed, 1 insertion(+)
kernel/rcu/update.c +1 -0
index 4c7c49c106ee..39cb23d22109 100644
@@ -814,6 +814,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 			list = next;
 			cond_resched();
 		}
+		/* Paranoid sleep to keep this from entering a tight loop */
 		schedule_timeout_uninterruptible(HZ/10);
 	}
 }
merge bot
#14 lore raw

[tip:core/rcu] rcu: Speed up calling of RCU tasks callbacks

In reply to Joel Fernandes
rcu: Speed up calling of RCU tasks callbacks
Commit-ID:  c03be752d39dc64dcfda0ac8ce87fb10b1ee5621
Gitweb:     https://git.kernel.org/tip/c03be752d39dc64dcfda0ac8ce87fb10b1ee5621
Author:     Steven Rostedt (VMware) <[email protected]>
AuthorDate: Thu, 24 May 2018 18:49:46 -0400
Committer:  Paul E. McKenney <[email protected]>
CommitDate: Thu, 12 Jul 2018 15:39:21 -0700

rcu: Speed up calling of RCU tasks callbacks

Joel Fernandes found that the synchronize_rcu_tasks() was taking a significant amount of time. He demonstrated it with the following test:

 # cd /sys/kernel/tracing
 # while [ 1 ]; do x=1; done &
 # echo '__schedule_bug:traceon' > set_ftrace_filter
 # time echo '!__schedule_bug:traceon' > set_ftrace_filter;

real 0m1.064s
user 0m0.000s
sys 0m0.004s

Where it takes a little over a second to perform the synchronize, because there's a loop that waits 1 second at a time for tasks to get through their quiescent points when there's a task that must be waited for.

After discussion we came up with a simple way to wait for holdouts but increase the time for each iteration of the loop but no more than a full second.

With the new patch we have:

 # time echo '!__schedule_bug:traceon' > set_ftrace_filter;

real 0m0.131s
user 0m0.000s
sys 0m0.004s

Which drops it down to 13% of what the original wait time was.

Link http://lkml.kernel.org/r/[email protected] open here
Reported-by Joel Fernandes (Google) <[email protected]>
Suggested-by Joel Fernandes (Google) <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
---
 kernel/rcu/update.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
kernel/rcu/update.c +15 -2
index 5783bdf86e5a..4c7c49c106ee 100644
@@ -668,6 +668,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 	struct rcu_head *list;
 	struct rcu_head *next;
 	LIST_HEAD(rcu_tasks_holdouts);
+	int fract;
 
 	/* Run on housekeeping CPUs by default.  Sysadm can move if desired. */
 	housekeeping_affine(current, HK_FLAG_RCU);

@@ -749,13 +750,25 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 		 * holdouts.  When the list is empty, we are done.
 		 */
 		lastreport = jiffies;
-		while (!list_empty(&rcu_tasks_holdouts)) {
+
+		/* Start off with HZ/10 wait and slowly back off to 1 HZ wait*/
+		fract = 10;
+
+		for (;;) {
 			bool firstreport;
 			bool needreport;
 			int rtst;
 			struct task_struct *t1;
 
-			schedule_timeout_interruptible(HZ);
+			if (list_empty(&rcu_tasks_holdouts))
+				break;
+
+			/* Slowly back off waiting for holdouts */
+			schedule_timeout_interruptible(HZ/fract);
+
+			if (fract > 1)
+				fract--;
+
 			rtst = READ_ONCE(rcu_task_stall_timeout);
 			needreport = rtst > 0 &&
 				     time_after(jiffies, lastreport + rtst);
author
#15 lore raw

PATCH 2/4 rcu: Add comment documenting how rcu_seq_snap works

In reply to Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

rcu_seq_snap may be tricky to decipher. Lets document how it works with an example to make it easier.

Signed-off-by Joel Fernandes (Google) <[email protected]>
---
 kernel/rcu/rcu.h | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)
kernel/rcu/rcu.h +33 -1
index 0453a7d12b3f..5819a37f19fb 100644
@@ -91,7 +91,39 @@ static inline void rcu_seq_end(unsigned long *sp)
 	WRITE_ONCE(*sp, rcu_seq_endval(sp));
 }
 
-/* Take a snapshot of the update side's sequence number. */
+/*
+ * rcu_seq_snap - Take a snapshot of the update side's sequence number.
+ *
+ * This function returns the earliest value of the grace-period sequence number
+ * that will indicate that a full grace period has elapsed since the current
+ * time.  Once the grace-period sequence number has reached this value, it will
+ * be safe to invoke all callbacks that have been registered prior to the
+ * current time. This value is the current grace-period number plus two to the
+ * power of the number of low-order bits reserved for state, then rounded up to
+ * the next value in which the state bits are all zero.
+ *
+ * In the current design, RCU_SEQ_STATE_MASK=3 and the least significant bit of
+ * the seq is used to track if a GP is in progress or not. Given this, it is
+ * sufficient if we add (6+1) and mask with ~3 to get the next GP. Let's see
+ * why with an example:
+ *
+ * Say the current seq is 12 which is 0b1100 (GP is 3 and state bits are 0b00).
+ * To get to the next GP number of 4, we have to add 0b100 to this (0x1 << 2)
+ * to account for the shift due to 2 state bits. Now, if the current seq is
+ * 13 (GP is 3 and state bits are 0b01), then it means the current grace period
+ * is already in progress so the next GP that a future call back will be queued
+ * to run at is GP+2 = 5, not 4. To account for the extra +1, we just overflow
+ * the 2 lower bits by adding 0b11. In case the lower bit was set, the overflow
+ * will cause the extra +1 to the GP, along with the usual +1 explained before.
+ * This gives us GP+2. Finally we mask the lower to bits by ~0x3 in case the
+ * overflow didn't occur. This masking is needed because in case RCU was idle
+ * (no GP in progress so lower 2 bits are 0b00), then the overflow of the lower
+ * 2 state bits wouldn't occur, so we mask to zero out those lower 2 bits.
+ *
+ * In other words, the next seq can be obtained by (0b11 + 0b100) & (~0b11)
+ * which can be generalized to:
+ * seq + (RCU_SEQ_STATE_MASK + (RCU_SEQ_STATE_MASK + 1)) & (~RCU_SEQ_STATE_MASK)
+ */
 static inline unsigned long rcu_seq_snap(unsigned long *sp)
 {
 	unsigned long s;
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
#16 lore raw

Re: [PATCH 2/4] rcu: Add comment documenting how rcu_seq_snap works

Quoted 6 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

rcu_seq_snap may be tricky to decipher. Lets document how it works with an example to make it easier.

Signed-off-by Joel Fernandes (Google) <[email protected]>

Very good, queued for further review, thank you!

Thanx, Paul

Quoted 52 lines
---
 kernel/rcu/rcu.h | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)
kernel/rcu/rcu.h +33 -1
index 0453a7d12b3f..5819a37f19fb 100644
@@ -91,7 +91,39 @@ static inline void rcu_seq_end(unsigned long *sp)
 	WRITE_ONCE(*sp, rcu_seq_endval(sp));
 }
 
-/* Take a snapshot of the update side's sequence number. */
+/*
+ * rcu_seq_snap - Take a snapshot of the update side's sequence number.
+ *
+ * This function returns the earliest value of the grace-period sequence number
+ * that will indicate that a full grace period has elapsed since the current
+ * time.  Once the grace-period sequence number has reached this value, it will
+ * be safe to invoke all callbacks that have been registered prior to the
+ * current time. This value is the current grace-period number plus two to the
+ * power of the number of low-order bits reserved for state, then rounded up to
+ * the next value in which the state bits are all zero.
+ *
+ * In the current design, RCU_SEQ_STATE_MASK=3 and the least significant bit of
+ * the seq is used to track if a GP is in progress or not. Given this, it is
+ * sufficient if we add (6+1) and mask with ~3 to get the next GP. Let's see
+ * why with an example:
+ *
+ * Say the current seq is 12 which is 0b1100 (GP is 3 and state bits are 0b00).
+ * To get to the next GP number of 4, we have to add 0b100 to this (0x1 << 2)
+ * to account for the shift due to 2 state bits. Now, if the current seq is
+ * 13 (GP is 3 and state bits are 0b01), then it means the current grace period
+ * is already in progress so the next GP that a future call back will be queued
+ * to run at is GP+2 = 5, not 4. To account for the extra +1, we just overflow
+ * the 2 lower bits by adding 0b11. In case the lower bit was set, the overflow
+ * will cause the extra +1 to the GP, along with the usual +1 explained before.
+ * This gives us GP+2. Finally we mask the lower to bits by ~0x3 in case the
+ * overflow didn't occur. This masking is needed because in case RCU was idle
+ * (no GP in progress so lower 2 bits are 0b00), then the overflow of the lower
+ * 2 state bits wouldn't occur, so we mask to zero out those lower 2 bits.
+ *
+ * In other words, the next seq can be obtained by (0b11 + 0b100) & (~0b11)
+ * which can be generalized to:
+ * seq + (RCU_SEQ_STATE_MASK + (RCU_SEQ_STATE_MASK + 1)) & (~RCU_SEQ_STATE_MASK)
+ */
 static inline unsigned long rcu_seq_snap(unsigned long *sp)
 {
 	unsigned long s;
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
author
#17 lore raw

PATCH 3/4 rcu: Use better variable names in funnel locking loop

In reply to Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

The funnel locking loop in rcu_start_this_gp uses rcu_root as a temporary variable while walking the combining tree. This causes a tiresome exercise of a code reader reminding themselves that rcu_root may not be root. Lets just call it rnp, and rename other variables as well to be more appropriate.

Original patch: https://patchwork.kernel.org/patch/10396577/

Signed-off-by Joel Fernandes <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>
---
 kernel/rcu/tree.c | 52 +++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)
kernel/rcu/tree.c +26 -26
index 0ad61c97da69..31f4b4b7d824 100644
@@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
 
 /*
  * rcu_start_this_gp - Request the start of a particular grace period
- * @rnp: The leaf node of the CPU from which to start.
+ * @rnp_start: The leaf node of the CPU from which to start.
  * @rdp: The rcu_data corresponding to the CPU from which to start.
  * @gp_seq_req: The gp_seq of the grace period to start.
  *

@@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
  *
  * Returns true if the GP thread needs to be awakened else false.
  */
-static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
+static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
 			      unsigned long gp_seq_req)
 {
 	bool ret = false;
 	struct rcu_state *rsp = rdp->rsp;
-	struct rcu_node *rnp_root;
+	struct rcu_node *rnp;
 
 	/*
 	 * Use funnel locking to either acquire the root rcu_node

@@ -1556,58 +1556,58 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
 	 * scan the leaf rcu_node structures.  Note that rnp->lock must
 	 * not be released.
 	 */
-	raw_lockdep_assert_held_rcu_node(rnp);
-	trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
-	for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
-		if (rnp_root != rnp)
-			raw_spin_lock_rcu_node(rnp_root);
-		if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
-		    rcu_seq_started(&rnp_root->gp_seq, gp_seq_req) ||
-		    (rnp != rnp_root &&
-		     rcu_seq_state(rcu_seq_current(&rnp_root->gp_seq)))) {
-			trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
+	raw_lockdep_assert_held_rcu_node(rnp_start);
+	trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
+	for (rnp = rnp_start; 1; rnp = rnp->parent) {
+		if (rnp != rnp_start)
+			raw_spin_lock_rcu_node(rnp);
+		if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
+		    rcu_seq_started(&rnp->gp_seq, gp_seq_req) ||
+		    (rnp != rnp_start &&
+		     rcu_seq_state(rcu_seq_current(&rnp->gp_seq)))) {
+			trace_rcu_this_gp(rnp, rdp, gp_seq_req,
 					  TPS("Prestarted"));
 			goto unlock_out;
 		}
-		rnp_root->gp_seq_needed = gp_seq_req;
-		if (rcu_seq_state(rcu_seq_current(&rnp->gp_seq))) {
+		rnp->gp_seq_needed = gp_seq_req;
+		if (rcu_seq_state(rcu_seq_current(&rnp_start->gp_seq))) {
 			/*
 			 * We just marked the leaf, and a grace period
 			 * is in progress, which means that rcu_gp_cleanup()
 			 * will see the marking.  Bail to reduce contention.
 			 */
-			trace_rcu_this_gp(rnp, rdp, gp_seq_req,
+			trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
 					  TPS("Startedleaf"));
 			goto unlock_out;
 		}
-		if (rnp_root != rnp && rnp_root->parent != NULL)
-			raw_spin_unlock_rcu_node(rnp_root);
-		if (!rnp_root->parent)
+		if (rnp != rnp_start && rnp->parent != NULL)
+			raw_spin_unlock_rcu_node(rnp);
+		if (!rnp->parent)
 			break;  /* At root, and perhaps also leaf. */
 	}
 
 	/* If GP already in progress, just leave, otherwise start one. */
 	if (rcu_gp_in_progress(rsp)) {
-		trace_rcu_this_gp(rnp_root, rdp, gp_seq_req, TPS("Startedleafroot"));
+		trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startedleafroot"));
 		goto unlock_out;
 	}
-	trace_rcu_this_gp(rnp_root, rdp, gp_seq_req, TPS("Startedroot"));
+	trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startedroot"));
 	WRITE_ONCE(rsp->gp_flags, rsp->gp_flags | RCU_GP_FLAG_INIT);
 	rsp->gp_req_activity = jiffies;
 	if (!rsp->gp_kthread) {
-		trace_rcu_this_gp(rnp_root, rdp, gp_seq_req, TPS("NoGPkthread"));
+		trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("NoGPkthread"));
 		goto unlock_out;
 	}
 	trace_rcu_grace_period(rsp->name, READ_ONCE(rsp->gp_seq), TPS("newreq"));
 	ret = true;  /* Caller must wake GP kthread. */
 unlock_out:
 	/* Push furthest requested GP to leaf node and rcu_data structure. */
-	if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req)) {
-		rnp->gp_seq_needed = gp_seq_req;
+	if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req)) {
+		rnp_start->gp_seq_needed = gp_seq_req;
 		rdp->gp_seq_needed = gp_seq_req;
 	}
-	if (rnp != rnp_root)
-		raw_spin_unlock_rcu_node(rnp_root);
+	if (rnp != rnp_start)
+		raw_spin_unlock_rcu_node(rnp);
 	return ret;
 }
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
#18 lore raw

Re: [PATCH 3/4] rcu: Use better variable names in funnel locking loop

Quoted 8 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

The funnel locking loop in rcu_start_this_gp uses rcu_root as a temporary variable while walking the combining tree. This causes a tiresome exercise of a code reader reminding themselves that rcu_root may not be root. Lets just call it rnp, and rename other variables as well to be more appropriate.

Quoted 4 lines

Original patch: https://patchwork.kernel.org/patch/10396577/

Signed-off-by Joel Fernandes <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>

I used to have double Signed-off-by back when I was seconded to Linaro. But I am guessing that you want the second and don't need the first one. Unless you tell me otherwise, I will remove the first one on my next rebase.

Anyway, the new variable names are much more clear, good stuff, queued for further review and testing, thank you!

Thanx, Paul

Quoted 117 lines
---
 kernel/rcu/tree.c | 52 +++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 26 deletions(-)
kernel/rcu/tree.c +26 -26
index 0ad61c97da69..31f4b4b7d824 100644
@@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
 
 /*
  * rcu_start_this_gp - Request the start of a particular grace period
- * @rnp: The leaf node of the CPU from which to start.
+ * @rnp_start: The leaf node of the CPU from which to start.
  * @rdp: The rcu_data corresponding to the CPU from which to start.
  * @gp_seq_req: The gp_seq of the grace period to start.
  *

@@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
  *
  * Returns true if the GP thread needs to be awakened else false.
  */
-static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
+static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
 			      unsigned long gp_seq_req)
 {
 	bool ret = false;
 	struct rcu_state *rsp = rdp->rsp;
-	struct rcu_node *rnp_root;
+	struct rcu_node *rnp;
 
 	/*
 	 * Use funnel locking to either acquire the root rcu_node

@@ -1556,58 +1556,58 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
 	 * scan the leaf rcu_node structures.  Note that rnp->lock must
 	 * not be released.
 	 */
-	raw_lockdep_assert_held_rcu_node(rnp);
-	trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
-	for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
-		if (rnp_root != rnp)
-			raw_spin_lock_rcu_node(rnp_root);
-		if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
-		    rcu_seq_started(&rnp_root->gp_seq, gp_seq_req) ||
-		    (rnp != rnp_root &&
-		     rcu_seq_state(rcu_seq_current(&rnp_root->gp_seq)))) {
-			trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
+	raw_lockdep_assert_held_rcu_node(rnp_start);
+	trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
+	for (rnp = rnp_start; 1; rnp = rnp->parent) {
+		if (rnp != rnp_start)
+			raw_spin_lock_rcu_node(rnp);
+		if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
+		    rcu_seq_started(&rnp->gp_seq, gp_seq_req) ||
+		    (rnp != rnp_start &&
+		     rcu_seq_state(rcu_seq_current(&rnp->gp_seq)))) {
+			trace_rcu_this_gp(rnp, rdp, gp_seq_req,
 					  TPS("Prestarted"));
 			goto unlock_out;
 		}
-		rnp_root->gp_seq_needed = gp_seq_req;
-		if (rcu_seq_state(rcu_seq_current(&rnp->gp_seq))) {
+		rnp->gp_seq_needed = gp_seq_req;
+		if (rcu_seq_state(rcu_seq_current(&rnp_start->gp_seq))) {
 			/*
 			 * We just marked the leaf, and a grace period
 			 * is in progress, which means that rcu_gp_cleanup()
 			 * will see the marking.  Bail to reduce contention.
 			 */
-			trace_rcu_this_gp(rnp, rdp, gp_seq_req,
+			trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
 					  TPS("Startedleaf"));
 			goto unlock_out;
 		}
-		if (rnp_root != rnp && rnp_root->parent != NULL)
-			raw_spin_unlock_rcu_node(rnp_root);
-		if (!rnp_root->parent)
+		if (rnp != rnp_start && rnp->parent != NULL)
+			raw_spin_unlock_rcu_node(rnp);
+		if (!rnp->parent)
 			break;  /* At root, and perhaps also leaf. */
 	}
 
 	/* If GP already in progress, just leave, otherwise start one. */
 	if (rcu_gp_in_progress(rsp)) {
-		trace_rcu_this_gp(rnp_root, rdp, gp_seq_req, TPS("Startedleafroot"));
+		trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startedleafroot"));
 		goto unlock_out;
 	}
-	trace_rcu_this_gp(rnp_root, rdp, gp_seq_req, TPS("Startedroot"));
+	trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startedroot"));
 	WRITE_ONCE(rsp->gp_flags, rsp->gp_flags | RCU_GP_FLAG_INIT);
 	rsp->gp_req_activity = jiffies;
 	if (!rsp->gp_kthread) {
-		trace_rcu_this_gp(rnp_root, rdp, gp_seq_req, TPS("NoGPkthread"));
+		trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("NoGPkthread"));
 		goto unlock_out;
 	}
 	trace_rcu_grace_period(rsp->name, READ_ONCE(rsp->gp_seq), TPS("newreq"));
 	ret = true;  /* Caller must wake GP kthread. */
 unlock_out:
 	/* Push furthest requested GP to leaf node and rcu_data structure. */
-	if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req)) {
-		rnp->gp_seq_needed = gp_seq_req;
+	if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req)) {
+		rnp_start->gp_seq_needed = gp_seq_req;
 		rdp->gp_seq_needed = gp_seq_req;
 	}
-	if (rnp != rnp_root)
-		raw_spin_unlock_rcu_node(rnp_root);
+	if (rnp != rnp_start)
+		raw_spin_unlock_rcu_node(rnp);
 	return ret;
 }
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
#19 lore raw

Re: [PATCH 3/4] rcu: Use better variable names in funnel locking loop

Quoted 14 lines · Paul E. McKenney
Quoted 8 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

The funnel locking loop in rcu_start_this_gp uses rcu_root as a temporary variable while walking the combining tree. This causes a tiresome exercise of a code reader reminding themselves that rcu_root may not be root. Lets just call it rnp, and rename other variables as well to be more appropriate.

Quoted 4 lines

Original patch: https://patchwork.kernel.org/patch/10396577/

Signed-off-by Joel Fernandes <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>
Quoted 7 lines

I used to have double Signed-off-by back when I was seconded to Linaro. But I am guessing that you want the second and don't need the first one. Unless you tell me otherwise, I will remove the first one on my next rebase.

Anyway, the new variable names are much more clear, good stuff, queued for further review and testing, thank you!

And it looks to me like I should fold in the patchlet below to change to rnp_start in a comment. Please let me know if this would mess things up.

Thanx, Paul

------------------------------------------------------------------------

commit 94ce05d9d110b8c34eca6641ca5221c1b150e99f

Author: Paul E. McKenney <[email protected]>
Date:   Wed May 23 12:22:01 2018 -0700

fixup! rcu: Use better variable names in funnel locking loop

Signed-off-by Paul E. McKenney <[email protected]>
kernel/rcu/tree.c +5 -5
index 65e49282429c..fdba8ab95e2c 100644
@@ -1550,11 +1550,11 @@ static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
 	/*
 	 * Use funnel locking to either acquire the root rcu_node
 	 * structure's lock or bail out if the need for this grace period
-	 * has already been recorded -- or has already started.  If there
-	 * is already a grace period in progress in a non-leaf node, no
-	 * recording is needed because the end of the grace period will
-	 * scan the leaf rcu_node structures.  Note that rnp->lock must
-	 * not be released.
+	 * has already been recorded -- or if that grace period has in
+	 * fact already started.  If there is already a grace period in
+	 * progress in a non-leaf node, no recording is needed because the
+	 * end of the grace period will scan the leaf rcu_node structures.
+	 * Note that rnp_start->lock must not be released.
 	 */
 	raw_lockdep_assert_held_rcu_node(rnp_start);
 	trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
#20 lore raw

Re: [PATCH 3/4] rcu: Use better variable names in funnel locking loop

Quoted 20 lines · Paul E. McKenney
Quoted 19 lines · Paul E. McKenney
Quoted 8 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

The funnel locking loop in rcu_start_this_gp uses rcu_root as a temporary variable while walking the combining tree. This causes a tiresome exercise of a code reader reminding themselves that rcu_root may not be root. Lets just call it rnp, and rename other variables as well to be more appropriate.

Quoted 4 lines

Original patch: https://patchwork.kernel.org/patch/10396577/

Signed-off-by Joel Fernandes <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>

I used to have double Signed-off-by back when I was seconded to Linaro. But I am guessing that you want the second and don't need the first one. Unless you tell me otherwise, I will remove the first one on my next rebase.

Quoted 5 lines
Quoted 2 lines

Anyway, the new variable names are much more clear, good stuff, queued for further review and testing, thank you!

And it looks to me like I should fold in the patchlet below to change to rnp_start in a comment. Please let me know if this would mess things up.

Yes, missed that. Sorry. It looks great, thanks!

#21 lore raw

Re: [PATCH 3/4] rcu: Use better variable names in funnel locking loop

Quoted 21 lines · Joel Fernandes
Quoted 20 lines · Paul E. McKenney
Quoted 19 lines · Paul E. McKenney
Quoted 8 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

The funnel locking loop in rcu_start_this_gp uses rcu_root as a temporary variable while walking the combining tree. This causes a tiresome exercise of a code reader reminding themselves that rcu_root may not be root. Lets just call it rnp, and rename other variables as well to be more appropriate.

Quoted 4 lines

Original patch: https://patchwork.kernel.org/patch/10396577/

Signed-off-by Joel Fernandes <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>

I used to have double Signed-off-by back when I was seconded to Linaro. But I am guessing that you want the second and don't need the first one. Unless you tell me otherwise, I will remove the first one on my next rebase.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Anyway, the new variable names are much more clear, good stuff, queued for further review and testing, thank you!

And it looks to me like I should fold in the patchlet below to change to rnp_start in a comment. Please let me know if this would mess things up.

Yes, missed that. Sorry. It looks great, thanks!

Done! And not a problem -- this is after all why we do reviews.

Thanx, Paul

author
#22 lore raw

PATCH 4/4 rcu: Identify grace period is in progress as we advance up the tree

In reply to Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

There's no need to keep checking the same starting node for whether a grace period is in progress as we advance up the funnel lock loop. Its sufficient if we just checked it in the start, and then subsequently checked the internal nodes as we advanced up the combining tree. This also makes sense because the grace-period updates propogate from the root to the leaf, so there's a chance we may find a grace period has started as we advance up, lets check for the same.

Reported-by Paul McKenney <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>
---
 kernel/rcu/tree.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
kernel/rcu/tree.c +5 -4
index 31f4b4b7d824..65e49282429c 100644
@@ -1570,11 +1570,12 @@ static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
 			goto unlock_out;
 		}
 		rnp->gp_seq_needed = gp_seq_req;
-		if (rcu_seq_state(rcu_seq_current(&rnp_start->gp_seq))) {
+		if (rcu_seq_state(rcu_seq_current(&rnp->gp_seq))) {
 			/*
-			 * We just marked the leaf, and a grace period
-			 * is in progress, which means that rcu_gp_cleanup()
-			 * will see the marking.  Bail to reduce contention.
+			 * We just marked the leaf or internal node, and a
+			 * grace period is in progress, which means that
+			 * rcu_gp_cleanup() will see the marking.  Bail to
+			 * reduce contention.
 			 */
 			trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
 					  TPS("Startedleaf"));
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
#23 lore raw

Re: [PATCH 4/4] rcu: Identify grace period is in progress as we advance up the tree

Quoted 10 lines · Joel Fernandes

From: "Joel Fernandes (Google)" <[email protected]>

There's no need to keep checking the same starting node for whether a grace period is in progress as we advance up the funnel lock loop. Its sufficient if we just checked it in the start, and then subsequently checked the internal nodes as we advanced up the combining tree. This also makes sense because the grace-period updates propogate from the root to the leaf, so there's a chance we may find a grace period has started as we advance up, lets check for the same.

Quoted 2 lines
Reported-by Paul McKenney <[email protected]>
Signed-off-by Joel Fernandes (Google) <[email protected]>

Looks good, queued for further review and testing, thank you!

Thanx, Paul

Quoted 28 lines
---
 kernel/rcu/tree.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
kernel/rcu/tree.c +5 -4
index 31f4b4b7d824..65e49282429c 100644
@@ -1570,11 +1570,12 @@ static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
 			goto unlock_out;
 		}
 		rnp->gp_seq_needed = gp_seq_req;
-		if (rcu_seq_state(rcu_seq_current(&rnp_start->gp_seq))) {
+		if (rcu_seq_state(rcu_seq_current(&rnp->gp_seq))) {
 			/*
-			 * We just marked the leaf, and a grace period
-			 * is in progress, which means that rcu_gp_cleanup()
-			 * will see the marking.  Bail to reduce contention.
+			 * We just marked the leaf or internal node, and a
+			 * grace period is in progress, which means that
+			 * rcu_gp_cleanup() will see the marking.  Bail to
+			 * reduce contention.
 			 */
 			trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
 					  TPS("Startedleaf"));
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog