Prev: sound: push BKL into open functions
Next: edd: fix possible memory leak in edd_init() error path
From: Milton Miller on 11 Jul 2010 04:00

On Fri, 09 Jul 2010 about 08:55:01 -0000, Will Schmidt wrote:
> We've been seeing some issues with userspace randomly SIGSEGV'ing while
> running the -RT kernels on POWER7 based systems. After lots of
> debugging, head scratching, and experimental changes to the code, the
> problem has been narrowed down such that we can avoid the problems by
> disabling the TLB batching.
>
> After some input from Ben and further debug, we've found that the
> restoration of the batch->active value near the end of __switch_to()
> seems to be the key. ( The -RT related changes within
> arch/powerpc/kernel/processor.c __switch_to() do the equivalent of a
> arch_leave_lazy_mmu_mode() before calling _switch, use a hadbatch flag
> to indicate if batching was active, and then restore that batch->active
> value on the way out after the call to _switch_to. That particular
> code is in the -RT branch, and not found in mainline )
>
> Deferring to Ben (or others in the know) for whether this is the proper
> solution or if there is something deeper, but..


I looked at the patch and noticed 2 changes:
1) the batch is checked and cleared after local_irq_save
2) enabling the batch is skipped

I talked to Will and had him try moving the local_irq_save above the
check for the active batch. That alone did not seem to be enough.
However, he confirmed that we are setting batch to active when it is
already active in lazy_mmu_enter, meaning that batching is being turned
on recursively. I suggested debug to check that irqs are off after the
restore when re-enabling when our debug session timed out.

milton

>
> diff -aurp linux-2.6.33.5-rt23.orig/arch/powerpc/kernel/process.c linux-2.6.33.5-rt23.exp/arch/powerpc/kernel/process.c
> --- linux-2.6.33.5-rt23.orig/arch/powerpc/kernel/process.c 2010-06-21 11:41:34.402513904 -0500
> +++ linux-2.6.33.5-rt23.exp/arch/powerpc/kernel/process.c 2010-07-09 13:15:13.533269904 -0500
> @@ -304,10 +304,6 @@ struct task_struct *__switch_to(struct t
> struct thread_struct *new_thread, *old_thread;
> unsigned long flags;
> struct task_struct *last;
> -#if defined(CONFIG_PPC64) && defined (CONFIG_PREEMPT_RT)
> - struct ppc64_tlb_batch *batch;
> - int hadbatch;
> -#endif
>
> #ifdef CONFIG_SMP
> /* avoid complexity of lazy save/restore of fpu
> @@ -401,16 +397,6 @@ struct task_struct *__switch_to(struct t
> new_thread->start_tb = current_tb;
> }
>
> -#ifdef CONFIG_PREEMPT_RT
> - batch = &__get_cpu_var(ppc64_tlb_batch);
> - if (batch->active) {
> - hadbatch = 1;
> - if (batch->index) {
> - __flush_tlb_pending(batch);
> - }
> - batch->active = 0;
> - }
> -#endif /* #ifdef CONFIG_PREEMPT_RT */
> #endif
>
> local_irq_save(flags);
> @@ -425,16 +411,13 @@ struct task_struct *__switch_to(struct t
> * of sync. Hard disable here.
> */
> hard_irq_disable();
> - last = _switch(old_thread, new_thread);
> -
> - local_irq_restore(flags);
>
> #if defined(CONFIG_PPC64) && defined(CONFIG_PREEMPT_RT)
> - if (hadbatch) {
> - batch = &__get_cpu_var(ppc64_tlb_batch);
> - batch->active = 1;
> - }
> + arch_leave_lazy_mmu_mode();
> #endif
> + last = _switch(old_thread, new_thread);
> +
> + local_irq_restore(flags);
>
> return last;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Will Schmidt on 4 Aug 2010 17:50
On Sun, 2010-07-11 at 02:49 -0500, Milton Miller wrote:
> On Fri, 09 Jul 2010 about 08:55:01 -0000, Will Schmidt wrote:
> > We've been seeing some issues with userspace randomly SIGSEGV'ing while
> > running the -RT kernels on POWER7 based systems. After lots of
> > debugging, head scratching, and experimental changes to the code, the
> > problem has been narrowed down such that we can avoid the problems by
> > disabling the TLB batching.
> >
> > After some input from Ben and further debug, we've found that the
> > restoration of the batch->active value near the end of __switch_to()
> > seems to be the key. ( The -RT related changes within
> > arch/powerpc/kernel/processor.c __switch_to() do the equivalent of a
> > arch_leave_lazy_mmu_mode() before calling _switch, use a hadbatch flag
> > to indicate if batching was active, and then restore that batch->active
> > value on the way out after the call to _switch_to. That particular
> > code is in the -RT branch, and not found in mainline )
> >
> > Deferring to Ben (or others in the know) for whether this is the proper
> > solution or if there is something deeper, but..

I believe this is still on Ben's list of things to look at. Between
then and now, I'll see if I can get Thomas to pick this up for the -RT
tree to keep RT functional on P7 in the mean-time.

A bit more debug info below.

>
>
> I looked at the patch and noticed 2 changes:
> 1) the batch is checked and cleared after local_irq_save
> 2) enabling the batch is skipped
>
> I talked to Will and had him try moving the local_irq_save above the
> check for the active batch. That alone did not seem to be enough.
> However, he confirmed that we are setting batch to active when it is
> already active in lazy_mmu_enter, meaning that batching is being turned
> on recursively. I suggested debug to check that irqs are off after the
> restore when re-enabling when our debug session timed out.

