| Message ID | CANj_sbADwtPR6cgk05QkmvrSUOOQRYXB9vPB6KATKEmwcN8UrA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 2, 2012 at 3:07 PM, Andrew Yan-Pai Chen <yanpai.chen@gmail.com> wrote: > On Mon, Oct 1, 2012 at 11:50 AM, Jason Lin <kernel.jason@gmail.com> wrote: >> >> Dear all: >> By the way, I used the CPU with VIPT cache. >> Thanks. >> >> 2012/10/1 Jason Lin <kernel.jason@gmail.com>: >> > Dear all: >> > When I do LTP direct I/O tests, it produced a cache coherence issue >> > caused by flush_pfn_alias() (arch/arm/mm/flush.c). >> > I think we should to disable preemption or lock before setting page >> > table and enable preemption or unlock after flushing pages >> > >> > Any comments are appreciated. >> > Thanks. >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > [RFC PATCH] make flush_pfn_alias() nonpreemptible > > Since flush_pfn_alias() is preemptible, it is possible to be > preempted just after set_top_pte() is done. If the process > which preempts the previous happened to invoke flush_pfn_alias() > with the same colour vaddr as that of the previous, the same > top pte will be overwritten. When switching back to the previous, > it attempts to flush cache lines with incorrect mapping. Then > no lines (or wrong lines) will be flushed because of the nature > of vipt caches. > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > index 40ca11e..bd07918 100644 > --- a/arch/arm/mm/flush.c > +++ b/arch/arm/mm/flush.c > @@ -27,6 +27,8 @@ static void flush_pfn_alias(unsigned long pfn, > unsigned long vaddr) > unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << > PAGE_SHIFT); > const int zero = 0; > > + preempt_disable(); > + > set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL)); > > asm( "mcrr p15, 0, %1, %0, c14\n" > @@ -34,6 +36,8 @@ static void flush_pfn_alias(unsigned long pfn, > unsigned long vaddr) > : > : "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero) > : "cc"); > + > + preempt_enable(); > } > > -- > Regards, > Andrew Hi Russell, We met the problem when running the direct I/O test (diotest03) in LTP testsuite on PB1176. And we got the error messages: bufcmp: offset 224: Expected: 0x1b, got 0x1a diotest03 1 TFAIL : comparsion failed; child=11 offset=1069056 diotest03 2 TFAIL : Read Direct-child 11 failed ... The error was gone after applying this patch. Any idea would be appreciated. -- Regards, Andrew
On Tue, Oct 02, 2012 at 03:07:33PM +0800, Andrew Yan-Pai Chen wrote: > [RFC PATCH] make flush_pfn_alias() nonpreemptible > > Since flush_pfn_alias() is preemptible, it is possible to be > preempted just after set_top_pte() is done. If the process > which preempts the previous happened to invoke flush_pfn_alias() > with the same colour vaddr as that of the previous, the same > top pte will be overwritten. When switching back to the previous, > it attempts to flush cache lines with incorrect mapping. Then > no lines (or wrong lines) will be flushed because of the nature > of vipt caches. > > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c > index 40ca11e..bd07918 100644 > --- a/arch/arm/mm/flush.c > +++ b/arch/arm/mm/flush.c > @@ -27,6 +27,8 @@ static void flush_pfn_alias(unsigned long pfn, > unsigned long vaddr) > unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << > PAGE_SHIFT); > const int zero = 0; > > + preempt_disable(); > + > set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL)); > > asm( "mcrr p15, 0, %1, %0, c14\n" > @@ -34,6 +36,8 @@ static void flush_pfn_alias(unsigned long pfn, > unsigned long vaddr) > : > : "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero) > : "cc"); > + > + preempt_enable(); > } This looks like it's quite correct, but if you look at the file a little deeper, you'll notice that flush_icache_alias() potentially suffers the same issue. They should probably both also have a comment indicating that they're not to be called for SMP setups (because there's no locking for that.)
On Fri, Oct 5, 2012 at 5:10 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Oct 02, 2012 at 03:07:33PM +0800, Andrew Yan-Pai Chen wrote: >> [RFC PATCH] make flush_pfn_alias() nonpreemptible >> >> Since flush_pfn_alias() is preemptible, it is possible to be >> preempted just after set_top_pte() is done. If the process >> which preempts the previous happened to invoke flush_pfn_alias() >> with the same colour vaddr as that of the previous, the same >> top pte will be overwritten. When switching back to the previous, >> it attempts to flush cache lines with incorrect mapping. Then >> no lines (or wrong lines) will be flushed because of the nature >> of vipt caches. >> >> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c >> index 40ca11e..bd07918 100644 >> --- a/arch/arm/mm/flush.c >> +++ b/arch/arm/mm/flush.c >> @@ -27,6 +27,8 @@ static void flush_pfn_alias(unsigned long pfn, >> unsigned long vaddr) >> unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << >> PAGE_SHIFT); >> const int zero = 0; >> >> + preempt_disable(); >> + >> set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL)); >> >> asm( "mcrr p15, 0, %1, %0, c14\n" >> @@ -34,6 +36,8 @@ static void flush_pfn_alias(unsigned long pfn, >> unsigned long vaddr) >> : >> : "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero) >> : "cc"); >> + >> + preempt_enable(); >> } > > This looks like it's quite correct, but if you look at the file a little > deeper, you'll notice that flush_icache_alias() potentially suffers the > same issue. > > They should probably both also have a comment indicating that they're not > to be called for SMP setups (because there's no locking for that.) OK. I will resend the patch later. Thanks! -- Regards, Andrew
On Fri, Oct 5, 2012 at 6:09 PM, Andrew Yan-Pai Chen <yanpai.chen@gmail.com> wrote: > On Fri, Oct 5, 2012 at 5:10 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: >> On Tue, Oct 02, 2012 at 03:07:33PM +0800, Andrew Yan-Pai Chen wrote: >>> [RFC PATCH] make flush_pfn_alias() nonpreemptible >>> >>> Since flush_pfn_alias() is preemptible, it is possible to be >>> preempted just after set_top_pte() is done. If the process >>> which preempts the previous happened to invoke flush_pfn_alias() >>> with the same colour vaddr as that of the previous, the same >>> top pte will be overwritten. When switching back to the previous, >>> it attempts to flush cache lines with incorrect mapping. Then >>> no lines (or wrong lines) will be flushed because of the nature >>> of vipt caches. >>> >>> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c >>> index 40ca11e..bd07918 100644 >>> --- a/arch/arm/mm/flush.c >>> +++ b/arch/arm/mm/flush.c >>> @@ -27,6 +27,8 @@ static void flush_pfn_alias(unsigned long pfn, >>> unsigned long vaddr) >>> unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << >>> PAGE_SHIFT); >>> const int zero = 0; >>> >>> + preempt_disable(); >>> + >>> set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL)); >>> >>> asm( "mcrr p15, 0, %1, %0, c14\n" >>> @@ -34,6 +36,8 @@ static void flush_pfn_alias(unsigned long pfn, >>> unsigned long vaddr) >>> : >>> : "r" (to), "r" (to + PAGE_SIZE - L1_CACHE_BYTES), "r" (zero) >>> : "cc"); >>> + >>> + preempt_enable(); >>> } >> >> This looks like it's quite correct, but if you look at the file a little >> deeper, you'll notice that flush_icache_alias() potentially suffers the >> same issue. >> >> They should probably both also have a comment indicating that they're not >> to be called for SMP setups (because there's no locking for that.) > > OK. I will resend the patch later. > Thanks! > > -- > Regards, > Andrew Hi Russell, Is flush_icache_alias() not to be called for SMP setups? It seems that processors with a vipt non-aliasing D-cache but a vipt aliasing I-cache (such like cortex-a9) will call this function. So perhaps we should have a lock in this case? -- Regards, Andrew
diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c index 40ca11e..bd07918 100644 --- a/arch/arm/mm/flush.c +++ b/arch/arm/mm/flush.c @@ -27,6 +27,8 @@ static void flush_pfn_alias(unsigned long pfn, unsigned long vaddr) unsigned long to = FLUSH_ALIAS_START + (CACHE_COLOUR(vaddr) << PAGE_SHIFT); const int zero = 0; + preempt_disable(); + set_top_pte(to, pfn_pte(pfn, PAGE_KERNEL));