LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Fix hibernate/resume in presence of PREEMPT_RCU and hotplug
@ 2008-02-28  0:21 Paul E. McKenney
  2008-02-28  5:05 ` Gautham R Shenoy
  2008-02-28 19:51 ` [PATCH] Remove never-migrates assumption from rcu_process_callbacks() Paul E. McKenney
  0 siblings, 2 replies; 7+ messages in thread
From: Paul E. McKenney @ 2008-02-28  0:21 UTC (permalink / raw)
  To: linux-kernel
  Cc: hidave.darkstar, akpm, fzu, mingo, ego, dipankar, niv, dvhltc

Hello!

This fixes a oops encountered when doing hibernate/resume in presence
of PREEMPT_RCU.  The problem was that the code failed to disable preemption
when accessing a per-CPU variable.  This is OK when called from code
that already has preemption disabled, but such is not the case from
the suspend/resume code path.

Reported-by: Dave Young <hidave.darkstar@gmail.com>
Tested-by: Dave Young <hidave.darkstar@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 rcupreempt.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -urpNa -X dontdiff linux-2.6.25-rc3/kernel/rcupreempt.c linux-2.6.25-rc3-rcuoffline/kernel/rcupreempt.c
--- linux-2.6.25-rc3/kernel/rcupreempt.c	2008-02-26 16:58:43.000000000 -0800
+++ linux-2.6.25-rc3-rcuoffline/kernel/rcupreempt.c	2008-02-26 17:04:17.000000000 -0800
@@ -702,8 +702,9 @@ void rcu_offline_cpu(int cpu)
 	 * fix.
 	 */
 
+	local_irq_save(flags);
 	rdp = RCU_DATA_ME();
-	spin_lock_irqsave(&rdp->lock, flags);
+	spin_lock(&rdp->lock);
 	*rdp->nexttail = list;
 	if (list)
 		rdp->nexttail = tail;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Fix hibernate/resume in presence of PREEMPT_RCU and hotplug
  2008-02-28  0:21 [PATCH] Fix hibernate/resume in presence of PREEMPT_RCU and hotplug Paul E. McKenney
@ 2008-02-28  5:05 ` Gautham R Shenoy
  2008-02-28 19:43   ` Paul E. McKenney
  2008-02-28 19:51 ` [PATCH] Remove never-migrates assumption from rcu_process_callbacks() Paul E. McKenney
  1 sibling, 1 reply; 7+ messages in thread
From: Gautham R Shenoy @ 2008-02-28  5:05 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, hidave.darkstar, akpm, fzu, mingo, dipankar, niv, dvhltc

On Wed, Feb 27, 2008 at 04:21:10PM -0800, Paul E. McKenney wrote:
> Hello!
> 
> This fixes a oops encountered when doing hibernate/resume in presence
> of PREEMPT_RCU.  The problem was that the code failed to disable preemption
> when accessing a per-CPU variable.  This is OK when called from code
> that already has preemption disabled, but such is not the case from
> the suspend/resume code path.

Well, rcu_offline_cpu() is called from the cpu-offline callback path, and
AFAICS, it doesn't disable preemption.

I wonder how we're seeing this only now! Indeed a good catch!

Reviewed-by: Gautham R Shenoy <ego@in.ibm.com>

> 
> Reported-by: Dave Young <hidave.darkstar@gmail.com>
> Tested-by: Dave Young <hidave.darkstar@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
> 
>  rcupreempt.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff -urpNa -X dontdiff linux-2.6.25-rc3/kernel/rcupreempt.c linux-2.6.25-rc3-rcuoffline/kernel/rcupreempt.c
> --- linux-2.6.25-rc3/kernel/rcupreempt.c	2008-02-26 16:58:43.000000000 -0800
> +++ linux-2.6.25-rc3-rcuoffline/kernel/rcupreempt.c	2008-02-26 17:04:17.000000000 -0800
> @@ -702,8 +702,9 @@ void rcu_offline_cpu(int cpu)
>  	 * fix.
>  	 */
> 
> +	local_irq_save(flags);
>  	rdp = RCU_DATA_ME();
> -	spin_lock_irqsave(&rdp->lock, flags);
> +	spin_lock(&rdp->lock);
>  	*rdp->nexttail = list;
>  	if (list)
>  		rdp->nexttail = tail;

-- 
Thanks and Regards
gautham

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Fix hibernate/resume in presence of PREEMPT_RCU and hotplug
  2008-02-28  5:05 ` Gautham R Shenoy
