Probably cache synchronization issue on SH7723

Hello,

when starting bigger graphical applications they suddenly stop with a segmentation fault or a bus error. If I execute the application a second time, everything is fine. Applications that are linked to shared libraries are more likely to fail than statically linked applications.

Changing the cache from write-back to write-through fixes it. Maybe the instruction cache and the data cache are not synchronized correctly?

I didn't have this problem with the linux-2.6.29 vanilla kernel. 2.6.32rc2 from
git://git.kernel.org/pub/scm/linux/kernel/git/lethal/sh-2.6 introduced it. And after a pull yesterday even /bin/init is failing.

On 9th Oct 2009, there was a similar bug report for the sh7785lcr. Does this platform now work?

Mit freundlichen Grüßen / Best Regards

Markus Pietrek
Software engineer

emtrion GmbH
Greschbachstr. 12
76229 Karlsruhe
GERMANY

Phone  +49 721 62725-45
Fax    +49 721 62725-19
E-mail [email protected]
http://www.emtrion.de

_____________________________________

Amtsgericht Mannheim
HRB 110 300
Geschäftsführer: Dieter Baur, Ramona Maurer
_____________________________________

Important Note:
- This e-mail may contain trade secrets or privileged, undisclosed or 
otherwise confidential information.
- If you have received this e-mail in error, you are hereby notified that
   any review, copying or distribution of it is strictly prohibited.
- Please inform us immediately and destroy the original transmittal.
Thank you for your cooperation.

Hi Markus,

I've found that sh7722 and sh7724 doesn't work with copy back cache right now, see below. There may be other issues as well, but going back to OK fixes it for my initramfs-only case.

Cheers,

/ magnus

BAD:

commit 39ac11c1607f1d566e7cf885acd403fa4f07f8a2

Author: Stuart Menefy <[email protected]>
Date:   Tue Oct 27 15:14:06 2009 +0000

sh: Improve performance of SH4 versions of copy/clear_user_highpage

OK:

commit 49fb2cd2571e0134e5a12c5abab227696e4940c7
Merge: dfc3494 260af56
Author: Paul Mundt <[email protected]>
Date: Tue Nov 24 16:32:11 2009 +0900

Merge branch 'master' into sh/st-integration

I've reverted the problematic parts of this patch for now, so that should fix up these cases. There's an outstanding issue addressed by Matt's patch that is being debugged now and which I will push once the remaining faults on SH7724 have been resolved.

In any event, these changes are bound for 2.6.33, so stock 2.6.32 should never have been impacted by any of this, and the remaining issues should be sorted before 2.6.33-rc1. We'll give it another spin for 2.6.34.

Hi,

Lacking some understanding of the caching system I'm not sure why in update_cache() some dirty pages are skipped. The following patch fixes my problem, but I'm not sure about any negative side effects.

index e9415d3..30c3d7e 100644
@@ -136,8 +136,7 @@ void __update_cache(struct vm_area_struct *vma,
                if (dirty) {
                        unsigned long addr = (unsigned long)page_address(page);
 
-                       if (pages_do_alias(addr, address & PAGE_MASK))
-                               __flush_purge_region((void *)addr, PAGE_SIZE);
+                        __flush_purge_region((void *)addr, PAGE_SIZE);
                }
        }
 }

Best regards,
Markus Pietrek

-----Ursprüngliche Nachricht-----
Von: [email protected] [mailto:[email protected]] Im Auftrag von Paul Mundt
Gesendet: Freitag, 4. Dezember 2009 07:48
An: Magnus Damm
Cc: Pietrek, Markus; [email protected]
Betreff: Re: Probably cache synchronization issue on SH7723

I've reverted the problematic parts of this patch for now, so that should fix up these cases. There's an outstanding issue addressed by Matt's patch that is being debugged now and which I will push once the remaining faults on SH7724 have been resolved.

In any event, these changes are bound for 2.6.33, so stock 2.6.32 should never have been impacted by any of this, and the remaining issues should be sorted before 2.6.33-rc1. We'll give it another spin for 2.6.34. --
To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

_____________________________________

