Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1737576 > unrolled thread

[PATCH 1/3] srcu: use cpu_online() instead custom check

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] srcu: use cpu_online() instead custom check Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-22 17:30 +0200
    [PATCH 3/3] rcu/segcblist: include rcupdate.h Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-22 17:30 +0200
      Re: [PATCH 3/3] rcu/segcblist: include rcupdate.h "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-22 20:50 +0200
    [PATCH 2/3] srcu: queue work without holding the lock Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-22 17:30 +0200
      Re: [PATCH 2/3] srcu: queue work without holding the lock "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-22 20:50 +0200
    Re: [PATCH 1/3] srcu: use cpu_online() instead custom check "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-09-22 20:50 +0200

#1737576 — [PATCH 1/3] srcu: use cpu_online() instead custom check

The current check via srcu_online is slightly racy because after looking
at srcu_online there could be an interrupt that interrupted us long
enough until the CPU we checked against went offline.
An alternative would be to hold the hotplug rwsem (so the CPUs don't
change their state) and then check based on cpu_online() if we queue it
on a specific CPU or not. queue_work_on() itself can handle if something
is enqueued on an offline CPU but a timer which is enqueued on an offline
CPU won't fire until the CPU is back online.

I am not sure if the removal in rcu_init() is okay or not. I assume that
SRCU won't enqueue a work item before SRCU is up and ready.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/rcu/srcutree.c | 22 ++++------------------
 kernel/rcu/tree.c     |  6 ------
 2 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 729a8706751d..d190af0e56f8 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -36,6 +36,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/srcu.h>
+#include <linux/cpu.h>
 
 #include "rcu.h"
 #include "rcu_segcblist.h"
@@ -424,21 +425,6 @@ static void srcu_gp_start(struct srcu_struct *sp)
 	WARN_ON_ONCE(state != SRCU_STATE_SCAN1);
 }
 
