2.6.13-rc3: cache flush missing from somewhere

2.6.13-rc3: cache flush missing from somewhere

[Russell King]

Hi,

I've been trying for the last 4 or 5 days to get the kernel stable on
an ARM SMP platform.  This platform has harvard PIPT caches with no
aliasing issues inside the separate I/D caches, except for the lack
of snooping between the I and D cache.  The caches are in write allocate
mode.

This means we need to ensure that the I/D coherency is handled, and
we do this via flush_dcache_page().  We actually do this lazily using
the Sparc64 method, so __flush_dcache_page() actually does the cache
operations.

My current patch to get this working is below.  The only thing which
really seems to fix the issue is the __flush_dcache_page call in
read_pages() - if I remove this, I get spurious segfaults and illegal
instruction faults.

If I make flush_dcache_page() non-lazy, this also fixes it, but this
is not desirable.  The problem also goes away if I disable the write
allocate cache mode.

If I call __flush_dcache_page() from update_mmu_cache() (iow, always
ensure that we have I/D coherency when the page is mapped into user
space) the effect is the same - I see random faults.

This is using cramfs as the filesystem, which does call flush_dcache_page()
on pages returned via its readpages implementation.

Unfortunately, I've only recently obtained this hardware, but I know
a previous kernel (2.6.7-based) works fine on it (already supplied by
others.)  However, there's a massive delta from mainline for this
which makes it totally impractical to try other mainline kernels.

To me, it feels like there's a path which results in pages mapped into
user space without update_mmu_cache() being called, but I'm unable to
find it.  Ideas?

diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -r orig/mm/filemap.c linux/mm/filemap.c
--- orig/mm/filemap.c	Wed Jun 29 15:52:51 2005
+++ linux/mm/filemap.c	Fri Jul 29 15:32:40 2005
@@ -849,6 +849,8 @@ readpage:
 			unlock_page(page);
 		}
 
+if (page->mapping && !mapping_mapped(page->mapping)) BUG_ON(!test_bit(PG_dcache_dirty, &page->flags));
+{ void __flush_dcache_page(struct address_space *mapping, struct page *page); __flush_dcache_page(page->mapping, page); }
 		/*
 		 * i_size must be checked after we have done ->readpage.
 		 *
@@ -1158,6 +1160,8 @@ static int fastcall page_cache_read(stru
 	error = add_to_page_cache_lru(page, mapping, offset, GFP_KERNEL);
 	if (!error) {
 		error = mapping->a_ops->readpage(file, page);
+if (page->mapping && !mapping_mapped(page->mapping)) BUG_ON(!test_bit(PG_dcache_dirty, &page->flags));
+{ void __flush_dcache_page(struct address_space *mapping, struct page *page); __flush_dcache_page(page->mapping, page); }
 		page_cache_release(page);
 		return error;
 	}
@@ -1254,7 +1258,9 @@ retry_find:
 		page = find_get_page(mapping, pgoff);
 		if (!page)
 			goto no_cached_page;
+if (page->mapping && !mapping_mapped(page->mapping)) BUG_ON(!test_bit(PG_dcache_dirty, &page->flags));
 	}
+if (page->mapping && !mapping_mapped(page->mapping)) BUG_ON(!test_bit(PG_dcache_dirty, &page->flags));
 
 	if (!did_readaround)
 		ra->mmap_hit++;
@@ -1267,6 +1273,8 @@ retry_find:
 		goto page_not_uptodate;
 
 success:
+if (page->mapping && !mapping_mapped(page->mapping)) BUG_ON(!test_bit(PG_dcache_dirty, &page->flags));
+{ void __flush_dcache_page(struct address_space *mapping, struct page *page); __flush_dcache_page(page->mapping, page); }
 	/*
 	 * Found the page and have a reference on it.
 	 */
@@ -1402,6 +1410,8 @@ retry_find:
 	}
 
 success:
+if (page->mapping && !mapping_mapped(page->mapping)) BUG_ON(!test_bit(PG_dcache_dirty, &page->flags));
+{ void __flush_dcache_page(struct address_space *mapping, struct page *page); __flush_dcache_page(page->mapping, page); }
 	/*
 	 * Found the page and have a reference on it.
 	 */