@ 2008-02-28 19:43   ` Paul E. McKenney
  0 siblings, 0 replies; 7+ messages in thread
From: Paul E. McKenney @ 2008-02-28 19:43 UTC (permalink / raw)
  To: Gautham R Shenoy
  Cc: linux-kernel, hidave.darkstar, akpm, fzu, mingo, dipankar, niv, dvhltc

On Thu, Feb 28, 2008 at 10:35:08AM +0530, Gautham R Shenoy wrote:
> On Wed, Feb 27, 2008 at 04:21:10PM -0800, Paul E. McKenney wrote:
> > Hello!
> > 
> > This fixes a oops encountered when doing hibernate/resume in presence
> > of PREEMPT_RCU.  The problem was that the code failed to disable preemption
> > when accessing a per-CPU variable.  This is OK when called from code
> > that already has preemption disabled, but such is not the case from
> > the suspend/resume code path.
> 
> Well, rcu_offline_cpu() is called from the cpu-offline callback path, and
> AFAICS, it doesn't disable preemption.
> 
> I wonder how we're seeing this only now! Indeed a good catch!

I wonder as well.  In fact there is (at least) one other such bug,
submitting patch separately.  :-/

							Thanx, Paul

> Reviewed-by: Gautham R Shenoy <ego@in.ibm.com>
> 
> > 
> > Reported-by: Dave Young <hidave.darkstar@gmail.com>
> > Tested-by: Dave Young <hidave.darkstar@gmail.com>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> > 
> >  rcupreempt.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff -urpNa -X dontdiff linux-2.6.25-rc3/kernel/rcupreempt.c linux-2.6.25-rc3-rcuoffline/kernel/rcupreempt.c
> > --- linux-2.6.25-rc3/kernel/rcupreempt.c	2008-02-26 16:58:43.000000000 -0800
> > +++ linux-2.6.25-rc3-rcuoffline/kernel/rcupreempt.c	2008-02-26 17:04:17.000000000 -0800
> > @@ -702,8 +702,9 @@ void rcu_offline_cpu(int cpu)
> >  	 * fix.
> >  	 */
> > 
> > +	local_irq_save(flags);
> >  	rdp = RCU_DATA_ME();
> > -	spin_lock_irqsave(&rdp->lock, flags);
> > +	spin_lock(&rdp->lock);
> >  	*rdp->nexttail = list;
> >  	if (list)
> >  		rdp->nexttail = tail;
> 
> -- 
> Thanks and Regards
> gautham

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] Remove never-migrates assumption from rcu_process_callbacks()
  2008-02-28  0:21 [PATCH] Fix hibernate/resume in presence of PREEMPT_RCU and hotplug Paul E. McKenney
  2008-02-28  5:05 ` Gautham R Shenoy
@ 2008-02-28 19:51 ` Paul E. McKenney
  2008-02-28 19:53   ` Ingo Molnar
  1 sibling, 1 reply; 7+ messages in thread
From: Paul E. McKenney @ 2008-02-28 19:51 UTC (permalink / raw)
  To: linux-kernel
  Cc: hidave.darkstar, akpm, fzu, mingo, ego, dipankar, niv, dvhltc

Hello again!

