Re: [PATCH] xtime_lock vs update_process_times

2008-02-13 Thread Paul Mundt 
On Wed, Feb 13, 2008 at 09:33:16PM +0100, Peter Zijlstra wrote:
> Subject: xtime_lock vs update_process_times
> From: Peter Zijlstra <[EMAIL PROTECTED]>
> 
> ( repost from: http://lkml.org/lkml/2008/1/28/101 )
> 
> Commit: d3d74453c34f8fd87674a8cf5b8a327c68f22e99
> Subject: hrtimer: fixup the HRTIMER_CB_IRQSAFE_NO_SOFTIRQ fallback
> 
> Broke several archs, since only Russel bothered to merge the fix,
> and Greg to ACK his arch, I'm sending this for merger.
> 
> I have confirmation that the Alpha bit results in a booting kernel.
> That leaves: blackfin, frv, sh and sparc untested.
> 
> The deadlock in question was found by Russell:
> 
>   IRQ handle 
> -> timer_tick() - xtime seqlock held for write
>   -> update_process_times() 
> -> run_local_timers()
>   -> hrtimer_run_queues()
> -> hrtimer_get_softirq_time() - tries to get a read lock
> 
> Now, Thomas assures me the fix is trivial, only do_timer() needs to be
> done under the xtime_lock, and update_process_times() can savely be removed
> from under it.
> 
The SH bits also work fine. I've already merged that part in to my tree.
Thanks, Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] xtime_lock vs update_process_times

2008-02-13 Thread Peter Zijlstra 
This time with LKML CC'ed. Sorry for the duplication.


Subject: xtime_lock vs update_process_times
From: Peter Zijlstra <[EMAIL PROTECTED]>