@@ -1508,6 +1518,7 @@ repeat:
 	if (!page && !nonblock)
 		return -ENOMEM;
 	if (page) {
+{ void __flush_dcache_page(struct address_space *mapping, struct page *page); __flush_dcache_page(page->mapping, page); }
 		err = install_page(mm, vma, addr, page, prot);
 		if (err) {
 			page_cache_release(page);
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -r orig/mm/memory.c linux/mm/memory.c
--- orig/mm/memory.c	Wed Jun 29 15:52:51 2005
+++ linux/mm/memory.c	Fri Jul 29 15:41:11 2005
@@ -1821,6 +1821,7 @@ do_no_page(struct mm_struct *mm, struct 
 retry:
 	cond_resched();
 	new_page = vma->vm_ops->nopage(vma, address & PAGE_MASK, &ret);
+{ void __flush_dcache_page(struct address_space *mapping, struct page *page); __flush_dcache_page(new_page->mapping, new_page); }
 	/*
 	 * No smp_rmb is needed here as long as there's a full
 	 * spin_lock/unlock sequence inside the ->nopage callback
diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x '*.orig' -x '*.rej' -r orig/mm/readahead.c linux/mm/readahead.c
--- orig/mm/readahead.c	Mon Apr  4 22:54:55 2005
+++ linux/mm/readahead.c	Fri Jul 29 15:57:18 2005
@@ -172,6 +172,7 @@ static int read_pages(struct address_spa
 		if (!add_to_page_cache(page, mapping,
 					page->index, GFP_KERNEL)) {
 			mapping->a_ops->readpage(filp, page);
+{ void __flush_dcache_page(struct address_space *mapping, struct page *page); __flush_dcache_page(page->mapping, page); }
 			if (!pagevec_add(&lru_pvec, page))
 				__pagevec_lru_add(&lru_pvec);
 		} else {


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.13-rc3: cache flush missing from somewhere

[David S. Miller]

From: Russell King <rmk+lkml@arm.linux.org.uk>
Date: Fri, 29 Jul 2005 16:13:43 +0100

> My current patch to get this working is below.  The only thing which
> really seems to fix the issue is the __flush_dcache_page call in
> read_pages() - if I remove this, I get spurious segfaults and illegal
> instruction faults.

If one cpu stores, does it get picked up in the other cpu's I-cache?
If not, you cannot use the lazy dcache flushing method, and in fact
you must broadcast the flush on all processors.

> If I make flush_dcache_page() non-lazy, this also fixes it, but this
> is not desirable.  The problem also goes away if I disable the write
> allocate cache mode.

Strange.

> If I call __flush_dcache_page() from update_mmu_cache() (iow, always
> ensure that we have I/D coherency when the page is mapped into user
> space) the effect is the same - I see random faults.

You have to do the flush on the processor that does the store,
at a minimum.  But if other cpus have no way to "notice" stores
on other cpus in their I-cache, this alone is not sufficient.

Based upon your report, I strongly suspect that remote I-caches
do not see cpu local stores when the cache is in write allocate
mode.  If this is true, it's a horrible design decision for an
SMP system :(

> To me, it feels like there's a path which results in pages mapped into
> user space without update_mmu_cache() being called, but I'm unable to
> find it.  Ideas?

Ptrace can make this occur, but that's obviously not what you're
seeing here.

Re: 2.6.13-rc3: cache flush missing from somewhere

[Russell King]

On Sat, Jul 30, 2005 at 12:40:52PM -0700, David S. Miller wrote:
> From: Russell King <rmk+lkml@arm.linux.org.uk>
> > My current patch to get this working is below.  The only thing which
> > really seems to fix the issue is the __flush_dcache_page call in
> > read_pages() - if I remove this, I get spurious segfaults and illegal
> > instruction faults.
> 
> If one cpu stores, does it get picked up in the other cpu's I-cache?

No - there's absolutely no coherency between the I-cache and D-cache.
However, the I-cache and D-cache are individually PIPT.

> If not, you cannot use the lazy dcache flushing method, and in fact
> you must broadcast the flush on all processors.

I guess that answers the question, thanks.

> > If I call __flush_dcache_page() from update_mmu_cache() (iow, always
> > ensure that we have I/D coherency when the page is mapped into user
> > space) the effect is the same - I see random faults.
> 
> You have to do the flush on the processor that does the store,
> at a minimum.  But if other cpus have no way to "notice" stores
> on other cpus in their I-cache, this alone is not sufficient.
> 
> Based upon your report, I strongly suspect that remote I-caches
> do not see cpu local stores when the cache is in write allocate
> mode.  If this is true, it's a horrible design decision for an
> SMP system :(

I'm now told that the system is only coherent with the caches in
write back write allocate mode, which is rather odd since it appears
stable with write back read allocate.  However, I'll forward your
comments back to the designers.

Thanks David.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.13-rc3: cache flush missing from somewhere

[David S. Miller]

From: Russell King <rmk+lkml@arm.linux.org.uk>
Date: Sat, 30 Jul 2005 21:08:07 +0100

> Thanks David.

No problem.  An interesting tidbit would be whether the
system is stable, without your patch you posted, in pure
uniprocessor mode with these cpus.

Re: 2.6.13-rc3: cache flush missing from somewhere

[Catalin Marinas]

"David S. Miller" <davem@davemloft.net> wrote:
> From: Russell King <rmk+lkml@arm.linux.org.uk>
> Date: Fri, 29 Jul 2005 16:13:43 +0100
>
>> My current patch to get this working is below.  The only thing which
>> really seems to fix the issue is the __flush_dcache_page call in
>> read_pages() - if I remove this, I get spurious segfaults and illegal
>> instruction faults.
>
> If one cpu stores, does it get picked up in the other cpu's I-cache?

It only gets picked up by the other CPU's D-cache (which is fully
coherent between cores). The I-cache needs to be invalidated on each
CPU.

> If not, you cannot use the lazy dcache flushing method, and in fact
> you must broadcast the flush on all processors.

Why wouldn't the lazy dcache flushing method work? My understanding is
that if there is no user mapping for a given page, there's no reason
to flush the dcache and just postpone it until the page is faulted
in. When the page fault occurs the dcache should be flushed (on one
CPU is enough) and the icache invalidated on all the CPUs.

-- 
Catalin

Re: 2.6.13-rc3: cache flush missing from somewhere

[David S. Miller]

From: Catalin Marinas <catalin.marinas@arm.com>
Date: Mon, 01 Aug 2005 13:24:04 +0100

> "David S. Miller" <davem@davemloft.net> wrote:
> > If not, you cannot use the lazy dcache flushing method, and in fact
> > you must broadcast the flush on all processors.
> 
> Why wouldn't the lazy dcache flushing method work? My understanding is
> that if there is no user mapping for a given page, there's no reason
> to flush the dcache and just postpone it until the page is faulted
> in. When the page fault occurs the dcache should be flushed (on one
> CPU is enough) and the icache invalidated on all the CPUs.

The "lazy dcache flushing" he mentioned only flushes on the
processor where the store occurred, not on any other cpus.

He took the sparc64 code which, at the time of the flush_dcache_page()
call, stores the current cpu number in the page->flags and sets a
bit indicating a flush is needed.  When some condition occurs
requiring the delayed flush to occur, we look at the cpu number
in the page and ask that specific cpu to do the flush.

This works perfectly for what sparc64 is trying to do, which is deal
with bad aliasing in a virtually indexed D-cache where only the cpu
who does the store has to do any flushing, but that won't work for
this ARM SMP case at all.

I've seen implementations where the I-cache does not snoop local cpu
stores, but I've never seen one where other cpus do not snoop such
stores.  You _HAVE_ to implement handling of I-cache update on L2
cache line changes to handle updates from devices doing DMA, so why
in the world special case stores done by other cpus?

It almost sounds impossible to implement this and have the I-cache
be coherent wrt. DMA transactions.

Do you have to flush the whole I-cache every time some device DMAs
a page into memory, before you can execute instructions out of it?

Re: 2.6.13-rc3: cache flush missing from somewhere

[Catalin Marinas]

"David S. Miller" <davem@davemloft.net> wrote:
> The "lazy dcache flushing" he mentioned only flushes on the
> processor where the store occurred, not on any other cpus.
>
> He took the sparc64 code which, at the time of the flush_dcache_page()
> call, stores the current cpu number in the page->flags and sets a
> bit indicating a flush is needed.  When some condition occurs
> requiring the delayed flush to occur, we look at the cpu number
> in the page and ask that specific cpu to do the flush.

That's a point I missed. The D-cache flushing should take place on the
CPU that wrote the page, not the one that got the page fault (and the
I-cache invalidation on all the CPUs). I don't see why this wouldn't
work.

> I've seen implementations where the I-cache does not snoop local cpu
> stores, but I've never seen one where other cpus do not snoop such
> stores.

On this ARM SMP implementation, only the D-cache snoops the other CPUs
stores, not the I-cache.

> You _HAVE_ to implement handling of I-cache update on L2
> cache line changes to handle updates from devices doing DMA, so why
> in the world special case stores done by other cpus?
>
> It almost sounds impossible to implement this and have the I-cache
> be coherent wrt. DMA transactions.

Shouldn't flush_dcache_page() be called anyway when a page is modified
by the kernel (or by a device via DMA)? With the Harvard cache
architecture in ARM, the I cache should be invalidated even in a
uniprocessor system. For SMP it is just a matter of invalidating it on
all the CPUs (done by issuing an inter-processor interrupt).

> Do you have to flush the whole I-cache every time some device DMAs
> a page into memory, before you can execute instructions out of it?

IMHO, it only needs invalidating the I-cache corresponding to the
DMA'ed page, not the whole I-cache but, as I said, this should be
handled by flush_dcache_page, whether lazily or not.

Thanks,

-- 
Catalin

Re: 2.6.13-rc3: cache flush missing from somewhere

[Russell King]

On Mon, Aug 01, 2005 at 01:24:04PM +0100, Catalin Marinas wrote:
> "David S. Miller" <davem@davemloft.net> wrote:
> > From: Russell King <rmk+lkml@arm.linux.org.uk>
> > Date: Fri, 29 Jul 2005 16:13:43 +0100
> >
> >> My current patch to get this working is below.  The only thing which
> >> really seems to fix the issue is the __flush_dcache_page call in
> >> read_pages() - if I remove this, I get spurious segfaults and illegal
> >> instruction faults.
> >
> > If one cpu stores, does it get picked up in the other cpu's I-cache?
> 
> It only gets picked up by the other CPU's D-cache (which is fully
> coherent between cores). The I-cache needs to be invalidated on each
> CPU.

Are you sure about this requirement?  I see no evidence of it in Harry's
patch set.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.13-rc3: cache flush missing from somewhere

[Catalin Marinas]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> On Mon, Aug 01, 2005 at 01:24:04PM +0100, Catalin Marinas wrote:
>> "David S. Miller" <davem@davemloft.net> wrote:
>> > If one cpu stores, does it get picked up in the other cpu's I-cache?
>> 
>> It only gets picked up by the other CPU's D-cache (which is fully
>> coherent between cores). The I-cache needs to be invalidated on each
>> CPU.
>
> Are you sure about this requirement?  I see no evidence of it in Harry's
> patch set.

I asked the people that know more about this architecture than me and
they confirmed that this is a requirement. I will double check
tomorrow with the people that did the initial Linux support.

I haven't checked the original patch but it might work (by luck)
without the I-cache invalidation (and without stressing it too
much). This is because you might do a full mm flush when a process
exits and the I-cache would be clean for newly allocated pages (only
the D-cache needs flushing). If you don't overload memory, you don't
get pages swapped-out/removed and the code in a page for a given
process might remain unchanged until the process exits.

-- 
Catalin

Re: 2.6.13-rc3: cache flush missing from somewhere

[Russell King]

On Mon, Aug 01, 2005 at 05:54:33PM +0100, Catalin Marinas wrote:
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > On Mon, Aug 01, 2005 at 01:24:04PM +0100, Catalin Marinas wrote:
> >> "David S. Miller" <davem@davemloft.net> wrote:
> >> > If one cpu stores, does it get picked up in the other cpu's I-cache?
> >> 
> >> It only gets picked up by the other CPU's D-cache (which is fully
> >> coherent between cores). The I-cache needs to be invalidated on each
> >> CPU.
> >
> > Are you sure about this requirement?  I see no evidence of it in Harry's
> > patch set.
> 
> I asked the people that know more about this architecture than me and
> they confirmed that this is a requirement. I will double check
> tomorrow with the people that did the initial Linux support.
> 
> I haven't checked the original patch but it might work (by luck)
> without the I-cache invalidation (and without stressing it too
> much). This is because you might do a full mm flush when a process
> exits and the I-cache would be clean for newly allocated pages (only
> the D-cache needs flushing). If you don't overload memory, you don't
> get pages swapped-out/removed and the code in a page for a given
> process might remain unchanged until the process exits.

Given that it's sometimes going wrong as early as the first process, I
doubt this is what's happening.  The i-cache will be clean at this point
for the userspace programs.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.13-rc3: cache flush missing from somewhere

[Catalin Marinas]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> On Mon, Aug 01, 2005 at 05:54:33PM +0100, Catalin Marinas wrote:
>> I haven't checked the original patch but it might work (by luck)
>> without the I-cache invalidation (and without stressing it too
>> much). This is because you might do a full mm flush when a process
>> exits and the I-cache would be clean for newly allocated pages (only
>> the D-cache needs flushing). If you don't overload memory, you don't
>> get pages swapped-out/removed and the code in a page for a given
>> process might remain unchanged until the process exits.
>
> Given that it's sometimes going wrong as early as the first process, I
> doubt this is what's happening.  The i-cache will be clean at this point
> for the userspace programs.

The I-cache might not be completely clean when executing the first
process since the kernel already ran code from the .init.text section
which is freed.

Anyway, the problem you are seeing, apart from the I-cache flushing,
looks to me like being caused by not flushing the D-cache on the CPU
that actually wrote the page (not the one handling the page
fault). That's why flushing immediately (not lazily) seems to work.

-- 
Catalin