diff mbox

powerpc/8xx: fix regression introduced by cache coherency rewrite

Message ID 20090925013528.GA2584@compile2.chatsunix.int.mrv.com (mailing list archive)
State Superseded
Headers show

Commit Message

Rex Feany Sept. 25, 2009, 1:35 a.m. UTC 
Thus spake Benjamin Herrenschmidt (benh@kernel.crashing.org):

> > Your tree hangs on boot, similar to what I saw without the 8xx
> > work-around patch -- it is hard to tell if it is the same though. :(
> 
> There's no backtrace ? Where does it hang ? Also which workaround
> patch ? The missing tlbil_va() or the _PAGE_SPECIAL problem ?
> 

Ben, I'm sorry, my last email was basically useless. I was refering to the
missing tlbil_va().  The system doesn't crash, but it does seem to hang right
after "Freeing unused kernel memory: 100k init" using your tree.

If I move the tlbil_va() outside of the test for PG_arch_1 :


Then I can boot and get to a shell, but userspace is slow. 8 seconds to mount
/proc (vs. less then a second using my old kernel)! Maybe this is an
unrelated issue?  I'm pretty clueless about the details, I'm sorry.
PG_arch_1 is used to prevent a cache flush unless it is actually needed?
Then why would changing the location of the tlbil_va() make a difference?

take care!
/rex.

Comments

Benjamin Herrenschmidt Sept. 25, 2009, 1:51 a.m. UTC | #1 
On Thu, 2009-09-24 at 18:35 -0700, Rex Feany wrote:
> 
> Then I can boot and get to a shell, but userspace is slow. 8 seconds
> to mount
> /proc (vs. less then a second using my old kernel)! Maybe this is an
> unrelated issue?  I'm pretty clueless about the details, I'm sorry.
> PG_arch_1 is used to prevent a cache flush unless it is actually
> needed?
> Then why would changing the location of the tlbil_va() make a
> difference?

I think there's more finishyness to 8xx than we thought. IE. That
tlbil_va might have more reasons to be there than what the comment
seems to advertize. Can you try to move it even higher up ? IE.
Unconditionally at the beginning of set_pte_filter ?

Also, if that doesn't help, can you try putting one in
set_access_flags_filter() just below ?

(Beware that there's two different versions of both functions, only the
first one is compiled/used on 8xx).

It's going to be hard for me to get that "right" since I don't really
know what's going on with the core here, but I suppose if we get it
moving along with extra tlb invalidations, that should be "good enough"
until somebody who really knows what's going on comes up with possibly
a better fix.

Cheers,
Ben.
Rex Feany Sept. 29, 2009, 1:21 a.m. UTC | #2 
Thus spake Benjamin Herrenschmidt (benh@kernel.crashing.org):

> On Thu, 2009-09-24 at 18:35 -0700, Rex Feany wrote:
> > 
> > Then I can boot and get to a shell, but userspace is slow. 8 seconds
> > to mount
> > /proc (vs. less then a second using my old kernel)! Maybe this is an
> > unrelated issue?  I'm pretty clueless about the details, I'm sorry.
> > PG_arch_1 is used to prevent a cache flush unless it is actually
> > needed?
> > Then why would changing the location of the tlbil_va() make a
> > difference?
> 
> I think there's more finishyness to 8xx than we thought. IE. That
> tlbil_va might have more reasons to be there than what the comment
> seems to advertize. Can you try to move it even higher up ? IE.
> Unconditionally at the beginning of set_pte_filter ?
> 
> Also, if that doesn't help, can you try putting one in
> set_access_flags_filter() just below ?
> 
> (Beware that there's two different versions of both functions, only the
> first one is compiled/used on 8xx).
> 
> It's going to be hard for me to get that "right" since I don't really
> know what's going on with the core here, but I suppose if we get it
> moving along with extra tlb invalidations, that should be "good enough"
> until somebody who really knows what's going on comes up with possibly
> a better fix.

I've tried sticking tlbil_va() in those places, nothing seems to help.
In some cases userspace is slow, in other cases userspace is faster and
unstable: sometimes commands hang, sometimes I am able to ctrl-c and
and kill it, sometimes I get other strange crashes or falures (so far no
kernel oopses though).

take care!
/rex.
Joakim Tjernlund Sept. 29, 2009, 6:26 a.m. UTC | #3 
>
> Thus spake Benjamin Herrenschmidt (benh@kernel.crashing.org):
>
> > On Thu, 2009-09-24 at 18:35 -0700, Rex Feany wrote:
> > >
> > > Then I can boot and get to a shell, but userspace is slow. 8 seconds
> > > to mount
> > > /proc (vs. less then a second using my old kernel)! Maybe this is an
> > > unrelated issue?  I'm pretty clueless about the details, I'm sorry.
> > > PG_arch_1 is used to prevent a cache flush unless it is actually
> > > needed?
> > > Then why would changing the location of the tlbil_va() make a
> > > difference?
> >
> > I think there's more finishyness to 8xx than we thought. IE. That
> > tlbil_va might have more reasons to be there than what the comment
> > seems to advertize. Can you try to move it even higher up ? IE.
> > Unconditionally at the beginning of set_pte_filter ?
> >
> > Also, if that doesn't help, can you try putting one in
> > set_access_flags_filter() just below ?
> >
> > (Beware that there's two different versions of both functions, only the
> > first one is compiled/used on 8xx).
> >
> > It's going to be hard for me to get that "right" since I don't really
> > know what's going on with the core here, but I suppose if we get it
> > moving along with extra tlb invalidations, that should be "good enough"
> > until somebody who really knows what's going on comes up with possibly
> > a better fix.
>
> I've tried sticking tlbil_va() in those places, nothing seems to help.
> In some cases userspace is slow, in other cases userspace is faster and
> unstable: sometimes commands hang, sometimes I am able to ctrl-c and
> and kill it, sometimes I get other strange crashes or falures (so far no
> kernel oopses though).

This is exactly what you get when the "cache insn does not update DAR" bug hits
you.
Benjamin Herrenschmidt Sept. 29, 2009, 7:07 a.m. UTC | #4 
On Mon, 2009-09-28 at 18:21 -0700, Rex Feany wrote:
> > It's going to be hard for me to get that "right" since I don't really
> > know what's going on with the core here, but I suppose if we get it
> > moving along with extra tlb invalidations, that should be "good enough"
> > until somebody who really knows what's going on comes up with possibly
> > a better fix.
> 
> I've tried sticking tlbil_va() in those places, nothing seems to help.
> In some cases userspace is slow, in other cases userspace is faster and
> unstable: sometimes commands hang, sometimes I am able to ctrl-c and
> and kill it, sometimes I get other strange crashes or falures (so far no
> kernel oopses though).

And you are positive that with 2.6.31 and your other patch, it works
both fast and stable ? This is strange... the code should be mostly
identical. I'll have a second look and see if I can get you a patch that
reproduce -exactly- the behaviour of 2.6.31 plus your patch.

Cheers,
Ben.
Benjamin Herrenschmidt Sept. 29, 2009, 7:07 a.m. UTC | #5 
On Tue, 2009-09-29 at 08:26 +0200, Joakim Tjernlund wrote:
> > I've tried sticking tlbil_va() in those places, nothing seems to
> help.
> > In some cases userspace is slow, in other cases userspace is faster
> and
> > unstable: sometimes commands hang, sometimes I am able to ctrl-c and
> > and kill it, sometimes I get other strange crashes or falures (so
> far no
> > kernel oopses though).
> 
> This is exactly what you get when the "cache insn does not update DAR"
> bug hits
> you.

But then why was it working fine before ? Or it wasn't ?

Ben.
Rex Feany Sept. 29, 2009, 9:09 p.m. UTC | #6 
Thus spake Benjamin Herrenschmidt (benh@kernel.crashing.org):

> On Mon, 2009-09-28 at 18:21 -0700, Rex Feany wrote:
> > > It's going to be hard for me to get that "right" since I don't really
> > > know what's going on with the core here, but I suppose if we get it
> > > moving along with extra tlb invalidations, that should be "good enough"
> > > until somebody who really knows what's going on comes up with possibly
> > > a better fix.
> > 
> > I've tried sticking tlbil_va() in those places, nothing seems to help.
> > In some cases userspace is slow, in other cases userspace is faster and
> > unstable: sometimes commands hang, sometimes I am able to ctrl-c and
> > and kill it, sometimes I get other strange crashes or falures (so far no
> > kernel oopses though).
> 
> And you are positive that with 2.6.31 and your other patch, it works
> both fast and stable ? This is strange... the code should be mostly
> identical. I'll have a second look and see if I can get you a patch that
> reproduce -exactly- the behaviour of 2.6.31 plus your patch.

The difference is night and day - with 2.6.31 I can boot into single
user mode, I can run our custom software (I left it running over night
with very simple test script - no crashes).

With the top of the tree I can sometimes boot into a shell, and if it
isn't crashing or hanging on boot it runs very slow (10+ seconds to do
anything, if I am lucky).

thanks!
/rex
diff mbox

Patch

diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 5304093..d927269 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -176,7 +176,7 @@  static pte_t set_pte_filter(pte_t pte, unsigned long addr)
                struct page *pg = maybe_pte_to_page(pte);
                if (!pg)
                        return pte;
-               if (!test_bit(PG_arch_1, &pg->flags)) {
+
 #ifdef CONFIG_8xx
                        /* On 8xx, cache control instructions (particularly
                         * "dcbst" from flush_dcache_icache) fault as write
@@ -188,6 +188,8 @@  static pte_t set_pte_filter(pte_t pte, unsigned long addr)
                        /* 8xx doesn't care about PID, size or ind args */
                        _tlbil_va(addr, 0, 0, 0);
 #endif /* CONFIG_8xx */
+
+               if (!test_bit(PG_arch_1, &pg->flags)) {
                        flush_dcache_icache_page(pg);
                        set_bit(PG_arch_1, &pg->flags);
                }