-/*
- * Track online CPUs to guide callback workqueue placement.
- */
-DEFINE_PER_CPU(bool, srcu_online);
-
-void srcu_online_cpu(unsigned int cpu)
-{
-	WRITE_ONCE(per_cpu(srcu_online, cpu), true);
-}
-
-void srcu_offline_cpu(unsigned int cpu)
-{
-	WRITE_ONCE(per_cpu(srcu_online, cpu), false);
-}
-
 /*
  * Place the workqueue handler on the specified CPU if online, otherwise
  * just run it whereever.  This is useful for placing workqueue handlers
@@ -450,12 +436,12 @@ static bool srcu_queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
 {
 	bool ret;
 
-	preempt_disable();
-	if (READ_ONCE(per_cpu(srcu_online, cpu)))
+	cpus_read_lock();
+	if (cpu_online(cpu))
 		ret = queue_delayed_work_on(cpu, wq, dwork, delay);
 	else
 		ret = queue_delayed_work(wq, dwork, delay);
-	preempt_enable();
+	cpus_read_unlock();
 	return ret;
 }
 
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1250e4bd4b85..a3cb562955c9 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3729,8 +3729,6 @@ int rcutree_online_cpu(unsigned int cpu)
 {
 	sync_sched_exp_online_cleanup(cpu);
 	rcutree_affinity_setting(cpu, -1);
-	if (IS_ENABLED(CONFIG_TREE_SRCU))
-		srcu_online_cpu(cpu);
 	return 0;
 }
 
@@ -3741,8 +3739,6 @@ int rcutree_online_cpu(unsigned int cpu)
 int rcutree_offline_cpu(unsigned int cpu)
 {
 	rcutree_affinity_setting(cpu, cpu);
-	if (IS_ENABLED(CONFIG_TREE_SRCU))
-		srcu_offline_cpu(cpu);
 	return 0;
 }
 
@@ -4188,8 +4184,6 @@ void __init rcu_init(void)
 	for_each_online_cpu(cpu) {
 		rcutree_prepare_cpu(cpu);
 		rcu_cpu_starting(cpu);
-		if (IS_ENABLED(CONFIG_TREE_SRCU))
-			srcu_online_cpu(cpu);
 	}
 }
 
-- 
2.14.1

[toc] | [next] | [standalone]

#1737579 — [PATCH 3/3] rcu/segcblist: include rcupdate.h

The RT build on ARM complains about non-existing ULONG_CMP_LT.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/rcu/rcu_segcblist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
index 7649fcd2c4c7..88cba7c2956c 100644
--- a/kernel/rcu/rcu_segcblist.c
+++ b/kernel/rcu/rcu_segcblist.c
@@ -23,6 +23,7 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
+#include <linux/rcupdate.h>
 
 #include "rcu_segcblist.h"
 
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1737759 — Re: [PATCH 3/3] rcu/segcblist: include rcupdate.h

On Fri, Sep 22, 2017 at 05:28:06PM +0200, Sebastian Andrzej Siewior wrote:
> The RT build on ARM complains about non-existing ULONG_CMP_LT.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I have queued this one for review and testing, thank you!

							Thanx, Paul

> ---
>  kernel/rcu/rcu_segcblist.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
> index 7649fcd2c4c7..88cba7c2956c 100644
> --- a/kernel/rcu/rcu_segcblist.c
> +++ b/kernel/rcu/rcu_segcblist.c
> @@ -23,6 +23,7 @@
>  #include <linux/types.h>
>  #include <linux/kernel.h>
>  #include <linux/interrupt.h>
> +#include <linux/rcupdate.h>
>  
>  #include "rcu_segcblist.h"
>  
> -- 
> 2.14.1
> 

[toc] | [prev] | [next] | [standalone]

#1737580 — [PATCH 2/3] srcu: queue work without holding the lock

On RT we can't invoke queue_delayed_work() within an atomic section
(which is provided by raw_spin_lock_irqsave()).
srcu_reschedule() invokes queue_delayed_work() outside of the
raw_spin_lock_irq_rcu_node() section so this should be fine here, too.
If the remaining callers of call_srcu() aren't atomic
(spin_lock_irqsave() is fine) then this should work on RT, too.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/rcu/srcutree.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index d190af0e56f8..3ee4ef40f23e 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -648,12 +648,17 @@ static void srcu_funnel_gp_start(struct srcu_struct *sp, struct srcu_data *sdp,
 	/* If grace period not already done and none in progress, start it. */
 	if (!rcu_seq_done(&sp->srcu_gp_seq, s) &&
 	    rcu_seq_state(sp->srcu_gp_seq) == SRCU_STATE_IDLE) {
+		unsigned long delay;
+
 		WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
 		srcu_gp_start(sp);
+		delay = srcu_get_delay(sp);
+		raw_spin_unlock_irqrestore_rcu_node(sp, flags);
+
 		queue_delayed_work(system_power_efficient_wq, &sp->work,
-				   srcu_get_delay(sp));
-	}
-	raw_spin_unlock_irqrestore_rcu_node(sp, flags);
+				   delay);
+	} else
+		raw_spin_unlock_irqrestore_rcu_node(sp, flags);
 }
 
 /*
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1737750 — Re: [PATCH 2/3] srcu: queue work without holding the lock

On Fri, Sep 22, 2017 at 05:28:05PM +0200, Sebastian Andrzej Siewior wrote:
> On RT we can't invoke queue_delayed_work() within an atomic section
> (which is provided by raw_spin_lock_irqsave()).
> srcu_reschedule() invokes queue_delayed_work() outside of the
> raw_spin_lock_irq_rcu_node() section so this should be fine here, too.
> If the remaining callers of call_srcu() aren't atomic
> (spin_lock_irqsave() is fine) then this should work on RT, too.

Just to make sure I understand...   The problem is not the _irqsave,
but rather the raw_?

							Thanx, Paul

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/rcu/srcutree.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index d190af0e56f8..3ee4ef40f23e 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -648,12 +648,17 @@ static void srcu_funnel_gp_start(struct srcu_struct *sp, struct srcu_data *sdp,
>  	/* If grace period not already done and none in progress, start it. */
>  	if (!rcu_seq_done(&sp->srcu_gp_seq, s) &&
>  	    rcu_seq_state(sp->srcu_gp_seq) == SRCU_STATE_IDLE) {
> +		unsigned long delay;
> +
>  		WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
>  		srcu_gp_start(sp);
> +		delay = srcu_get_delay(sp);
> +		raw_spin_unlock_irqrestore_rcu_node(sp, flags);
> +
>  		queue_delayed_work(system_power_efficient_wq, &sp->work,
> -				   srcu_get_delay(sp));
> -	}
> -	raw_spin_unlock_irqrestore_rcu_node(sp, flags);
> +				   delay);
> +	} else
> +		raw_spin_unlock_irqrestore_rcu_node(sp, flags);
>  }
>  
>  /*
> -- 
> 2.14.1
> 

[toc] | [prev] | [next] | [standalone]

#1737762

On Fri, Sep 22, 2017 at 05:28:04PM +0200, Sebastian Andrzej Siewior wrote:
> The current check via srcu_online is slightly racy because after looking
> at srcu_online there could be an interrupt that interrupted us long
> enough until the CPU we checked against went offline.

But in that case, wouldn't the interrupt block the synchronize_sched()
later in the offline sequence?

More to the point, are you actually seeing this failure, or is this
a theoretical bug?

> An alternative would be to hold the hotplug rwsem (so the CPUs don't
> change their state) and then check based on cpu_online() if we queue it
> on a specific CPU or not. queue_work_on() itself can handle if something
> is enqueued on an offline CPU but a timer which is enqueued on an offline
> CPU won't fire until the CPU is back online.
> 
> I am not sure if the removal in rcu_init() is okay or not. I assume that
> SRCU won't enqueue a work item before SRCU is up and ready.

Another alternative would be to disable preemption across the check and
the call to queue_delayed_work_on().

Yet another alternative would be to have an SRCU-specific per-CPU lock
that is acquired across the setting and clearing of srcu_online,
and also across the check and the call to queue_delayed_work_on().
This last would be more consistent with a desire to remove the
synchronize_sched() from the offline sequence.

Or am I missing something here?

							Thanx, Paul

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/rcu/srcutree.c | 22 ++++------------------
>  kernel/rcu/tree.c     |  6 ------
>  2 files changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 729a8706751d..d190af0e56f8 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -36,6 +36,7 @@
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/srcu.h>
> +#include <linux/cpu.h>
>  
>  #include "rcu.h"
>  #include "rcu_segcblist.h"
> @@ -424,21 +425,6 @@ static void srcu_gp_start(struct srcu_struct *sp)
>  	WARN_ON_ONCE(state != SRCU_STATE_SCAN1);
>  }
>  
> -/*
> - * Track online CPUs to guide callback workqueue placement.
> - */
> -DEFINE_PER_CPU(bool, srcu_online);
> -
> -void srcu_online_cpu(unsigned int cpu)
> -{
> -	WRITE_ONCE(per_cpu(srcu_online, cpu), true);
> -}
> -
> -void srcu_offline_cpu(unsigned int cpu)
> -{
> -	WRITE_ONCE(per_cpu(srcu_online, cpu), false);
> -}
> -
>  /*
>   * Place the workqueue handler on the specified CPU if online, otherwise
>   * just run it whereever.  This is useful for placing workqueue handlers
> @@ -450,12 +436,12 @@ static bool srcu_queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
>  {
>  	bool ret;
>  
> -	preempt_disable();
> -	if (READ_ONCE(per_cpu(srcu_online, cpu)))
> +	cpus_read_lock();
> +	if (cpu_online(cpu))
>  		ret = queue_delayed_work_on(cpu, wq, dwork, delay);
>  	else
>  		ret = queue_delayed_work(wq, dwork, delay);
> -	preempt_enable();
> +	cpus_read_unlock();
>  	return ret;
>  }
>  
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 1250e4bd4b85..a3cb562955c9 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3729,8 +3729,6 @@ int rcutree_online_cpu(unsigned int cpu)
>  {
>  	sync_sched_exp_online_cleanup(cpu);
>  	rcutree_affinity_setting(cpu, -1);
> -	if (IS_ENABLED(CONFIG_TREE_SRCU))
> -		srcu_online_cpu(cpu);
>  	return 0;
>  }
>  
> @@ -3741,8 +3739,6 @@ int rcutree_online_cpu(unsigned int cpu)
>  int rcutree_offline_cpu(unsigned int cpu)
>  {
>  	rcutree_affinity_setting(cpu, cpu);
> -	if (IS_ENABLED(CONFIG_TREE_SRCU))
> -		srcu_offline_cpu(cpu);
>  	return 0;
>  }
>  
> @@ -4188,8 +4184,6 @@ void __init rcu_init(void)
>  	for_each_online_cpu(cpu) {
>  		rcutree_prepare_cpu(cpu);
>  		rcu_cpu_starting(cpu);
> -		if (IS_ENABLED(CONFIG_TREE_SRCU))
> -			srcu_online_cpu(cpu);
>  	}
>  }
>  
> -- 
> 2.14.1
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web