Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c
From: Hugh Dickins <hidden>
Date: 2007-05-22 17:04:54
Also in:
linux-mm, sparclinux
On Tue, 22 May 2007, Benjamin Herrenschmidt wrote:
This patch reworks ptep_set_access_flags() and the callers so that the comparison to the old PTE is done inside that function, which then returns wether an update_mmu_cache() is needed. That allows fixing the sun4c situation where update_mmu_cache() needs to be forced, always. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- Ok, so that's only compile tested on sparc32 and powerpc 32 bits, boot tested on powerpc64 and not tested on others (I could use some help testing x86, x86_64 and s390 who also have their own implementations).
Looks pretty good to me. There was a minor build error in x86 (see below), and ia64 is missing (again see below). I've now built and am running this on x86, x86_64 and powerpc64; but I'm very unlikely to be doing anything which actually tickles these changes, or Andrea's original handle_pte_fault optimization. Would the "__changed && __dirty" architectures (x86, x86_64, ia64) be better off saying __changed = __dirty && pte_same? I doubt it's worth bothering about. You've updated do_wp_page to do "if (ptep_set_access_flags(...", but not updated set_huge_ptep_writable in the same way: I'd have thought you'd either leave both alone, or update them both: any reason for one not the other? But again, not really an issue. These changes came about because the sun4c needs to update_mmu_cache even in the pte_same case: might it also need to flush_tlb_page then?
quoted hunk ↗ jump to hunk
--- linux-work.orig/include/asm-generic/pgtable.h 2007-05-22 15:04:45.000000000 +1000 +++ linux-work/include/asm-generic/pgtable.h 2007-05-22 15:32:21.000000000 +1000@@ -27,13 +27,20 @@ do { \ * Largely same as above, but only sets the access flags (dirty, * accessed, and writable). Furthermore, we know it always gets set * to a "more permissive" setting, which allows most architectures - * to optimize this. + * to optimize this. We return wether the PTE actually changed, which
whether
+ * in turn instructs the caller to do things like update__mmu_cache.
update_mmu_cache.
+ * This used to be done in the caller, but sparc needs minor faults to + * force that call on sun4c so we changed this macro slightly */
quoted hunk ↗ jump to hunk
--- linux-work.orig/include/asm-i386/pgtable.h 2007-05-22 15:06:17.000000000 +1000 +++ linux-work/include/asm-i386/pgtable.h 2007-05-22 15:16:11.000000000 +1000@@ -285,13 +285,15 @@ static inline pte_t native_local_ptep_ge */ #define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS #define ptep_set_access_flags(vma, address, ptep, entry, dirty) \ -do { \ - if (dirty) { \ +({ \ + int __changed = !pte_same(*(__ptep), __entry); \
That just needs to be: + int __changed = !pte_same(*(ptep), entry); \
+ if (__changed && dirty) { \
(ptep)->pte_low = (entry).pte_low; \
pte_update_defer((vma)->vm_mm, (address), (ptep)); \
flush_tlb_page(vma, address); \
} \
-} while (0)
+ __changed; \
+})Here's what I think the ia64 hunk would be, unbuilt and untested.
--- linux-work.orig/include/asm-ia64/pgtable.h 2007-05-13 05:41:00.000000000 +0100
+++ linux-work/include/asm-ia64/pgtable.h 2007-05-22 17:33:58.000000000 +0100@@ -533,16 +533,23 @@ extern void lazy_mmu_prot_update (pte_t * daccess_bit in ivt.S). */ #ifdef CONFIG_SMP -# define ptep_set_access_flags(__vma, __addr, __ptep, __entry, __safely_writable) \ -do { \ - if (__safely_writable) { \ - set_pte(__ptep, __entry); \ - flush_tlb_page(__vma, __addr); \ - } \ -} while (0) +# define ptep_set_access_flags(__vma, __addr, __ptep, __entry, __safely_writable) \ +({ \ + int __changed = !pte_same(*(__ptep), __entry); \ + if (__changed && __safely_writable) { \ + set_pte(__ptep, __entry); \ + flush_tlb_page(__vma, __addr); \ + } \ + __changed; \ +}) #else -# define ptep_set_access_flags(__vma, __addr, __ptep, __entry, __safely_writable) \ - ptep_establish(__vma, __addr, __ptep, __entry) +# define ptep_set_access_flags(__vma, __addr, __ptep, __entry, __safely_writable) \ +({ \ + int __changed = !pte_same(*(__ptep), __entry); \ + if (__changed) \ + ptep_establish(__vma, __addr, __ptep, __entry); \ + __changed; \ +}) #endif # ifdef CONFIG_VIRTUAL_MEM_MAP