* Re: periodically-drain-non-local-pagesets-fix.patch added to -mm tree
2005-06-03 6:10 periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
@ 2005-06-03 7:15 ` Keith Owens
2005-06-03 7:22 ` periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Keith Owens @ 2005-06-03 7:15 UTC (permalink / raw)
To: linux-ia64
On Thu, 2 Jun 2005 23:10:57 -0700,
Andrew Morton <akpm@osdl.org> wrote:
>
>(added linux-ia64)
>
>Christoph Lameter <clameter@engr.sgi.com> wrote:
>>
>> printk: 272 messages suppressed.
>> BUG: using smp_processor_id() in preemptible [00000001] code:
>> K10boot.swap/14959
>> caller is ia64_flush_fph+0x40/0x1a0
>>
>> Call Trace:
>> [<a000000100010840>] show_stack+0x80/0xa0
>> spà00023c1571fb20 bspà00023c15719158
>> [<a000000100010890>] dump_stack+0x30/0x60
>> spà00023c1571fcf0 bspà00023c15719148
>> [<a0000001003f3be0>] debug_smp_processor_id+0x2a0/0x2c0
>> spà00023c1571fcf0 bspà00023c15719128
>> [<a00000010002f6c0>] ia64_flush_fph+0x40/0x1a0
>> spà00023c1571fd70 bspà00023c15719110
>> [<a000000100035060>] setup_sigcontext+0x80/0x5e0
>> spà00023c1571fd80 bspà00023c157190c8
>> [<a000000100035aa0>] setup_frame+0x3a0/0x4a0
>> spà00023c1571fd80 bspà00023c15719068
>> [<a000000100035d30>] handle_signal+0x190/0x1a0
>> spà00023c1571fd80 bspà00023c15719030
>> [<a000000100035e80>] ia64_do_signal+0x140/0x400
>> spà00023c1571fd80 bspà00023c15718f88
>> [<a000000100011250>] do_notify_resume_user+0x110/0x120
>
>Seems to me to be a preempt bug in the ia64 code:
>
>inline void
>ia64_flush_fph (struct task_struct *task)
>{
> struct ia64_psr *psr = ia64_psr(ia64_task_regs(task));
>
> if (ia64_is_local_fpu_owner(task) && psr->mfh) {
> psr->mfh = 0;
> task->thread.flags |= IA64_THREAD_FPH_VALID;
> ia64_save_fpu(&task->thread.fph[0]);
> }
>}
>
>ia64_is_local_fpu_owner() diddles around with the concept of "the CPU we're
>running on", but as no locks are held, smp_processor_id() can change at any
>time.
Does this fix the problem? Compiled but not tested.
Index: linux/include/asm-ia64/processor.h
=================================--- linux.orig/include/asm-ia64/processor.h 2005-06-03 13:05:48.421839027 +1000
+++ linux/include/asm-ia64/processor.h 2005-06-03 17:10:43.911784989 +1000
@@ -407,15 +407,18 @@ extern void ia64_setreg_unknown_kr (void
#define ia64_is_local_fpu_owner(t) \
({ \
struct task_struct *__ia64_islfo_task = (t); \
- (__ia64_islfo_task->thread.last_fph_cpu = smp_processor_id() \
+ int ret = (__ia64_islfo_task->thread.last_fph_cpu = get_cpu() \
&& __ia64_islfo_task = (struct task_struct *) ia64_get_kr(IA64_KR_FPU_OWNER)); \
+ put_cpu(); \
+ ret; \
})
/* Mark task T as owning the fph partition of the CPU we're running on. */
#define ia64_set_local_fpu_owner(t) do { \
struct task_struct *__ia64_slfo_task = (t); \
- __ia64_slfo_task->thread.last_fph_cpu = smp_processor_id(); \
+ __ia64_slfo_task->thread.last_fph_cpu = get_cpu(); \
ia64_set_kr(IA64_KR_FPU_OWNER, (unsigned long) __ia64_slfo_task); \
+ put_cpu(); \
} while (0)
/* Mark the fph partition of task T as being invalid on all CPUs. */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: periodically-drain-non-local-pagesets-fix.patch added to -mm
2005-06-03 6:10 periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
2005-06-03 7:15 ` periodically-drain-non-local-pagesets-fix.patch added to -mm tree Keith Owens
@ 2005-06-03 7:22 ` Andrew Morton
2005-06-03 7:31 ` periodically-drain-non-local-pagesets-fix.patch added to -mm tree Keith Owens
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2005-06-03 7:22 UTC (permalink / raw)
To: linux-ia64
Keith Owens <kaos@sgi.com> wrote:
>
> Does this fix the problem? Compiled but not tested.
>
> Index: linux/include/asm-ia64/processor.h
> =================================> --- linux.orig/include/asm-ia64/processor.h 2005-06-03 13:05:48.421839027 +1000
> +++ linux/include/asm-ia64/processor.h 2005-06-03 17:10:43.911784989 +1000
> @@ -407,15 +407,18 @@ extern void ia64_setreg_unknown_kr (void
> #define ia64_is_local_fpu_owner(t) \
> ({ \
> struct task_struct *__ia64_islfo_task = (t); \
> - (__ia64_islfo_task->thread.last_fph_cpu = smp_processor_id() \
> + int ret = (__ia64_islfo_task->thread.last_fph_cpu = get_cpu() \
> && __ia64_islfo_task = (struct task_struct *) ia64_get_kr(IA64_KR_FPU_OWNER)); \
> + put_cpu(); \
> + ret; \
> })
>
> /* Mark task T as owning the fph partition of the CPU we're running on. */
> #define ia64_set_local_fpu_owner(t) do { \
> struct task_struct *__ia64_slfo_task = (t); \
> - __ia64_slfo_task->thread.last_fph_cpu = smp_processor_id(); \
> + __ia64_slfo_task->thread.last_fph_cpu = get_cpu(); \
> ia64_set_kr(IA64_KR_FPU_OWNER, (unsigned long) __ia64_slfo_task); \
> + put_cpu(); \
> } while (0)
>
> /* Mark the fph partition of task T as being invalid on all CPUs. */
Well it'll make the warning go away but I think there's a more fundamental
problem, in that:
get_cpu();
cond = something_which_uses_smp_processor_id();
put_cpu();
/* `cond' can become false at any time from here */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: periodically-drain-non-local-pagesets-fix.patch added to -mm tree
2005-06-03 6:10 periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
2005-06-03 7:15 ` periodically-drain-non-local-pagesets-fix.patch added to -mm tree Keith Owens
2005-06-03 7:22 ` periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
@ 2005-06-03 7:31 ` Keith Owens
2005-06-03 7:35 ` periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
2005-06-03 7:52 ` Christoph Lameter
4 siblings, 0 replies; 6+ messages in thread
From: Keith Owens @ 2005-06-03 7:31 UTC (permalink / raw)
To: linux-ia64
On Fri, 3 Jun 2005 00:22:10 -0700,
Andrew Morton <akpm@osdl.org> wrote:
>Keith Owens <kaos@sgi.com> wrote:
>> Does this fix the problem? Compiled but not tested.
>
>Well it'll make the warning go away but I think there's a more fundamental
>problem, in that:
>
> get_cpu();
> cond = something_which_uses_smp_processor_id();
> put_cpu();
>
> /* `cond' can become false at any time from here */
Good point. The entire fpu load/restore logic on ia64 needs reviewing
for preemption. There is an implicit assumption that the fpu state is
atomically saved and restored on the current cpu, while under the
control of psr->mfh. Preempt has the potential to migrate a task while
it is in the middle of save/restore fpu, breaking that assumption.
Other ia64 register save/restore code might have similar problems.
Leaving this one to the rest of the list.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: periodically-drain-non-local-pagesets-fix.patch added to -mm
2005-06-03 6:10 periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
` (2 preceding siblings ...)
2005-06-03 7:31 ` periodically-drain-non-local-pagesets-fix.patch added to -mm tree Keith Owens
@ 2005-06-03 7:35 ` Andrew Morton
2005-06-03 7:52 ` Christoph Lameter
4 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2005-06-03 7:35 UTC (permalink / raw)
To: linux-ia64
Keith Owens <kaos@sgi.com> wrote:
>
> On Fri, 3 Jun 2005 00:22:10 -0700,
> Andrew Morton <akpm@osdl.org> wrote:
> >Keith Owens <kaos@sgi.com> wrote:
> >> Does this fix the problem? Compiled but not tested.
> >
> >Well it'll make the warning go away but I think there's a more fundamental
> >problem, in that:
> >
> > get_cpu();
> > cond = something_which_uses_smp_processor_id();
> > put_cpu();
> >
> > /* `cond' can become false at any time from here */
>
> Good point. The entire fpu load/restore logic on ia64 needs reviewing
> for preemption. There is an implicit assumption that the fpu state is
> atomically saved and restored on the current cpu, while under the
> control of psr->mfh. Preempt has the potential to migrate a task while
> it is in the middle of save/restore fpu, breaking that assumption.
> Other ia64 register save/restore code might have similar problems.
OK, thanks. Could someone please spin up a disable-preempt-on-ia64 patch
for the 2.6.12 release?
> Leaving this one to the rest of the list.
Sensible man ;)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: periodically-drain-non-local-pagesets-fix.patch added to -mm
2005-06-03 6:10 periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
` (3 preceding siblings ...)
2005-06-03 7:35 ` periodically-drain-non-local-pagesets-fix.patch added to -mm Andrew Morton
@ 2005-06-03 7:52 ` Christoph Lameter
4 siblings, 0 replies; 6+ messages in thread
From: Christoph Lameter @ 2005-06-03 7:52 UTC (permalink / raw)
To: linux-ia64
On Fri, 3 Jun 2005, Andrew Morton wrote:
> OK, thanks. Could someone please spin up a disable-preempt-on-ia64 patch
> for the 2.6.12 release?
Ok. But .... I hope that we all agree on this?
Index: linux-2.6.12-rc5/arch/ia64/Kconfig
=================================--- linux-2.6.12-rc5.orig/arch/ia64/Kconfig 2005-06-02 21:59:26.000000000 -0700
+++ linux-2.6.12-rc5/arch/ia64/Kconfig 2005-06-03 00:49:01.000000000 -0700
@@ -285,7 +285,7 @@ config SCHED_SMT
overhead in some places. If unsure say N here.
config PREEMPT
- bool "Preemptible Kernel"
+ default n
help
This option reduces the latency of the kernel when reacting to
real-time or interactive events by allowing a low priority process to
^ permalink raw reply [flat|nested] 6+ messages in thread