Based on some of the debug suggestions from Milton:


A WARN_ON for (!irqs_disabled) after local_irq_restore() did not show
any hits. (while otherwise continuing to suffer from the tlb batching
troubles).

---><----

hard_irq_disable();
last = _switch(old_thread, new_thread);

local_irq_restore(flags);

WARN_ON(!irqs_disabled()); <<<<----------


#if defined(CONFIG_PPC64) && defined(CONFIG_PREEMPT_RT) && 1
if (hadbatch) {
batch = &__get_cpu_var(ppc64_tlb_batch);
batch->active = 1;
}
#endif

----><----


Another assortment of WARN_ONs in the arch_{enter,leave}_lazy_mmu_mode
functions. As Milton stated above, the check for batch->active on the
way into the arch_enter_* function did generate lots of hits, the other
warn_ons did not.

-----><-------
static inline void arch_enter_lazy_mmu_mode(void)
{
struct ppc64_tlb_batch *batch = &get_cpu_var(ppc64_tlb_batch);

//|-----WARN_ON(batch->active); /* lots of hits if enabled */
|-------WARN_ON(irqs_disabled()); /* nothing.... */
|-------batch->active = 1;

.....


static inline void arch_leave_lazy_mmu_mode(void)
{
|-------struct ppc64_tlb_batch *batch = &get_cpu_var(ppc64_tlb_batch);

|-------WARN_ON(!batch->active); /* nothing.....*/
|-------WARN_ON(irqs_disabled()); /* nothing.... */

.....



>
> milton
>
> >
> > diff -aurp linux-2.6.33.5-rt23.orig/arch/powerpc/kernel/process.c linux-2.6.33.5-rt23.exp/arch/powerpc/kernel/process.c
> > --- linux-2.6.33.5-rt23.orig/arch/powerpc/kernel/process.c 2010-06-21 11:41:34.402513904 -0500
> > +++ linux-2.6.33.5-rt23.exp/arch/powerpc/kernel/process.c 2010-07-09 13:15:13.533269904 -0500
> > @@ -304,10 +304,6 @@ struct task_struct *__switch_to(struct t
> > struct thread_struct *new_thread, *old_thread;
> > unsigned long flags;
> > struct task_struct *last;
> > -#if defined(CONFIG_PPC64) && defined (CONFIG_PREEMPT_RT)
> > - struct ppc64_tlb_batch *batch;
> > - int hadbatch;
> > -#endif
> >
> > #ifdef CONFIG_SMP
> > /* avoid complexity of lazy save/restore of fpu
> > @@ -401,16 +397,6 @@ struct task_struct *__switch_to(struct t
> > new_thread->start_tb = current_tb;
> > }
> >
> > -#ifdef CONFIG_PREEMPT_RT
> > - batch = &__get_cpu_var(ppc64_tlb_batch);
> > - if (batch->active) {
> > - hadbatch = 1;
> > - if (batch->index) {
> > - __flush_tlb_pending(batch);
> > - }
> > - batch->active = 0;
> > - }
> > -#endif /* #ifdef CONFIG_PREEMPT_RT */
> > #endif
> >
> > local_irq_save(flags);
> > @@ -425,16 +411,13 @@ struct task_struct *__switch_to(struct t
> > * of sync. Hard disable here.
> > */
> > hard_irq_disable();
> > - last = _switch(old_thread, new_thread);
> > -
> > - local_irq_restore(flags);
> >
> > #if defined(CONFIG_PPC64) && defined(CONFIG_PREEMPT_RT)
> > - if (hadbatch) {
> > - batch = &__get_cpu_var(ppc64_tlb_batch);
> > - batch->active = 1;
> > - }
> > + arch_leave_lazy_mmu_mode();
> > #endif
> > + last = _switch(old_thread, new_thread);
> > +
> > + local_irq_restore(flags);
> >
> > return last;
> > }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: sound: push BKL into open functions
Next: edd: fix possible memory leak in edd_init() error path