This patch fixes a potentially invalid access to a per-CPU variable in
rcu_process_callbacks().  This per-CPU access needs to be done in such
a way as to guarantee that the code using it cannot move to some other
CPU before all uses of the value accessed have completed.  Even though
this code is currently only invoked from softirq context, which currrently
cannot migrate to some other CPU, life would be better if this
code did not silently make such an assumption.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 rcupreempt.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff -urpNa -X dontdiff linux-2.6.25-rc3-rcuoffline/kernel/rcupreempt.c linux-2.6.25-rc3-rcuoffline-fp/kernel/rcupreempt.c
--- linux-2.6.25-rc3-rcuoffline/kernel/rcupreempt.c	2008-02-26 17:04:17.000000000 -0800
+++ linux-2.6.25-rc3-rcuoffline-fp/kernel/rcupreempt.c	2008-02-27 23:04:18.000000000 -0800
@@ -736,9 +736,11 @@ static void rcu_process_callbacks(struct
 {
 	unsigned long flags;
 	struct rcu_head *next, *list;
-	struct rcu_data *rdp = RCU_DATA_ME();
+	struct rcu_data *rdp;
 
-	spin_lock_irqsave(&rdp->lock, flags);
+	local_irq_save(flags);
+	rdp = RCU_DATA_ME();
+	spin_lock(&rdp->lock);
 	list = rdp->donelist;
 	if (list == NULL) {
 		spin_unlock_irqrestore(&rdp->lock, flags);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove never-migrates assumption from rcu_process_callbacks()
  2008-02-28 19:51 ` [PATCH] Remove never-migrates assumption from rcu_process_callbacks() Paul E. McKenney
@ 2008-02-28 19:53   ` Ingo Molnar
  2008-02-28 19:54     ` Ingo Molnar
  0 siblings, 1 reply; 7+ messages in thread
From: Ingo Molnar @ 2008-02-28 19:53 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, hidave.darkstar, akpm, fzu, ego, dipankar, niv, dvhltc


* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> Hello again!
> 
> This patch fixes a potentially invalid access to a per-CPU variable in 
> rcu_process_callbacks().  This per-CPU access needs to be done in such 
> a way as to guarantee that the code using it cannot move to some other 
> CPU before all uses of the value accessed have completed.  Even though 
> this code is currently only invoked from softirq context, which 
> currrently cannot migrate to some other CPU, life would be better if 
> this code did not silently make such an assumption.

i've got the patch below queued up already - same thing, right?

	Ingo

----------->
Subject: rcupreempt: fix hibernate/resume in presence of PREEMPT_RCU and hotplug
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Wed, 27 Feb 2008 16:21:10 -0800

This fixes a oops encountered when doing hibernate/resume in presence
of PREEMPT_RCU.  The problem was that the code failed to disable preemption
when accessing a per-CPU variable.  This is OK when called from code
that already has preemption disabled, but such is not the case from
the suspend/resume code path.

Reported-by: Dave Young <hidave.darkstar@gmail.com>
Tested-by: Dave Young <hidave.darkstar@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/rcupreempt.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux/kernel/rcupreempt.c
===================================================================
--- linux.orig/kernel/rcupreempt.c
+++ linux/kernel/rcupreempt.c
@@ -918,8 +918,9 @@ void rcu_offline_cpu(int cpu)
 	 * fix.
 	 */
 
+	local_irq_save(flags);
 	rdp = RCU_DATA_ME();
-	spin_lock_irqsave(&rdp->lock, flags);
+	spin_lock(&rdp->lock);
 	*rdp->nexttail = list;
 	if (list)
 		rdp->nexttail = tail;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove never-migrates assumption from rcu_process_callbacks()
  2008-02-28 19:53   ` Ingo Molnar
@ 2008-02-28 19:54     ` Ingo Molnar
  2008-02-28 23:10       ` Paul E. McKenney
  0 siblings, 1 reply; 7+ messages in thread
From: Ingo Molnar @ 2008-02-28 19:54 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, hidave.darkstar, akpm, fzu, ego, dipankar, niv, dvhltc


* Ingo Molnar <mingo@elte.hu> wrote:

> i've got the patch below queued up already - same thing, right?

oh, not the same thing - just looking very similar.

queued up the second patch too :-)

	Ingo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Remove never-migrates assumption from rcu_process_callbacks()
  2008-02-28 19:54     ` Ingo Molnar
@ 2008-02-28 23:10       ` Paul E. McKenney
  0 siblings, 0 replies; 7+ messages in thread
From: Paul E. McKenney @ 2008-02-28 23:10 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel, hidave.darkstar, akpm, fzu, ego, dipankar, niv, dvhltc

On Thu, Feb 28, 2008 at 08:54:54PM +0100, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > i've got the patch below queued up already - same thing, right?
> 
> oh, not the same thing - just looking very similar.
> 
> queued up the second patch too :-)

Thank you, Ingo!

/me goes back to analyzing the rcu-preempt code...

						Thanx, Paul

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2008-02-28 23:33 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-28  0:21 [PATCH] Fix hibernate/resume in presence of PREEMPT_RCU and hotplug Paul E. McKenney
2008-02-28  5:05 ` Gautham R Shenoy
2008-02-28 19:43   ` Paul E. McKenney
2008-02-28 19:51 ` [PATCH] Remove never-migrates assumption from rcu_process_callbacks() Paul E. McKenney
2008-02-28 19:53   ` Ingo Molnar
2008-02-28 19:54     ` Ingo Molnar
2008-02-28 23:10       ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).