Amtsgericht Mannheim
HRB 110 300
Geschäftsführer: Dieter Baur, Ramona Maurer
_____________________________________

Important Note:
- This e-mail may contain trade secrets or privileged, undisclosed or otherwise confidential information.
- If you have received this e-mail in error, you are hereby notified that any review, copying or distribution of it is strictly prohibited.
- Please inform us immediately and destroy the original transmittal.

Thank you for your cooperation.

Interesting! Good investigative work.

It's an optimisation so that we don't flush the dcache more than we need to. There shouldn't be any negative side effects of your patch, it's just a bit inefficient.

Having looked at the MIPS __update_cache() function I'm wondering if the bug is that we should be flushing the cache if the page is executable.

Could you try this patch?

index e9415d3..8460bbf 100644
@@ -126,8 +126,9 @@ void __update_cache(struct vm_area_struct *vma,
 {
 	struct page *page;
 	unsigned long pfn = pte_pfn(pte);
+	int exec = (vma->vm_flags & VM_EXEC);
 
-	if (!boot_cpu_data.dcache.n_aliases)
+	if (!boot_cpu_data.dcache.n_aliases && !exec)
 		return;
 
 	page = pfn_to_page(pfn);

@@ -136,7 +137,7 @@ void __update_cache(struct vm_area_struct *vma,
 		if (dirty) {
 			unsigned long addr = (unsigned long)page_address(page);
 
-			if (pages_do_alias(addr, address & PAGE_MASK))
+			if (exec || pages_do_alias(addr, address & PAGE_MASK))
 				__flush_purge_region((void *)addr, PAGE_SIZE);
 		}
 	}

Hi Matt,

update_mmu_cache() is called with vma = NULL in arch/sh/mm/fault_32.c. If we then assume exec=1, the patch works fine.

index e9415d3..054bb8a 100644
@@ -126,8 +126,10 @@ void __update_cache(struct vm_area_struct *vma,
 {
        struct page *page;
        unsigned long pfn = pte_pfn(pte);
+       /* flush any dirty data page before using it as a code page */
+       int exec = (vma != NULL) ? (vma->vm_flags & VM_EXEC) : 1;
 
-       if (!boot_cpu_data.dcache.n_aliases)
+       if (!boot_cpu_data.dcache.n_aliases && !exec)
                return;
 
        page = pfn_to_page(pfn);

@@ -136,7 +138,7 @@ void __update_cache(struct vm_area_struct *vma,
                if (dirty) {
                        unsigned long addr = (unsigned long)page_address(page);
 
-                       if (pages_do_alias(addr, address & PAGE_MASK))
+                       if (exec || pages_do_alias(addr, address & PAGE_MASK))
                                __flush_purge_region((void *)addr, PAGE_SIZE);
                }
        }

Best regards,
Markus Pietrek

-----Ursprüngliche Nachricht-----
Von: Matt Fleming [mailto:[email protected]]
Gesendet: Dienstag, 22. Dezember 2009 17:52
An: Pietrek, Markus
Cc: [email protected]
Betreff: Re: Probably cache synchronization issue on SH7723

Interesting! Good investigative work.

It's an optimisation so that we don't flush the dcache more than we need to. There shouldn't be any negative side effects of your patch, it's just a bit inefficient.

Having looked at the MIPS __update_cache() function I'm wondering if the bug is that we should be flushing the cache if the page is executable.

Could you try this patch?

index e9415d3..8460bbf 100644
@@ -126,8 +126,9 @@ void __update_cache(struct vm_area_struct *vma,
 {
        struct page *page;
        unsigned long pfn = pte_pfn(pte);
+       int exec = (vma->vm_flags & VM_EXEC);
 
-       if (!boot_cpu_data.dcache.n_aliases)
+       if (!boot_cpu_data.dcache.n_aliases && !exec)
                return;
 
        page = pfn_to_page(pfn);

@@ -136,7 +137,7 @@ void __update_cache(struct vm_area_struct *vma,
                if (dirty) {
                        unsigned long addr = (unsigned long)page_address(page);
 
-                       if (pages_do_alias(addr, address & PAGE_MASK))
+                       if (exec || pages_do_alias(addr, address & PAGE_MASK))
                                __flush_purge_region((void *)addr, PAGE_SIZE);
                }
        }

_____________________________________

Amtsgericht Mannheim
HRB 110 300
Geschäftsführer: Dieter Baur, Ramona Maurer
_____________________________________

Important Note:
- This e-mail may contain trade secrets or privileged, undisclosed or otherwise confidential information.
- If you have received this e-mail in error, you are hereby notified that any review, copying or distribution of it is strictly prohibited.
- Please inform us immediately and destroy the original transmittal.

Thank you for your cooperation.

Well, the aliasing test was added as an optimization, but dropping it and doing the flush on any PG_dcache_dirty hit is fine too. The exec case is a bit misleading as the purge in question only touches the D-cache lines, whilst the VM_EXEC case needs I-cache tidying. This is further complicated by the fact that I-cache tidying may be required regardless of whether there is a page mapping or not. However, that's a largely secondary issue. If the crashes observed go away with more aggressive D-cache flushing, then it's not likely to be exec related given that the I-cache remains unaltered.

So, I'm inclined to just apply Markus's original patch that drops the pages_do_alias() test and restores the old behaviour of just aggressively flushing in the PG_dcache_dirty case. If there's a reproduceable testcase for circumventing the aliasing test then obviously that's worth looking in to for future optimizations.

Hi Matt,

With exec = 0, the cache issues are still present.

The bug happens mostly on graphic applications like ts_test from the touch library libts or any Qt based application. There is a way to increase the probability to appear to almost 100% on my HiCO.DIMM7723 board when using nfsroot.

When booting by network, a lot of shared libraries have already been loaded before I can launch ts_test. So their pages might already been synchronized in the instruction/data cache. But if I mount the rootfs a second time and chroot into it and then start the application, it always crashes. The next calls might crash, but the third and later calls always works. To see the crash a second time I need to unmount the filesystem and mount it again.

BusyBox v1.12.4 (2009-12-09 12:43:33 CET) built-in shell (ash) Enter 'help' for a list of built-in commands.

~ # mount -o nolock 192.168.2.11:/nfsroot/hico7723/rootfs /mnt
~ # chroot /mnt
~ # mount -a; mdev -s; . /etc/profile._tslib
BusyBox v1.12.4 (2009-12-09 12:43:33 CET) built-in shell (ash)
Enter 'help' for a list of built-in commands.
~ # ts_test
Segmentation fault
~ # ts_test
Illegal instruction
~ # ts_test
^Csignal 2 caught
~ # exit
~ # umount /mnt/proc; umount /mnt/sys; umount /mnt/tmp; umount /mnt/mnt; umount
/mnt/var/run; umount /mnt/var/log; umount /mnt
~ # mount -o nolock 192.168.2.11:/nfsroot/hico7723/rootfs /mnt
~ # chroot /mnt
~ # mount -a; mdev -s; . /etc/profile._tslib
BusyBox v1.12.4 (2009-12-09 12:43:33 CET) built-in shell (ash)
Enter 'help' for a list of built-in commands.
~ # ts_test
Inconsistency detected by ld.so: dynamic-link.h: 165: elf_get_dynamic_info: Assertion `info[9]->d_un.d_val = sizeof (Elf32_Rela)' failed! ~ # ts_test
^Csignal 2 caught
~ #

The loss might not be as big as having to use the cache in write-through mode ;-)

_____________________________________

Amtsgericht Mannheim
HRB 110 300
Gesch?ftsf?hrer: Dieter Baur, Ramona Maurer
_____________________________________

Important Note:
- This e-mail may contain trade secrets or privileged, undisclosed or otherwise confidential information.
- If you have received this e-mail in error, you are hereby notified that any review, copying or distribution of it is strictly prohibited.
- Please inform us immediately and destroy the original transmittal.

Thank you for your cooperation.

I pushed out a reworked version of your patch, so give current git a try and see if that fixes up your remaining issues. These changes will be reflected in 2.6.33-rc2.