( repost from: http://lkml.org/lkml/2008/1/28/101 )

Commit: d3d74453c34f8fd87674a8cf5b8a327c68f22e99
Subject: hrtimer: fixup the HRTIMER_CB_IRQSAFE_NO_SOFTIRQ fallback

Broke several archs, since only Russel bothered to merge the fix,
and Greg to ACK his arch, I'm sending this for merger.

I have confirmation that the Alpha bit results in a booting kernel.
That leaves: blackfin, frv, sh and sparc untested.

The deadlock in question was found by Russell:

  IRQ handle 
-> timer_tick() - xtime seqlock held for write
  -> update_process_times() 
-> run_local_timers()
  -> hrtimer_run_queues()
-> hrtimer_get_softirq_time() - tries to get a read lock

Now, Thomas assures me the fix is trivial, only do_timer() needs to be
done under the xtime_lock, and update_process_times() can savely be removed
from under it.

Signed-off-by: Peter Zijlstra <[EMAIL PROTECTED]>
Acked-by: Greg Ungerer <[EMAIL PROTECTED]>
CC: [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
---
 arch/alpha/kernel/time.c   |   15 ---
 arch/blackfin/kernel/time.c|8 +---
 arch/frv/kernel/time.c |6 --
 arch/m68knommu/kernel/time.c   |   12 +++-
 arch/sh/kernel/timers/timer-cmt.c  |9 -
 arch/sh/kernel/timers/timer-mtu2.c |2 --
 arch/sparc/kernel/pcic.c   |2 +-
 arch/sparc/kernel/time.c   |7 +++
 8 files changed, 28 insertions(+), 33 deletions(-)

Index: linux-2.6/arch/alpha/kernel/time.c
===
--- linux-2.6.orig/arch/alpha/kernel/time.c
+++ linux-2.6/arch/alpha/kernel/time.c
@@ -119,13 +119,8 @@ irqreturn_t timer_interrupt(int irq, voi
state.partial_tick = delta & ((1UL << FIX_SHIFT) - 1); 
nticks = delta >> FIX_SHIFT;
 
-   while (nticks > 0) {
-   do_timer(1);
-#ifndef CONFIG_SMP
-   update_process_times(user_mode(get_irq_regs()));
-#endif
-   nticks--;
-   }
+   if (nticks)
+   do_timer(nticks);
 
/*
 * If we have an externally synchronized Linux clock, then update
@@ -141,6 +136,12 @@ irqreturn_t timer_interrupt(int irq, voi
}
 
write_sequnlock(_lock);
+
+#ifndef CONFIG_SMP
+   while (nticks--)
+   update_process_times(user_mode(get_irq_regs()));
+#endif
+
return IRQ_HANDLED;
 }
 
Index: linux-2.6/arch/blackfin/kernel/time.c
===
--- linux-2.6.orig/arch/blackfin/kernel/time.c
+++ linux-2.6/arch/blackfin/kernel/time.c
@@ -137,9 +137,6 @@ irqreturn_t timer_interrupt(int irq, voi
 
do_timer(1);
 
-#ifndef CONFIG_SMP
-   update_process_times(user_mode(get_irq_regs()));
-#endif
profile_tick(CPU_PROFILING);
 
/*
@@ -161,6 +158,11 @@ irqreturn_t timer_interrupt(int irq, voi
last_rtc_update = xtime.tv_sec - 600;
}
write_sequnlock(_lock);
+
+#ifndef CONFIG_SMP
+   update_process_times(user_mode(get_irq_regs()));
+#endif
+
return IRQ_HANDLED;
 }
 
Index: linux-2.6/arch/frv/kernel/time.c
===
--- linux-2.6.orig/arch/frv/kernel/time.c
+++ linux-2.6/arch/frv/kernel/time.c
@@ -63,6 +63,7 @@ static irqreturn_t timer_interrupt(int i
/* last time the cmos clock got updated */
static long last_rtc_update = 0;
 
+   profile_tick(CPU_PROFILING);
/*
 * Here we are in the timer irq handler. We just have irqs locally
 * disabled but we don't know if the timer_bh is running on the other
@@ -73,8 +74,6 @@ static irqreturn_t timer_interrupt(int i
write_seqlock(_lock);
 
do_timer(1);
-   update_process_times(user_mode(get_irq_regs()));
-   profile_tick(CPU_PROFILING);
 
/*
 * If we have an externally synchronized Linux clock, then update
@@ -99,6 +98,9 @@ static irqreturn_t timer_interrupt(int i
 #endif /* CONFIG_HEARTBEAT */
 
write_sequnlock(_lock);
+
+   update_process_times(user_mode(get_irq_regs()));
+
return IRQ_HANDLED;
 }
 
Index: linux-2.6/arch/m68knommu/kernel/time.c
===
--- linux-2.6.orig/arch/m68knommu/kernel/time.c
+++ linux-2.6/arch/m68knommu/kernel/time.c
@@ -42,14 +42,12 @@ irqreturn_t arch_timer_interrupt(int irq
/* last time the cmos clock got updated */
static long last_rtc_update=0;
 
+   if (current->pid)
+   profile_tick(CPU_PROFILING);
+
write_seqlock(_lock);
 
do_timer(1);
-#ifndef CONFIG_SMP
-   update_process_times(user_mode(get_irq_regs()));
-#endif
-   if (current->pid)
-

[PATCH] xtime_lock vs update_process_times

2008-02-13 Thread Peter Zijlstra 
This time with LKML CC'ed. Sorry for the duplication.


Subject: xtime_lock vs update_process_times
From: Peter Zijlstra [EMAIL PROTECTED]

( repost from: http://lkml.org/lkml/2008/1/28/101 )

Commit: d3d74453c34f8fd87674a8cf5b8a327c68f22e99
Subject: hrtimer: fixup the HRTIMER_CB_IRQSAFE_NO_SOFTIRQ fallback

Broke several archs, since only Russel bothered to merge the fix,
and Greg to ACK his arch, I'm sending this for merger.

I have confirmation that the Alpha bit results in a booting kernel.
That leaves: blackfin, frv, sh and sparc untested.

The deadlock in question was found by Russell:

  IRQ handle 
- timer_tick() - xtime seqlock held for write
  - update_process_times() 
- run_local_timers()
  - hrtimer_run_queues()
- hrtimer_get_softirq_time() - tries to get a read lock

Now, Thomas assures me the fix is trivial, only do_timer() needs to be
done under the xtime_lock, and update_process_times() can savely be removed
from under it.

Signed-off-by: Peter Zijlstra [EMAIL PROTECTED]
Acked-by: Greg Ungerer [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
CC: [EMAIL PROTECTED]
---
 arch/alpha/kernel/time.c   |   15 ---
 arch/blackfin/kernel/time.c|8 +---
 arch/frv/kernel/time.c |6 --
 arch/m68knommu/kernel/time.c   |   12 +++-
 arch/sh/kernel/timers/timer-cmt.c  |9 -
 arch/sh/kernel/timers/timer-mtu2.c |2 --
 arch/sparc/kernel/pcic.c   |2 +-
 arch/sparc/kernel/time.c   |7 +++
 8 files changed, 28 insertions(+), 33 deletions(-)

Index: linux-2.6/arch/alpha/kernel/time.c
===
--- linux-2.6.orig/arch/alpha/kernel/time.c
+++ linux-2.6/arch/alpha/kernel/time.c
@@ -119,13 +119,8 @@ irqreturn_t timer_interrupt(int irq, voi
state.partial_tick = delta  ((1UL  FIX_SHIFT) - 1); 
nticks = delta  FIX_SHIFT;
 
-   while (nticks  0) {
-   do_timer(1);
-#ifndef CONFIG_SMP
-   update_process_times(user_mode(get_irq_regs()));
-#endif
-   nticks--;
-   }
+   if (nticks)
+   do_timer(nticks);
 
/*
 * If we have an externally synchronized Linux clock, then update
@@ -141,6 +136,12 @@ irqreturn_t timer_interrupt(int irq, voi
}
 
write_sequnlock(xtime_lock);
+
+#ifndef CONFIG_SMP
+   while (nticks--)
+   update_process_times(user_mode(get_irq_regs()));
+#endif
+
return IRQ_HANDLED;
 }
 
Index: linux-2.6/arch/blackfin/kernel/time.c
===
--- linux-2.6.orig/arch/blackfin/kernel/time.c
+++ linux-2.6/arch/blackfin/kernel/time.c
@@ -137,9 +137,6 @@ irqreturn_t timer_interrupt(int irq, voi
 
do_timer(1);
 
-#ifndef CONFIG_SMP
-   update_process_times(user_mode(get_irq_regs()));
-#endif
profile_tick(CPU_PROFILING);
 
/*
@@ -161,6 +158,11 @@ irqreturn_t timer_interrupt(int irq, voi
last_rtc_update = xtime.tv_sec - 600;
}
write_sequnlock(xtime_lock);
+
+#ifndef CONFIG_SMP
+   update_process_times(user_mode(get_irq_regs()));
+#endif
+
return IRQ_HANDLED;
 }
 
Index: linux-2.6/arch/frv/kernel/time.c
===
--- linux-2.6.orig/arch/frv/kernel/time.c
+++ linux-2.6/arch/frv/kernel/time.c
@@ -63,6 +63,7 @@ static irqreturn_t timer_interrupt(int i
/* last time the cmos clock got updated */
static long last_rtc_update = 0;
 
+   profile_tick(CPU_PROFILING);
/*
 * Here we are in the timer irq handler. We just have irqs locally
 * disabled but we don't know if the timer_bh is running on the other
@@ -73,8 +74,6 @@ static irqreturn_t timer_interrupt(int i
write_seqlock(xtime_lock);
 
do_timer(1);
-   update_process_times(user_mode(get_irq_regs()));
-   profile_tick(CPU_PROFILING);
 
/*
 * If we have an externally synchronized Linux clock, then update
@@ -99,6 +98,9 @@ static irqreturn_t timer_interrupt(int i
 #endif /* CONFIG_HEARTBEAT */
 
write_sequnlock(xtime_lock);
+
+   update_process_times(user_mode(get_irq_regs()));
+
return IRQ_HANDLED;
 }
 
Index: linux-2.6/arch/m68knommu/kernel/time.c
===
--- linux-2.6.orig/arch/m68knommu/kernel/time.c
+++ linux-2.6/arch/m68knommu/kernel/time.c
@@ -42,14 +42,12 @@ irqreturn_t arch_timer_interrupt(int irq
/* last time the cmos clock got updated */
static long last_rtc_update=0;
 
+   if (current-pid)
+   profile_tick(CPU_PROFILING);
+
write_seqlock(xtime_lock);
 
do_timer(1);
-#ifndef CONFIG_SMP
-   update_process_times(user_mode(get_irq_regs()));
-#endif
-   if (current-pid)
-

Re: [PATCH] xtime_lock vs update_process_times

2008-02-13 Thread Paul Mundt 
On Wed, Feb 13, 2008 at 09:33:16PM +0100, Peter Zijlstra wrote:
 Subject: xtime_lock vs update_process_times
 From: Peter Zijlstra [EMAIL PROTECTED]
 
 ( repost from: http://lkml.org/lkml/2008/1/28/101 )
 
 Commit: d3d74453c34f8fd87674a8cf5b8a327c68f22e99
 Subject: hrtimer: fixup the HRTIMER_CB_IRQSAFE_NO_SOFTIRQ fallback
 
 Broke several archs, since only Russel bothered to merge the fix,
 and Greg to ACK his arch, I'm sending this for merger.
 
 I have confirmation that the Alpha bit results in a booting kernel.
 That leaves: blackfin, frv, sh and sparc untested.
 
 The deadlock in question was found by Russell:
 
   IRQ handle 
 - timer_tick() - xtime seqlock held for write
   - update_process_times() 
 - run_local_timers()
   - hrtimer_run_queues()
 - hrtimer_get_softirq_time() - tries to get a read lock
 
 Now, Thomas assures me the fix is trivial, only do_timer() needs to be
 done under the xtime_lock, and update_process_times() can savely be removed
 from under it.
 
The SH bits also work fine. I've already merged that part in to my tree.
Thanks, Peter.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/