Re: vm changes from linux-2.6.14 to linux-2.6.15

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Mark Fortescue]

Hi all,

I have tracked down a failure to successfully load/run the init task on my 
Sparcstation 1 clone (SS1) and Sparcstation 2 (SS2), sparc32 sun4c 
systems, to a patch:

   commit 1a44e149084d772a1bcf4cdbdde8a013a8a1cfde.
   [PATCH] .text page fault SMP scalability optimization

Removing this patch fixes the issue and allows me to use kernels later 
than v2.5.14. (tested using linux-2.6.20.9).

Given the comment provided by the git bisect, backing out this patch will 
probably have undesirable conseqnences for other platforms (especially 
powerpc64) so, if an architecture independent solution is not available, 
some/all of the code in handle_pte_fault() in mm/memory.c will need be to 
made architecture dependent.

I am not sufficiently familear with the how the SS1/SS2 mmu works and how 
the linux memory management system works to understand why this patch 
prevents my sun4c SS1/SS2 systems from working.

Advice and help on the approch to take and any code changes regarding this 
issue would be most welcome.

Regards
 	Mark Fortescue.

On Sun, 29 Apr 2007, Tom "spot" Callaway wrote:

> On Sun, 2007-04-29 at 14:02 +0100, Mark Fortescue wrote:
>> Hi,
>>
>> Does any one have or know of a good reference that would help me in
>> identifing the cause of a CPU soft lockup that apears to be related to
>> changes in the vertual memory handling between these two versions of the
>> kernel?
>
> Have you tried git bisecting between the two kernels to find the
> specific patch that breaks?
>
> ~spot
>
>

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Andrew Morton]

On Mon, 30 Apr 2007 22:36:27 +0100 (BST)
Mark Fortescue <mark@mtfhpc.demon.co.uk> wrote:

> Hi all,
> 
> I have tracked down a failure to successfully load/run the init task on my 
> Sparcstation 1 clone (SS1) and Sparcstation 2 (SS2), sparc32 sun4c 
> systems, to a patch:
> 
>    commit 1a44e149084d772a1bcf4cdbdde8a013a8a1cfde.
>    [PATCH] .text page fault SMP scalability optimization
> 
> Removing this patch fixes the issue and allows me to use kernels later 
> than v2.5.14. (tested using linux-2.6.20.9).
> 
> Given the comment provided by the git bisect, backing out this patch will 
> probably have undesirable conseqnences for other platforms (especially 
> powerpc64) so, if an architecture independent solution is not available, 
> some/all of the code in handle_pte_fault() in mm/memory.c will need be to 
> made architecture dependent.
> 
> I am not sufficiently familear with the how the SS1/SS2 mmu works and how 
> the linux memory management system works to understand why this patch 
> prevents my sun4c SS1/SS2 systems from working.
> 
> Advice and help on the approch to take and any code changes regarding this 
> issue would be most welcome.
> 

Interesting - thanks for working that out.  Let's keep linux-mm on cc please.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Mon, 30 Apr 2007 14:54:14 -0700

> On Mon, 30 Apr 2007 22:36:27 +0100 (BST)
> Mark Fortescue <mark@mtfhpc.demon.co.uk> wrote:
> 
> > Hi all,
> > 
> > I have tracked down a failure to successfully load/run the init task on my 
> > Sparcstation 1 clone (SS1) and Sparcstation 2 (SS2), sparc32 sun4c 
> > systems, to a patch:
> > 
> >    commit 1a44e149084d772a1bcf4cdbdde8a013a8a1cfde.
> >    [PATCH] .text page fault SMP scalability optimization
> > 
> > Removing this patch fixes the issue and allows me to use kernels later 
> > than v2.5.14. (tested using linux-2.6.20.9).
> > 
> > Given the comment provided by the git bisect, backing out this patch will 
> > probably have undesirable conseqnences for other platforms (especially 
> > powerpc64) so, if an architecture independent solution is not available, 
> > some/all of the code in handle_pte_fault() in mm/memory.c will need be to 
> > made architecture dependent.
> > 
> > I am not sufficiently familear with the how the SS1/SS2 mmu works and how 
> > the linux memory management system works to understand why this patch 
> > prevents my sun4c SS1/SS2 systems from working.
> > 
> > Advice and help on the approch to take and any code changes regarding this 
> > issue would be most welcome.
> > 
> 
> Interesting - thanks for working that out.  Let's keep linux-mm on cc please.

You can't elide the update_mmu_cache() call on sun4c because that will
miss some critical TLB setups which are performed there.

The sun4c TLB has two tiers of entries:

1) segment maps, these hold ptes for a range of addresses
2) ptes, mapped into segment maps

update_mmu_cache() on sun4c take care of allocating and setting
up the segment maps, so if you elide the call this never happens
and we fault forever.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Mark Fortescue]

Hi David,

Is this just sun4c or does it affect other sparc32 architectures.

Regards
 	Mark Fortescue.

On Mon, 30 Apr 2007, David Miller wrote:

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Mon, 30 Apr 2007 14:54:14 -0700
>
>> On Mon, 30 Apr 2007 22:36:27 +0100 (BST)
>> Mark Fortescue <mark@mtfhpc.demon.co.uk> wrote:
>>
>>> Hi all,
>>>
>>> I have tracked down a failure to successfully load/run the init task on my
>>> Sparcstation 1 clone (SS1) and Sparcstation 2 (SS2), sparc32 sun4c
>>> systems, to a patch:
>>>
>>>    commit 1a44e149084d772a1bcf4cdbdde8a013a8a1cfde.
>>>    [PATCH] .text page fault SMP scalability optimization
>>>
>>> Removing this patch fixes the issue and allows me to use kernels later
>>> than v2.5.14. (tested using linux-2.6.20.9).
>>>
>>> Given the comment provided by the git bisect, backing out this patch will
>>> probably have undesirable conseqnences for other platforms (especially
>>> powerpc64) so, if an architecture independent solution is not available,
>>> some/all of the code in handle_pte_fault() in mm/memory.c will need be to
>>> made architecture dependent.
>>>
>>> I am not sufficiently familear with the how the SS1/SS2 mmu works and how
>>> the linux memory management system works to understand why this patch
>>> prevents my sun4c SS1/SS2 systems from working.
>>>
>>> Advice and help on the approch to take and any code changes regarding this
>>> issue would be most welcome.
>>>
>>
>> Interesting - thanks for working that out.  Let's keep linux-mm on cc please.
>
> You can't elide the update_mmu_cache() call on sun4c because that will
> miss some critical TLB setups which are performed there.
>
> The sun4c TLB has two tiers of entries:
>
> 1) segment maps, these hold ptes for a range of addresses
> 2) ptes, mapped into segment maps
>
> update_mmu_cache() on sun4c take care of allocating and setting
> up the segment maps, so if you elide the call this never happens
> and we fault forever.
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: vm changes from linux-2.6.14 to linux-2.6.15

[David Miller]

From: Mark Fortescue <mark@mtfhpc.demon.co.uk>
Date: Mon, 30 Apr 2007 23:33:13 +0100 (BST)

> Is this just sun4c or does it affect other sparc32 architectures.

Only sun4c.

srmmu's update_mmu_cache() is basically a NOP.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Benjamin Herrenschmidt]

> > Interesting - thanks for working that out.  Let's keep linux-mm on cc please.
> 
> You can't elide the update_mmu_cache() call on sun4c because that will
> miss some critical TLB setups which are performed there.
> 
> The sun4c TLB has two tiers of entries:
> 
> 1) segment maps, these hold ptes for a range of addresses
> 2) ptes, mapped into segment maps
> 
> update_mmu_cache() on sun4c take care of allocating and setting
> up the segment maps, so if you elide the call this never happens
> and we fault forever.

Maybe we can move that logic to ptep_set_access_flags()... in fact, the
tlb flush logic should be done there too imho.

There would still be the update_mmu_cache() that we don't want on
powerpc in all cases I suppose. That can be done by having
ptep_set_access_flags() return a boolean indicating wether
update_mmu_cache() shall be called or not ...

Ben.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Tue, 01 May 2007 10:00:19 +1000

> 
> > > Interesting - thanks for working that out.  Let's keep linux-mm on cc please.
> > 
> > You can't elide the update_mmu_cache() call on sun4c because that will
> > miss some critical TLB setups which are performed there.
> > 
> > The sun4c TLB has two tiers of entries:
> > 
> > 1) segment maps, these hold ptes for a range of addresses
> > 2) ptes, mapped into segment maps
> > 
> > update_mmu_cache() on sun4c take care of allocating and setting
> > up the segment maps, so if you elide the call this never happens
> > and we fault forever.
> 
> Maybe we can move that logic to ptep_set_access_flags()... in fact, the
> tlb flush logic should be done there too imho.
>
> There would still be the update_mmu_cache() that we don't want on
> powerpc in all cases I suppose. That can be done by having
> ptep_set_access_flags() return a boolean indicating wether
> update_mmu_cache() shall be called or not ...

Always doing ptep_set_access_flags() and returning a boolean like
that might be a good idea.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Mark Fortescue]

On Mon, 30 Apr 2007, David Miller wrote:

> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Tue, 01 May 2007 10:00:19 +1000
>
>>
>>>> Interesting - thanks for working that out.  Let's keep linux-mm on cc please.
>>>
>>> You can't elide the update_mmu_cache() call on sun4c because that will
>>> miss some critical TLB setups which are performed there.
>>>
>>> The sun4c TLB has two tiers of entries:
>>>
>>> 1) segment maps, these hold ptes for a range of addresses
>>> 2) ptes, mapped into segment maps
>>>
>>> update_mmu_cache() on sun4c take care of allocating and setting
>>> up the segment maps, so if you elide the call this never happens
>>> and we fault forever.
>>
>> Maybe we can move that logic to ptep_set_access_flags()... in fact, the
>> tlb flush logic should be done there too imho.
>>
>> There would still be the update_mmu_cache() that we don't want on
>> powerpc in all cases I suppose. That can be done by having
>> ptep_set_access_flags() return a boolean indicating wether
>> update_mmu_cache() shall be called or not ...
>
> Always doing ptep_set_access_flags() and returning a boolean like
> that might be a good idea.

At present, update_mmu_cache() and lazy_mmu_prot_update() are always 
called when ptep_set_access_flags() is called so why not move them into 
ptep_set_access_flags() and change ptep_set_access_flags() to have an 
additional boolean parameter (__update) that would when set, cause 
update_mmu_cache() and lazy_mmu_prot_update() to be called.

On sun4c, an architecture specific function would be installed that 
always treats the __update parameter as set at all times.

The generic function would change to somthing along the lines:

#define ptep_set_access_flags(__vma, __address, \
                               __ptep, __entry, __dirty, __update)
do { \
    if (__update) { \
        set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry); \
        flush_tlb_page(__vma, __address); \
        update_mmu_cache(__vma, __address, __entry); \
        lazy_mmu_prot_update(__entry); \
    } else if (__dirty) {
        flush_tlb_page(__vma, __address); \
    } \
} while (0)

The code in mm/memory.c and mm/hugetlb.c and the architecture specific 
versions of ptep_set_access_flags would then be changed acordingly.

Regards
 	Mark Fortescue.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Benjamin Herrenschmidt]

> At present, update_mmu_cache() and lazy_mmu_prot_update() are always 
> called when ptep_set_access_flags() is called so why not move them into 
> ptep_set_access_flags() and change ptep_set_access_flags() to have an 
> additional boolean parameter (__update) that would when set, cause 
> update_mmu_cache() and lazy_mmu_prot_update() to be called.

Well, ptep_set_access_flags() is a low level arch hook, I'd rather not
start hiding update_mmu_cache() calls in it ...

Ben.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Mark Fortescue]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 772 bytes --]



On Tue, 1 May 2007, Benjamin Herrenschmidt wrote:

>
>> At present, update_mmu_cache() and lazy_mmu_prot_update() are always
>> called when ptep_set_access_flags() is called so why not move them into
>> ptep_set_access_flags() and change ptep_set_access_flags() to have an
>> additional boolean parameter (__update) that would when set, cause
>> update_mmu_cache() and lazy_mmu_prot_update() to be called.
>
> Well, ptep_set_access_flags() is a low level arch hook, I'd rather not
> start hiding update_mmu_cache() calls in it ...
>
> Ben.
>
>

I have attached a patch (so pine does not mangle it) for linux-2.6.20.9.
Is this what you had in mind?

For linux-2.6.21, more work will be needed as it has more code calling 
ptep_set_access_flags.

Regards
 	Mark Fortescue.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 10987 bytes --]

diff -ruNpd linux-2.6.20.9/include/asm-generic/pgtable.h linux-test/include/asm-generic/pgtable.h
--- linux-2.6.20.9/include/asm-generic/pgtable.h	2007-05-01 12:57:56.000000000 +0100
+++ linux-test/include/asm-generic/pgtable.h	2007-05-01 14:20:05.000000000 +0100
@@ -29,11 +29,16 @@ do {				  					\
  * to a "more permissive" setting, which allows most architectures
  * to optimize this.
  */
-#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-do {				  					  \
-	set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);	  \
-	flush_tlb_page(__vma, __address);				  \
-} while (0)
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty, __update) \
+({										    \
+	if (__update) {				  				    \
+		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);	    \
+		flush_tlb_page(__vma, __address);				    \
+	} else if (__dirty) {							    \
+		flush_tlb_page(__vma, __address);				    \
+	}									    \
+	__update;								    \
+})
 #endif
 
 #ifndef __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
diff -ruNpd linux-2.6.20.9/include/asm-i386/pgtable.h linux-test/include/asm-i386/pgtable.h
--- linux-2.6.20.9/include/asm-i386/pgtable.h	2007-05-01 12:57:56.000000000 +0100
+++ linux-test/include/asm-i386/pgtable.h	2007-05-01 13:43:38.000000000 +0100
@@ -273,14 +273,17 @@ static inline pte_t pte_mkhuge(pte_t pte
  * bit at the same time.
  */
 #define  __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
-#define ptep_set_access_flags(vma, address, ptep, entry, dirty)		\
-do {									\
-	if (dirty) {							\
-		(ptep)->pte_low = (entry).pte_low;			\
-		pte_update_defer((vma)->vm_mm, (address), (ptep));	\
-		flush_tlb_page(vma, address);				\
-	}								\
-} while (0)
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty, __update)	\
+({											\
+	if (__dirty) {									\
+		if (__update) {								\
+			(__ptep)->pte_low = (__entry).pte_low;				\
+			pte_update_defer((__vma)->vm_mm, (__address), (__ptep));	\
+		}									\
+		flush_tlb_page(__vma, __address);					\
+	}										\
+	__update;									\
+})
 
 /*
  * We don't actually have these, but we want to advertise them so that
diff -ruNpd linux-2.6.20.9/include/asm-ia64/pgtable.h linux-test/include/asm-ia64/pgtable.h
--- linux-2.6.20.9/include/asm-ia64/pgtable.h	2007-04-30 19:04:59.000000000 +0100
+++ linux-test/include/asm-ia64/pgtable.h	2007-05-01 13:48:07.000000000 +0100
@@ -537,16 +537,25 @@ 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);						\
+# define ptep_set_access_flags(__vma, __addr, __ptep, __entry, __dirty, __update)	\
+({											\
+	if (__dirty) {									\
+		if (__update)								\
+			set_pte(__ptep, __entry);					\
 		flush_tlb_page(__vma, __addr);						\
 	}										\
-} while (0)
+	__update;									\
+})
 #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, __dirty, __update)	\
+({											\
+	if (__update)									\
+		ptep_establish(__vma, __addr, __ptep, __entry);				\
+	else if (__dirty)								\
+		flush_tlb_page(__vma, __addr);						\
+	}										\
+	__update;									\
+})
 #endif
 
 #  ifdef CONFIG_VIRTUAL_MEM_MAP
diff -ruNpd linux-2.6.20.9/include/asm-powerpc/pgtable.h linux-test/include/asm-powerpc/pgtable.h
--- linux-2.6.20.9/include/asm-powerpc/pgtable.h	2007-05-01 12:57:56.000000000 +0100
+++ linux-test/include/asm-powerpc/pgtable.h	2007-05-01 13:58:56.000000000 +0100
@@ -437,11 +437,15 @@ static inline void __ptep_set_access_fla
 	:"r" (bits), "r" (ptep), "m" (*ptep), "i" (_PAGE_BUSY)
 	:"cc");
 }
-#define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								   \
-		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
-		flush_tlb_page_nohash(__vma, __address);	       	   \
-	} while(0)
+#define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty, __update) \
+({										     \
+	if (__update) {								     \
+		__ptep_set_access_flags(__ptep, __entry, __dirty);		     \
+		flush_tlb_page_nohash(__vma, __address);			     \
+	} else if (__dirty)							     \
+		flush_tlb_page(__vma, __address);				     \
+	__update;								     \
+})
 
 /*
  * Macro to mark a page protection value as "uncacheable".
diff -ruNpd linux-2.6.20.9/include/asm-ppc/pgtable.h linux-test/include/asm-ppc/pgtable.h
--- linux-2.6.20.9/include/asm-ppc/pgtable.h	2007-04-30 19:05:00.000000000 +0100
+++ linux-test/include/asm-ppc/pgtable.h	2007-05-01 13:50:30.000000000 +0100
@@ -693,11 +693,15 @@ static inline void __ptep_set_access_fla
 	pte_update(ptep, 0, bits);
 }
 
-#define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								   \
-		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
-		flush_tlb_page_nohash(__vma, __address);	       	   \
-	} while(0)
+#define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty, __update) \
+({										     \
+	if (__update) {								     \
+		__ptep_set_access_flags(__ptep, __entry, __dirty);		     \
+		flush_tlb_page_nohash(__vma, __address);			     \
+	} else if (__dirty)							     \
+		flush_tlb_page(__vma, __address);				     \
+	__update;								     \
+})
 
 /*
  * Macro to mark a page protection value as "uncacheable".
diff -ruNpd linux-2.6.20.9/include/asm-s390/pgtable.h linux-test/include/asm-s390/pgtable.h
--- linux-2.6.20.9/include/asm-s390/pgtable.h	2007-05-01 12:57:56.000000000 +0100
+++ linux-test/include/asm-s390/pgtable.h	2007-05-01 14:23:24.000000000 +0100
@@ -628,8 +628,14 @@ ptep_establish(struct vm_area_struct *vm
 	set_pte(ptep, entry);
 }
 
-#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	ptep_establish(__vma, __address, __ptep, __entry)
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty, __update) \
+	({									    \
+		if (__update)							    \
+			ptep_establish(__vma, __address, __ptep, __entry);	    \
+		else if (__dirty)						    \
+			flush_tlb_page(__vma, __address);			    \
+		__update;							    \
+	})
 
 /*
  * Test and clear dirty bit in storage key.
diff -ruNpd linux-2.6.20.9/include/asm-sparc/pgtable.h linux-test/include/asm-sparc/pgtable.h
--- linux-2.6.20.9/include/asm-sparc/pgtable.h	2007-04-30 19:05:01.000000000 +0100
+++ linux-test/include/asm-sparc/pgtable.h	2007-05-01 14:32:35.000000000 +0100
@@ -446,6 +446,27 @@ extern int io_remap_pfn_range(struct vm_
 #define GET_IOSPACE(pfn)		(pfn >> (BITS_PER_LONG - 4))
 #define GET_PFN(pfn)			(pfn & 0x0fffffffUL)
 
+#define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty, __update) \
+({										    \
+	int __ret;								    \
+										    \
+	if (sparc_cpu_model == sun4c) {						    \
+		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);	    \
+		flush_tlb_page(__vma, __address);				    \
+		__ret = 1;							    \
+	} else {								    \
+		if (__update) {			  				    \
+			set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);   \
+			flush_tlb_page(__vma, __address);			    \
+		} else if (__dirty) {						    \
+			flush_tlb_page(__vma, __address);			    \
+		}								    \
+		__ret = __update;						    \
+	}									    \
+	__ret;									    \
+})
+
 #include <asm-generic/pgtable.h>
 
 #endif /* !(__ASSEMBLY__) */
diff -ruNpd linux-2.6.20.9/include/asm-x86_64/pgtable.h linux-test/include/asm-x86_64/pgtable.h
--- linux-2.6.20.9/include/asm-x86_64/pgtable.h	2007-05-01 12:57:57.000000000 +0100
+++ linux-test/include/asm-x86_64/pgtable.h	2007-05-01 13:37:08.000000000 +0100
@@ -396,13 +396,15 @@ static inline pte_t pte_modify(pte_t pte
  * race with other CPU's that might be updating the dirty
  * bit at the same time. */
 #define  __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
-#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								  \
-		if (__dirty) {						  \
-			set_pte(__ptep, __entry);			  \
-			flush_tlb_page(__vma, __address);		  \
-		}							  \
-	} while (0)
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty, __update) \
+	({									    \
+		if (__dirty) {							    \
+			if (__update)						    \
+				set_pte(__ptep, __entry);			    \
+			flush_tlb_page(__vma, __address);			    \
+		}								    \
+		__update;							    \
+	})
 
 /* Encode and de-code a swap entry */
 #define __swp_type(x)			(((x).val >> 1) & 0x3f)
diff -ruNpd linux-2.6.20.9/mm/hugetlb.c linux-test/mm/hugetlb.c
--- linux-2.6.20.9/mm/hugetlb.c	2007-05-01 13:01:10.000000000 +0100
+++ linux-test/mm/hugetlb.c	2007-05-01 13:17:26.000000000 +0100
@@ -313,9 +313,10 @@ static void set_huge_ptep_writable(struc
 	pte_t entry;
 
 	entry = pte_mkwrite(pte_mkdirty(*ptep));
-	ptep_set_access_flags(vma, address, ptep, entry, 1);
-	update_mmu_cache(vma, address, entry);
-	lazy_mmu_prot_update(entry);
+	if (ptep_set_access_flags(vma, address, ptep, entry, 1, 1)) {
+		update_mmu_cache(vma, address, entry);
+		lazy_mmu_prot_update(entry);
+	}
 }
 
 
diff -ruNpd linux-2.6.20.9/mm/memory.c linux-test/mm/memory.c
--- linux-2.6.20.9/mm/memory.c	2007-05-01 12:57:57.000000000 +0100
+++ linux-test/mm/memory.c	2007-05-01 14:26:38.000000000 +0100
@@ -1553,9 +1553,11 @@ static int do_wp_page(struct mm_struct *
 		flush_cache_page(vma, address, pte_pfn(orig_pte));
 		entry = pte_mkyoung(orig_pte);
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
-		ptep_set_access_flags(vma, address, page_table, entry, 1);
-		update_mmu_cache(vma, address, entry);
-		lazy_mmu_prot_update(entry);
+		if (ptep_set_access_flags(vma, address, page_table,
+					  entry, 1, 1)) {
+			update_mmu_cache(vma, address, entry);
+			lazy_mmu_prot_update(entry);
+		}
 		ret |= VM_FAULT_WRITE;
 		goto unlock;
 	}
@@ -2423,19 +2425,10 @@ static inline int handle_pte_fault(struc
 		entry = pte_mkdirty(entry);
 	}
 	entry = pte_mkyoung(entry);
-	if (!pte_same(old_entry, entry)) {
-		ptep_set_access_flags(vma, address, pte, entry, write_access);
+	if (ptep_set_access_flags(vma, address, pte, entry, write_access,
+				  !pte_same(old_entry, entry))) {
 		update_mmu_cache(vma, address, entry);
 		lazy_mmu_prot_update(entry);
-	} else {
-		/*
-		 * This is needed only for protection faults but the arch code
-		 * is not yet telling us if this is a protection fault or not.
-		 * This still avoids useless tlb flushes for .text page faults
-		 * with threads.
-		 */
-		if (write_access)
-			flush_tlb_page(vma, address);
 	}
 unlock:
 	pte_unmap_unlock(pte, ptl);

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Benjamin Herrenschmidt]

> I have attached a patch (so pine does not mangle it) for linux-2.6.20.9.
> Is this what you had in mind?
> 
> For linux-2.6.21, more work will be needed as it has more code calling 
> ptep_set_access_flags.

I'm not 100% sure we need the 'update' argument... we can remove the
whole old_entry, pte_same, etc... and just have pte_set_access_flags()
read the old PTE and decide wether something needs to be changed or not.

Ben.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Mark Fortescue]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 794 bytes --]



On Wed, 2 May 2007, Benjamin Herrenschmidt wrote:

>
>> I have attached a patch (so pine does not mangle it) for linux-2.6.20.9.
>> Is this what you had in mind?
>>
>> For linux-2.6.21, more work will be needed as it has more code calling
>> ptep_set_access_flags.
>
> I'm not 100% sure we need the 'update' argument... we can remove the
> whole old_entry, pte_same, etc... and just have pte_set_access_flags()
> read the old PTE and decide wether something needs to be changed or not.
>
> Ben.
>
>

The attached patch works on sun4c (with my simple ADA compile test) but 
the change in functionality may break things other platforms.

The advantage of the previous patch is that the functionality is only 
changed for sparc sun4c so less testing would be required.

Regards
 	Mark Fortescue.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 10932 bytes --]

diff -ruNpd linux-2.6.20.9/include/asm-generic/pgtable.h linux-test/include/asm-generic/pgtable.h
--- linux-2.6.20.9/include/asm-generic/pgtable.h	2007-05-01 12:57:56.000000000 +0100
+++ linux-test/include/asm-generic/pgtable.h	2007-05-01 23:13:23.000000000 +0100
@@ -30,10 +30,17 @@ do {				  					\
  * to optimize this.
  */
 #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-do {				  					  \
-	set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);	  \
-	flush_tlb_page(__vma, __address);				  \
-} while (0)
+({									  \
+	int __update = !pte_same(*(__ptep), __entry);			  \
+									  \
+	if (__update) {				  			  \
+		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry); \
+		flush_tlb_page(__vma, __address);			  \
+	} else if (__dirty) {						  \
+		flush_tlb_page(__vma, __address);			  \
+	}								  \
+	__update;							  \
+})
 #endif
 
 #ifndef __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
diff -ruNpd linux-2.6.20.9/include/asm-i386/pgtable.h linux-test/include/asm-i386/pgtable.h
--- linux-2.6.20.9/include/asm-i386/pgtable.h	2007-05-01 12:57:56.000000000 +0100
+++ linux-test/include/asm-i386/pgtable.h	2007-05-01 23:18:50.000000000 +0100
@@ -273,14 +273,19 @@ static inline pte_t pte_mkhuge(pte_t pte
  * bit at the same time.
  */
 #define  __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
-#define ptep_set_access_flags(vma, address, ptep, entry, dirty)		\
-do {									\
-	if (dirty) {							\
-		(ptep)->pte_low = (entry).pte_low;			\
-		pte_update_defer((vma)->vm_mm, (address), (ptep));	\
-		flush_tlb_page(vma, address);				\
-	}								\
-} while (0)
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty)	 \
+({										 \
+	int __update = !pte_same(*(__ptep), __entry);				 \
+										 \
+	if (__dirty) {								 \
+		if (__update) {							 \
+			(__ptep)->pte_low = (__entry).pte_low;			 \
+			pte_update_defer((__vma)->vm_mm, (__address), (__ptep)); \
+		}								 \
+		flush_tlb_page(__vma, __address);				 \
+	}									 \
+	__update;								 \
+})
 
 /*
  * We don't actually have these, but we want to advertise them so that
diff -ruNpd linux-2.6.20.9/include/asm-ia64/pgtable.h linux-test/include/asm-ia64/pgtable.h
--- linux-2.6.20.9/include/asm-ia64/pgtable.h	2007-04-30 19:04:59.000000000 +0100
+++ linux-test/include/asm-ia64/pgtable.h	2007-05-01 23:16:47.000000000 +0100
@@ -537,16 +537,29 @@ 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, __dirty)	\
+({									\
+	int __update = !pte_same(*(__ptep), __entry);			\
+									\
+	if (__dirty) {							\
+		if (__update)						\
+			set_pte(__ptep, __entry);			\
+		flush_tlb_page(__vma, __addr);				\
+	}								\
+	__update;							\
+})
 #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, __dirty)	\
+({									\
+	int __update = !pte_same(*(__ptep), __entry);			\
+									\
+	if (__update)							\
+		ptep_establish(__vma, __addr, __ptep, __entry);		\
+	else if (__dirty)						\
+		flush_tlb_page(__vma, __addr);				\
+	}								\
+	__update;							\
+})
 #endif
 
 #  ifdef CONFIG_VIRTUAL_MEM_MAP
diff -ruNpd linux-2.6.20.9/include/asm-powerpc/pgtable.h linux-test/include/asm-powerpc/pgtable.h
--- linux-2.6.20.9/include/asm-powerpc/pgtable.h	2007-05-01 12:57:56.000000000 +0100
+++ linux-test/include/asm-powerpc/pgtable.h	2007-05-01 23:21:18.000000000 +0100
@@ -438,10 +438,16 @@ static inline void __ptep_set_access_fla
 	:"cc");
 }
 #define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								   \
+({									   \
+	int __update = !pte_same(*(__ptep), __entry);			   \
+									   \
+	if (__update) {							   \
 		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
-		flush_tlb_page_nohash(__vma, __address);	       	   \
-	} while(0)
+		flush_tlb_page_nohash(__vma, __address);		   \
+	} else if (__dirty)						   \
+		flush_tlb_page(__vma, __address);			   \
+	__update;							   \
+})
 
 /*
  * Macro to mark a page protection value as "uncacheable".
diff -ruNpd linux-2.6.20.9/include/asm-ppc/pgtable.h linux-test/include/asm-ppc/pgtable.h
--- linux-2.6.20.9/include/asm-ppc/pgtable.h	2007-04-30 19:05:00.000000000 +0100
+++ linux-test/include/asm-ppc/pgtable.h	2007-05-01 23:15:29.000000000 +0100
@@ -694,10 +694,16 @@ static inline void __ptep_set_access_fla
 }
 
 #define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								   \
+({									   \
+	int __update = !pte_same(*(__ptep), __entry);			   \
+									   \
+	if (__update) {							   \
 		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
-		flush_tlb_page_nohash(__vma, __address);	       	   \
-	} while(0)
+		flush_tlb_page_nohash(__vma, __address);		   \
+	} else if (__dirty)						   \
+		flush_tlb_page(__vma, __address);			   \
+	__update;							   \
+})
 
 /*
  * Macro to mark a page protection value as "uncacheable".
diff -ruNpd linux-2.6.20.9/include/asm-s390/pgtable.h linux-test/include/asm-s390/pgtable.h
--- linux-2.6.20.9/include/asm-s390/pgtable.h	2007-05-01 12:57:56.000000000 +0100
+++ linux-test/include/asm-s390/pgtable.h	2007-05-01 23:19:58.000000000 +0100
@@ -628,8 +628,16 @@ ptep_establish(struct vm_area_struct *vm
 	set_pte(ptep, entry);
 }
 
-#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	ptep_establish(__vma, __address, __ptep, __entry)
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty)  \
+	({								   \
+		int __update = !pte_same(*(__ptep), __entry);		   \
+									   \
+		if (__update)						   \
+			ptep_establish(__vma, __address, __ptep, __entry); \
+		else if (__dirty)					   \
+			flush_tlb_page(__vma, __address);		   \
+		__update;						   \
+	})
 
 /*
  * Test and clear dirty bit in storage key.
diff -ruNpd linux-2.6.20.9/include/asm-sparc/pgtable.h linux-test/include/asm-sparc/pgtable.h
--- linux-2.6.20.9/include/asm-sparc/pgtable.h	2007-04-30 19:05:01.000000000 +0100
+++ linux-test/include/asm-sparc/pgtable.h	2007-05-01 23:10:49.000000000 +0100
@@ -446,6 +446,27 @@ extern int io_remap_pfn_range(struct vm_
 #define GET_IOSPACE(pfn)		(pfn >> (BITS_PER_LONG - 4))
 #define GET_PFN(pfn)			(pfn & 0x0fffffffUL)
 
+#define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
+({										    \
+	int __update;								    \
+										    \
+	if (sparc_cpu_model == sun4c) {						    \
+		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);	    \
+		flush_tlb_page(__vma, __address);				    \
+		__update = 1;							    \
+	} else {								    \
+		__update = !pte_same(*(__ptep), __entry);			    \
+		if (__update) {			  				    \
+			set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);   \
+			flush_tlb_page(__vma, __address);			    \
+		} else if (__dirty) {						    \
+			flush_tlb_page(__vma, __address);			    \
+		}								    \
+	}									    \
+	__update;								    \
+})
+
 #include <asm-generic/pgtable.h>
 
 #endif /* !(__ASSEMBLY__) */
diff -ruNpd linux-2.6.20.9/include/asm-x86_64/pgtable.h linux-test/include/asm-x86_64/pgtable.h
--- linux-2.6.20.9/include/asm-x86_64/pgtable.h	2007-05-01 12:57:57.000000000 +0100
+++ linux-test/include/asm-x86_64/pgtable.h	2007-05-01 23:13:53.000000000 +0100
@@ -397,12 +397,16 @@ static inline pte_t pte_modify(pte_t pte
  * bit at the same time. */
 #define  __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
 #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								  \
+	({								  \
+		int __update = !pte_same(*(__ptep), __entry);		  \
+									  \
 		if (__dirty) {						  \
-			set_pte(__ptep, __entry);			  \
+			if (__update)					  \
+				set_pte(__ptep, __entry);		  \
 			flush_tlb_page(__vma, __address);		  \
 		}							  \
-	} while (0)
+		__update;						  \
+	})
 
 /* Encode and de-code a swap entry */
 #define __swp_type(x)			(((x).val >> 1) & 0x3f)
diff -ruNpd linux-2.6.20.9/mm/hugetlb.c linux-test/mm/hugetlb.c
--- linux-2.6.20.9/mm/hugetlb.c	2007-05-01 13:01:10.000000000 +0100
+++ linux-test/mm/hugetlb.c	2007-05-01 23:02:08.000000000 +0100
@@ -313,9 +313,10 @@ static void set_huge_ptep_writable(struc
 	pte_t entry;
 
 	entry = pte_mkwrite(pte_mkdirty(*ptep));
-	ptep_set_access_flags(vma, address, ptep, entry, 1);
-	update_mmu_cache(vma, address, entry);
-	lazy_mmu_prot_update(entry);
+	if (ptep_set_access_flags(vma, address, ptep, entry, 1)) {
+		update_mmu_cache(vma, address, entry);
+		lazy_mmu_prot_update(entry);
+	}
 }
 
 
diff -ruNpd linux-2.6.20.9/mm/memory.c linux-test/mm/memory.c
--- linux-2.6.20.9/mm/memory.c	2007-05-01 12:57:57.000000000 +0100
+++ linux-test/mm/memory.c	2007-05-01 23:06:21.000000000 +0100
@@ -1553,9 +1553,11 @@ static int do_wp_page(struct mm_struct *
 		flush_cache_page(vma, address, pte_pfn(orig_pte));
 		entry = pte_mkyoung(orig_pte);
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
-		ptep_set_access_flags(vma, address, page_table, entry, 1);
-		update_mmu_cache(vma, address, entry);
-		lazy_mmu_prot_update(entry);
+		if (ptep_set_access_flags(vma, address,
+					  page_table, entry, 1)) {
+			update_mmu_cache(vma, address, entry);
+			lazy_mmu_prot_update(entry);
+		}
 		ret |= VM_FAULT_WRITE;
 		goto unlock;
 	}
@@ -2387,10 +2389,9 @@ static inline int handle_pte_fault(struc
 		pte_t *pte, pmd_t *pmd, int write_access)
 {
 	pte_t entry;
-	pte_t old_entry;
 	spinlock_t *ptl;
 
-	old_entry = entry = *pte;
+	entry = *pte;
 	if (!pte_present(entry)) {
 		if (pte_none(entry)) {
 			if (vma->vm_ops) {
@@ -2423,19 +2424,9 @@ static inline int handle_pte_fault(struc
 		entry = pte_mkdirty(entry);
 	}
 	entry = pte_mkyoung(entry);
-	if (!pte_same(old_entry, entry)) {
-		ptep_set_access_flags(vma, address, pte, entry, write_access);
+	if (ptep_set_access_flags(vma, address, pte, entry, write_access)) {
 		update_mmu_cache(vma, address, entry);
 		lazy_mmu_prot_update(entry);
-	} else {
-		/*
-		 * This is needed only for protection faults but the arch code
-		 * is not yet telling us if this is a protection fault or not.
-		 * This still avoids useless tlb flushes for .text page faults
-		 * with threads.
-		 */
-		if (write_access)
-			flush_tlb_page(vma, address);
 	}
 unlock:
 	pte_unmap_unlock(pte, ptl);

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Mark Fortescue]

Hi Ben,

Is it worth formally sending in either of my patches or does more work 
need to be done first?

If you would like me to test any changes, it takes me app. 2 hours to 
cross-compile a sparc kernel for my sun4c. I use my sparc system as a 
diskless client with a very minimal setup to alow me to test cross 
compiled GCC and any small platform independent code I may be working on.

I have not yet tried to get linux-2.6.21 or later working but for the test 
setup I have been using, it should not take too long if that is the kernel 
needed for testing.

I may also be able to do the same testing on an embedded PowerPC (32bit) 
(it will need some work to get my cross compilation system working again 
as some kernel changes in the ppc/powerpc architechture have proven to be 
incompatible with my build scripts) and on x86_64/ix86. Once I have fixed 
the build scripts, it will take app. 4 to 6 hours to get the initial NFS 
root minimal system built for these additional architectures and then app. 
2 hours for each test kernel build.

If a simple ADA build is not considered a suficiently harsh test, then I 
could cross compile a specialist test application, if one is available, or 
compile a more extensive application (maybe gcc) on the test system. The 
problem of compiling a more extensive application on the sparc system is 
that it is a slow system running as a diskless client with its NFS root on 
an aging i486 over a 10MBit Ethernet. The result is it will take days to 
compile somthing like gcc.

Regards
 	Mark Fortescue

On Wed, 2 May 2007, Mark Fortescue wrote:

>
>
> On Wed, 2 May 2007, Benjamin Herrenschmidt wrote:
>
>> 
>>> I have attached a patch (so pine does not mangle it) for linux-2.6.20.9.
>>> Is this what you had in mind?
>>> 
>>> For linux-2.6.21, more work will be needed as it has more code calling
>>> ptep_set_access_flags.
>> 
>> I'm not 100% sure we need the 'update' argument... we can remove the
>> whole old_entry, pte_same, etc... and just have pte_set_access_flags()
>> read the old PTE and decide wether something needs to be changed or not.
>> 
>> Ben.
>> 
>> 
>
> The attached patch works on sun4c (with my simple ADA compile test) but the 
> change in functionality may break things other platforms.
>
> The advantage of the previous patch is that the functionality is only changed 
> for sparc sun4c so less testing would be required.
>
> Regards
> 	Mark Fortescue.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Benjamin Herrenschmidt]

On Wed, 2007-05-09 at 20:44 +0100, Mark Fortescue wrote:
> Hi Ben,
> 
> Is it worth formally sending in either of my patches or does more work 
> need to be done first?

Sorry, I've been busy with other things...

What do other thing about it ? Having update_mmu_cache() call buried
inside the ptep_set_access_flags() sounds good ? Somebody has a better
idea ?

One thing I was thinking was that we could replace the whole logic with
having ptep_set_access_flags() compare the new PTE bits with what was
already there and return wether an update_mmu_cache() is required....

Ben.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Andrew Morton]

On Wed, 2 May 2007 00:08:31 +0100 (BST) Mark Fortescue <mark@mtfhpc.demon.co.uk> wrote:

> On Wed, 2 May 2007, Benjamin Herrenschmidt wrote:
> 
> >
> >> I have attached a patch (so pine does not mangle it) for linux-2.6.20.9.
> >> Is this what you had in mind?
> >>
> >> For linux-2.6.21, more work will be needed as it has more code calling
> >> ptep_set_access_flags.
> >
> > I'm not 100% sure we need the 'update' argument... we can remove the
> > whole old_entry, pte_same, etc... and just have pte_set_access_flags()
> > read the old PTE and decide wether something needs to be changed or not.
> >
> > Ben.
> >
> >
> 
> The attached patch works on sun4c (with my simple ADA compile test) but 
> the change in functionality may break things other platforms.
> 
> The advantage of the previous patch is that the functionality is only 
> changed for sparc sun4c so less testing would be required.
> 
> Regards
>  	Mark Fortescue.
> 
> [Update-MMUCache-2.patch  TEXT/PLAIN (10.7KB)]
> diff -ruNpd linux-2.6.20.9/include/asm-generic/pgtable.h linux-test/include/asm-generic/pgtable.h
> --- linux-2.6.20.9/include/asm-generic/pgtable.h	2007-05-01 12:57:56.000000000 +0100
> +++ linux-test/include/asm-generic/pgtable.h	2007-05-01 23:13:23.000000000 +0100
> @@ -30,10 +30,17 @@ do {				  					\
>   * to optimize this.
>   */
>  #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
> -do {				  					  \
> -	set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);	  \
> -	flush_tlb_page(__vma, __address);				  \
> -} while (0)
> +({									  \
> +	int __update = !pte_same(*(__ptep), __entry);			  \
> +									  \
> +	if (__update) {				  			  \
> +		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry); \
> +		flush_tlb_page(__vma, __address);			  \
> +	} else if (__dirty) {						  \
> +		flush_tlb_page(__vma, __address);			  \
> +	}								  \
> +	__update;							  \
> +})

We never seemed to reach completion here?

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Benjamin Herrenschmidt]

> We never seemed to reach completion here?

Well, I'm waiting for other people comments too... as I said earlier,
I'm not too fan of burrying the update_mmu_cache() inside
ptep_set_access_flags(), but perhaps we could remove the whole logic of
reading the old PTE & comparing it, and instead have
ptep_set_access_flags() do that locally and return to the caller wether
a change occured that requires update_mmu_cache() to be called.

That way, archs who don't actually need update_mmu_cache() under some
circumstances will be able to return 0 there.

What do you guys thing ?

Ben.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 10 May 2007 16:29:43 +1000

> 
> > We never seemed to reach completion here?
> 
> Well, I'm waiting for other people comments too... as I said earlier,
> I'm not too fan of burrying the update_mmu_cache() inside
> ptep_set_access_flags(), but perhaps we could remove the whole logic of
> reading the old PTE & comparing it, and instead have
> ptep_set_access_flags() do that locally and return to the caller wether
> a change occured that requires update_mmu_cache() to be called.
> 
> That way, archs who don't actually need update_mmu_cache() under some
> circumstances will be able to return 0 there.
> 
> What do you guys thing ?

I think that's a good idea.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Hugh Dickins]

On Thu, 10 May 2007, David Miller wrote:
> > > We never seemed to reach completion here?
> > 
> > Well, I'm waiting for other people comments too... as I said earlier,
> > I'm not too fan of burrying the update_mmu_cache() inside
> > ptep_set_access_flags(), but perhaps we could remove the whole logic of
> > reading the old PTE & comparing it, and instead have
> > ptep_set_access_flags() do that locally and return to the caller wether
> > a change occured that requires update_mmu_cache() to be called.
> > 
> > That way, archs who don't actually need update_mmu_cache() under some
> > circumstances will be able to return 0 there.
> > 
> > What do you guys thing ?
> 
> I think that's a good idea.

I agree.

Hugh

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Benjamin Herrenschmidt]

On Mon, 2007-05-14 at 20:19 +0100, Hugh Dickins wrote:
> On Thu, 10 May 2007, David Miller wrote:
> > > > We never seemed to reach completion here?
> > > 
> > > Well, I'm waiting for other people comments too... as I said earlier,
> > > I'm not too fan of burrying the update_mmu_cache() inside
> > > ptep_set_access_flags(), but perhaps we could remove the whole logic of
> > > reading the old PTE & comparing it, and instead have
> > > ptep_set_access_flags() do that locally and return to the caller wether
> > > a change occured that requires update_mmu_cache() to be called.
> > > 
> > > That way, archs who don't actually need update_mmu_cache() under some
> > > circumstances will be able to return 0 there.
> > > 
> > > What do you guys thing ?
> > 
> > I think that's a good idea.
> 
> I agree.

Ok, I'll cook a patch today.

Ben.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Benjamin Herrenschmidt]

> Ok, I'll cook a patch today.

Let's make it tomorrow :-( Got delayed by other urgent things

Cheers,
Ben.

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Tom "spot" Callaway]

On Tue, 2007-05-15 at 16:56 +1000, Benjamin Herrenschmidt wrote:
> > Ok, I'll cook a patch today.
> 
> Let's make it tomorrow :-( Got delayed by other urgent things

Not to be annoying, but I'm patiently waiting on this patch. Would like
to test sun4c before shipping Aurora 3.0. Any status?

~spot

Re: vm changes from linux-2.6.14 to linux-2.6.15

[Benjamin Herrenschmidt]

On Mon, 2007-05-21 at 09:27 -0500, Tom "spot" Callaway wrote:
> On Tue, 2007-05-15 at 16:56 +1000, Benjamin Herrenschmidt wrote:
> > > Ok, I'll cook a patch today.
> > 
> > Let's make it tomorrow :-( Got delayed by other urgent things
> 
> Not to be annoying, but I'm patiently waiting on this patch. Would like
> to test sun4c before shipping Aurora 3.0. Any status?

Damn, kick me harder ! I totally forgot... probably what happens when
one is deep into bringing up some new HW toys !

Sorry about that, I'll do it asap.

Cheers
Ben.

[PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Benjamin Herrenschmidt]

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).

Index: linux-work/include/asm-generic/pgtable.h
===================================================================
--- 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
+ * in turn instructs the caller to do things like 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
  */
 #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-do {				  					  \
-	set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);	  \
-	flush_tlb_page(__vma, __address);				  \
-} while (0)
+({									  \
+	int __changed = !pte_same(*(__ptep), __entry);			  \
+	if (__changed) {						  \
+		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry); \
+		flush_tlb_page(__vma, __address);			  \
+	}								  \
+	__changed;							  \
+})
 #endif
 
 #ifndef __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
Index: linux-work/include/asm-powerpc/pgtable-ppc64.h
===================================================================
--- linux-work.orig/include/asm-powerpc/pgtable-ppc64.h	2007-05-22 15:04:45.000000000 +1000
+++ linux-work/include/asm-powerpc/pgtable-ppc64.h	2007-05-22 15:27:21.000000000 +1000
@@ -413,10 +413,14 @@ static inline void __ptep_set_access_fla
 	:"cc");
 }
 #define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								   \
-		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
-		flush_tlb_page_nohash(__vma, __address);	       	   \
-	} while(0)
+({									   \
+	int __changed = !pte_same(*(__ptep), __entry);			   \
+	if (__changed) {						   \
+		__ptep_set_access_flags(__ptep, __entry, __dirty);    	   \
+		flush_tlb_page_nohash(__vma, __address);		   \
+	}								   \
+	__changed;							   \
+})
 
 /*
  * Macro to mark a page protection value as "uncacheable".
Index: linux-work/mm/memory.c
===================================================================
--- linux-work.orig/mm/memory.c	2007-05-22 15:04:45.000000000 +1000
+++ linux-work/mm/memory.c	2007-05-22 15:38:19.000000000 +1000
@@ -1691,9 +1691,10 @@ static int do_wp_page(struct mm_struct *
 		flush_cache_page(vma, address, pte_pfn(orig_pte));
 		entry = pte_mkyoung(orig_pte);
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
-		ptep_set_access_flags(vma, address, page_table, entry, 1);
-		update_mmu_cache(vma, address, entry);
-		lazy_mmu_prot_update(entry);
+		if (ptep_set_access_flags(vma, address, page_table, entry,1)) {
+			update_mmu_cache(vma, address, entry);
+			lazy_mmu_prot_update(entry);
+		}
 		ret |= VM_FAULT_WRITE;
 		goto unlock;
 	}
@@ -2525,10 +2526,9 @@ static inline int handle_pte_fault(struc
 		pte_t *pte, pmd_t *pmd, int write_access)
 {
 	pte_t entry;
-	pte_t old_entry;
 	spinlock_t *ptl;
 
-	old_entry = entry = *pte;
+	entry = *pte;
 	if (!pte_present(entry)) {
 		if (pte_none(entry)) {
 			if (vma->vm_ops) {
@@ -2561,8 +2561,7 @@ static inline int handle_pte_fault(struc
 		entry = pte_mkdirty(entry);
 	}
 	entry = pte_mkyoung(entry);
-	if (!pte_same(old_entry, entry)) {
-		ptep_set_access_flags(vma, address, pte, entry, write_access);
+	if (ptep_set_access_flags(vma, address, pte, entry, write_access)) {
 		update_mmu_cache(vma, address, entry);
 		lazy_mmu_prot_update(entry);
 	} else {
Index: linux-work/include/asm-powerpc/pgtable-ppc32.h
===================================================================
--- linux-work.orig/include/asm-powerpc/pgtable-ppc32.h	2007-05-22 15:04:45.000000000 +1000
+++ linux-work/include/asm-powerpc/pgtable-ppc32.h	2007-05-22 15:26:07.000000000 +1000
@@ -673,10 +673,14 @@ static inline void __ptep_set_access_fla
 }
 
 #define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								   \
-		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
-		flush_tlb_page_nohash(__vma, __address);	       	   \
-	} while(0)
+({									   \
+	int __changed = !pte_same(*(__ptep), __entry);			   \
+	if (__changed) {						   \
+		__ptep_set_access_flags(__ptep, __entry, __dirty);    	   \
+		flush_tlb_page_nohash(__vma, __address);		   \
+	}								   \
+	__changed;							   \
+})
 
 /*
  * Macro to mark a page protection value as "uncacheable".
Index: linux-work/include/asm-i386/pgtable.h
===================================================================
--- 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);			\
+	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;							\
+})
 
 #define __HAVE_ARCH_PTEP_TEST_AND_CLEAR_DIRTY
 #define ptep_test_and_clear_dirty(vma, addr, ptep) ({			\
Index: linux-work/include/asm-ppc/pgtable.h
===================================================================
--- linux-work.orig/include/asm-ppc/pgtable.h	2007-05-22 15:25:58.000000000 +1000
+++ linux-work/include/asm-ppc/pgtable.h	2007-05-22 15:26:08.000000000 +1000
@@ -694,10 +694,14 @@ static inline void __ptep_set_access_fla
 }
 
 #define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								   \
-		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
-		flush_tlb_page_nohash(__vma, __address);	       	   \
-	} while(0)
+({									   \
+	int __changed = !pte_same(*(__ptep), __entry);			   \
+	if (__changed) {						   \
+		__ptep_set_access_flags(__ptep, __entry, __dirty);    	   \
+		flush_tlb_page_nohash(__vma, __address);		   \
+	}								   \
+	__changed;							   \
+})
 
 /*
  * Macro to mark a page protection value as "uncacheable".
Index: linux-work/include/asm-s390/pgtable.h
===================================================================
--- linux-work.orig/include/asm-s390/pgtable.h	2007-05-22 15:16:48.000000000 +1000
+++ linux-work/include/asm-s390/pgtable.h	2007-05-22 15:20:16.000000000 +1000
@@ -744,7 +744,12 @@ ptep_establish(struct vm_area_struct *vm
 }
 
 #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	ptep_establish(__vma, __address, __ptep, __entry)
+({									  \
+	int __changed = !pte_same(*(__ptep), __entry);			  \
+	if (__changed)							  \
+		ptep_establish(__vma, __address, __ptep, __entry);	  \
+	__changed;							  \
+})
 
 /*
  * Test and clear dirty bit in storage key.
Index: linux-work/include/asm-sparc/pgtable.h
===================================================================
--- linux-work.orig/include/asm-sparc/pgtable.h	2007-05-22 15:30:48.000000000 +1000
+++ linux-work/include/asm-sparc/pgtable.h	2007-05-22 15:35:56.000000000 +1000
@@ -446,6 +446,17 @@ extern int io_remap_pfn_range(struct vm_
 #define GET_IOSPACE(pfn)		(pfn >> (BITS_PER_LONG - 4))
 #define GET_PFN(pfn)			(pfn & 0x0fffffffUL)
 
+#define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
+#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
+({									  \
+	int __changed = !pte_same(*(__ptep), __entry);			  \
+	if (__changed) {						  \
+		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry); \
+		flush_tlb_page(__vma, __address);			  \
+	}								  \
+	(sparc_cpu_model == sun4c) || __changed;			  \
+})
+
 #include <asm-generic/pgtable.h>
 
 #endif /* !(__ASSEMBLY__) */
Index: linux-work/include/asm-x86_64/pgtable.h
===================================================================
--- linux-work.orig/include/asm-x86_64/pgtable.h	2007-05-22 15:20:40.000000000 +1000
+++ linux-work/include/asm-x86_64/pgtable.h	2007-05-22 15:21:52.000000000 +1000
@@ -395,12 +395,14 @@ static inline pte_t pte_modify(pte_t pte
  * bit at the same time. */
 #define  __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
 #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
-	do {								  \
-		if (__dirty) {						  \
-			set_pte(__ptep, __entry);			  \
-			flush_tlb_page(__vma, __address);		  \
-		}							  \
-	} while (0)
+({									  \
+	int __changed = !pte_same(*(__ptep), __entry);			  \
+	if (__changed && __dirty) {					  \
+		set_pte(__ptep, __entry);			  	  \
+		flush_tlb_page(__vma, __address);		  	  \
+	}								  \
+	__changed;							  \
+})
 
 /* Encode and de-code a swap entry */
 #define __swp_type(x)			(((x).val >> 1) & 0x3f)

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Hugh Dickins]

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?


> --- 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
>   */

> --- 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

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Benjamin Herrenschmidt]

> 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.

Ok.

> Would the "__changed && __dirty" architectures (x86, x86_64, ia64)
> be better off saying __changed = __dirty && pte_same?  I doubt it's
> worth bothering about.

I'd say let gcc figure it out :-)

> 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.

Nah, I must have missed set_huge_ptep_writable(). I don't think the wp
code path matters much anyway, it's likely to always be different.

> 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?

Well, I don't know which is why I'm waiting for Tom Callaway to test.
Davem mentioned update_mmu_cache only though when we discussed the
problem initially.

> >  #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);			\

Ah yes, sorry about that. I need to setup an x86 toolchain somewhere :-)

> Here's what I think the ia64 hunk would be, unbuilt and untested.

Ok.

I'll respin a patch later today.

Cheers,
Ben.

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Tom "spot" Callaway]

On Wed, 2007-05-23 at 08:59 +1000, Benjamin Herrenschmidt wrote:

> Well, I don't know which is why I'm waiting for Tom Callaway to test.
> Davem mentioned update_mmu_cache only though when we discussed the
> problem initially.

Mark already tested it and said it worked for him. This is sufficient
for me, as I'm not sure how many Aurora sun4c users there actually are.
If the extra bit turns out to be needed, I can push an update.

~spot

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Hugh Dickins]

On Wed, 23 May 2007, Benjamin Herrenschmidt wrote:
> 
> > Would the "__changed && __dirty" architectures (x86, x86_64, ia64)
> > be better off saying __changed = __dirty && pte_same?  I doubt it's
> > worth bothering about.
> 
> I'd say let gcc figure it out :-)

No, I wasn't meaning the optimization, but the significance of the
boolean __changed that's returned.  If ptep_set_access_flags does
not change the pte (because !dirty or !safely_writable or whatever
that arch calls it), then ideally it ought to return false.

But it doesn't affect correctness if it sometimes says true not
false, and these arches happen to have an empty update_mmu_cache
(with lazy_mmu_prot_update currently under separate review), and
what you have follows what was already being done, and sun4c
already has to "lie": so it's rather theoretical.

Hugh

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Benjamin Herrenschmidt]

On Wed, 2007-05-23 at 05:03 +0100, Hugh Dickins wrote:
> 
> No, I wasn't meaning the optimization, but the significance of the
> boolean __changed that's returned.  If ptep_set_access_flags does
> not change the pte (because !dirty or !safely_writable or whatever
> that arch calls it), then ideally it ought to return false.

Hrm... I prefer keeping the existing semantics. The old code used to
always update_mmu_cache() on those archs and I'd rather let it continue
do so unless the arch maintainer who knows better changes it :-)
 
> But it doesn't affect correctness if it sometimes says true not
> false, and these arches happen to have an empty update_mmu_cache
> (with lazy_mmu_prot_update currently under separate review), and
> what you have follows what was already being done, and sun4c
> already has to "lie": so it's rather theoretical. 

Ok.

Cheers,
Ben.

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Mark Fortescue]

Hi Benjamin,

I have just tested this patch on my Sun4c Sparcstation 1 using my 2.6.20.9 
test kernel without any problems.

Thank you for the work.

Regards
 	Mark Fortescue.

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).
>
> Index: linux-work/include/asm-generic/pgtable.h
> ===================================================================
> --- 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
> + * in turn instructs the caller to do things like 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
>  */
> #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
> -do {				  					  \
> -	set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry);	  \
> -	flush_tlb_page(__vma, __address);				  \
> -} while (0)
> +({									  \
> +	int __changed = !pte_same(*(__ptep), __entry);			  \
> +	if (__changed) {						  \
> +		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry); \
> +		flush_tlb_page(__vma, __address);			  \
> +	}								  \
> +	__changed;							  \
> +})
> #endif
>
> #ifndef __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
> Index: linux-work/include/asm-powerpc/pgtable-ppc64.h
> ===================================================================
> --- linux-work.orig/include/asm-powerpc/pgtable-ppc64.h	2007-05-22 15:04:45.000000000 +1000
> +++ linux-work/include/asm-powerpc/pgtable-ppc64.h	2007-05-22 15:27:21.000000000 +1000
> @@ -413,10 +413,14 @@ static inline void __ptep_set_access_fla
> 	:"cc");
> }
> #define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
> -	do {								   \
> -		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
> -		flush_tlb_page_nohash(__vma, __address);	       	   \
> -	} while(0)
> +({									   \
> +	int __changed = !pte_same(*(__ptep), __entry);			   \
> +	if (__changed) {						   \
> +		__ptep_set_access_flags(__ptep, __entry, __dirty);    	   \
> +		flush_tlb_page_nohash(__vma, __address);		   \
> +	}								   \
> +	__changed;							   \
> +})
>
> /*
>  * Macro to mark a page protection value as "uncacheable".
> Index: linux-work/mm/memory.c
> ===================================================================
> --- linux-work.orig/mm/memory.c	2007-05-22 15:04:45.000000000 +1000
> +++ linux-work/mm/memory.c	2007-05-22 15:38:19.000000000 +1000
> @@ -1691,9 +1691,10 @@ static int do_wp_page(struct mm_struct *
> 		flush_cache_page(vma, address, pte_pfn(orig_pte));
> 		entry = pte_mkyoung(orig_pte);
> 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> -		ptep_set_access_flags(vma, address, page_table, entry, 1);
> -		update_mmu_cache(vma, address, entry);
> -		lazy_mmu_prot_update(entry);
> +		if (ptep_set_access_flags(vma, address, page_table, entry,1)) {
> +			update_mmu_cache(vma, address, entry);
> +			lazy_mmu_prot_update(entry);
> +		}
> 		ret |= VM_FAULT_WRITE;
> 		goto unlock;
> 	}
> @@ -2525,10 +2526,9 @@ static inline int handle_pte_fault(struc
> 		pte_t *pte, pmd_t *pmd, int write_access)
> {
> 	pte_t entry;
> -	pte_t old_entry;
> 	spinlock_t *ptl;
>
> -	old_entry = entry = *pte;
> +	entry = *pte;
> 	if (!pte_present(entry)) {
> 		if (pte_none(entry)) {
> 			if (vma->vm_ops) {
> @@ -2561,8 +2561,7 @@ static inline int handle_pte_fault(struc
> 		entry = pte_mkdirty(entry);
> 	}
> 	entry = pte_mkyoung(entry);
> -	if (!pte_same(old_entry, entry)) {
> -		ptep_set_access_flags(vma, address, pte, entry, write_access);
> +	if (ptep_set_access_flags(vma, address, pte, entry, write_access)) {
> 		update_mmu_cache(vma, address, entry);
> 		lazy_mmu_prot_update(entry);
> 	} else {
> Index: linux-work/include/asm-powerpc/pgtable-ppc32.h
> ===================================================================
> --- linux-work.orig/include/asm-powerpc/pgtable-ppc32.h	2007-05-22 15:04:45.000000000 +1000
> +++ linux-work/include/asm-powerpc/pgtable-ppc32.h	2007-05-22 15:26:07.000000000 +1000
> @@ -673,10 +673,14 @@ static inline void __ptep_set_access_fla
> }
>
> #define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
> -	do {								   \
> -		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
> -		flush_tlb_page_nohash(__vma, __address);	       	   \
> -	} while(0)
> +({									   \
> +	int __changed = !pte_same(*(__ptep), __entry);			   \
> +	if (__changed) {						   \
> +		__ptep_set_access_flags(__ptep, __entry, __dirty);    	   \
> +		flush_tlb_page_nohash(__vma, __address);		   \
> +	}								   \
> +	__changed;							   \
> +})
>
> /*
>  * Macro to mark a page protection value as "uncacheable".
> Index: linux-work/include/asm-i386/pgtable.h
> ===================================================================
> --- 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);			\
> +	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;							\
> +})
>
> #define __HAVE_ARCH_PTEP_TEST_AND_CLEAR_DIRTY
> #define ptep_test_and_clear_dirty(vma, addr, ptep) ({			\
> Index: linux-work/include/asm-ppc/pgtable.h
> ===================================================================
> --- linux-work.orig/include/asm-ppc/pgtable.h	2007-05-22 15:25:58.000000000 +1000
> +++ linux-work/include/asm-ppc/pgtable.h	2007-05-22 15:26:08.000000000 +1000
> @@ -694,10 +694,14 @@ static inline void __ptep_set_access_fla
> }
>
> #define  ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
> -	do {								   \
> -		__ptep_set_access_flags(__ptep, __entry, __dirty);	   \
> -		flush_tlb_page_nohash(__vma, __address);	       	   \
> -	} while(0)
> +({									   \
> +	int __changed = !pte_same(*(__ptep), __entry);			   \
> +	if (__changed) {						   \
> +		__ptep_set_access_flags(__ptep, __entry, __dirty);    	   \
> +		flush_tlb_page_nohash(__vma, __address);		   \
> +	}								   \
> +	__changed;							   \
> +})
>
> /*
>  * Macro to mark a page protection value as "uncacheable".
> Index: linux-work/include/asm-s390/pgtable.h
> ===================================================================
> --- linux-work.orig/include/asm-s390/pgtable.h	2007-05-22 15:16:48.000000000 +1000
> +++ linux-work/include/asm-s390/pgtable.h	2007-05-22 15:20:16.000000000 +1000
> @@ -744,7 +744,12 @@ ptep_establish(struct vm_area_struct *vm
> }
>
> #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
> -	ptep_establish(__vma, __address, __ptep, __entry)
> +({									  \
> +	int __changed = !pte_same(*(__ptep), __entry);			  \
> +	if (__changed)							  \
> +		ptep_establish(__vma, __address, __ptep, __entry);	  \
> +	__changed;							  \
> +})
>
> /*
>  * Test and clear dirty bit in storage key.
> Index: linux-work/include/asm-sparc/pgtable.h
> ===================================================================
> --- linux-work.orig/include/asm-sparc/pgtable.h	2007-05-22 15:30:48.000000000 +1000
> +++ linux-work/include/asm-sparc/pgtable.h	2007-05-22 15:35:56.000000000 +1000
> @@ -446,6 +446,17 @@ extern int io_remap_pfn_range(struct vm_
> #define GET_IOSPACE(pfn)		(pfn >> (BITS_PER_LONG - 4))
> #define GET_PFN(pfn)			(pfn & 0x0fffffffUL)
>
> +#define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
> +#define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
> +({									  \
> +	int __changed = !pte_same(*(__ptep), __entry);			  \
> +	if (__changed) {						  \
> +		set_pte_at((__vma)->vm_mm, (__address), __ptep, __entry); \
> +		flush_tlb_page(__vma, __address);			  \
> +	}								  \
> +	(sparc_cpu_model == sun4c) || __changed;			  \
> +})
> +
> #include <asm-generic/pgtable.h>
>
> #endif /* !(__ASSEMBLY__) */
> Index: linux-work/include/asm-x86_64/pgtable.h
> ===================================================================
> --- linux-work.orig/include/asm-x86_64/pgtable.h	2007-05-22 15:20:40.000000000 +1000
> +++ linux-work/include/asm-x86_64/pgtable.h	2007-05-22 15:21:52.000000000 +1000
> @@ -395,12 +395,14 @@ static inline pte_t pte_modify(pte_t pte
>  * bit at the same time. */
> #define  __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS
> #define ptep_set_access_flags(__vma, __address, __ptep, __entry, __dirty) \
> -	do {								  \
> -		if (__dirty) {						  \
> -			set_pte(__ptep, __entry);			  \
> -			flush_tlb_page(__vma, __address);		  \
> -		}							  \
> -	} while (0)
> +({									  \
> +	int __changed = !pte_same(*(__ptep), __entry);			  \
> +	if (__changed && __dirty) {					  \
> +		set_pte(__ptep, __entry);			  	  \
> +		flush_tlb_page(__vma, __address);		  	  \
> +	}								  \
> +	__changed;							  \
> +})
>
> /* Encode and de-code a swap entry */
> #define __swp_type(x)			(((x).val >> 1) & 0x3f)
>
>
>

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[David Miller]

From: Mark Fortescue <mark@mtfhpc.demon.co.uk>
Date: Tue, 22 May 2007 22:52:13 +0100 (BST)

> Hi Benjamin,
> 
> I have just tested this patch on my Sun4c Sparcstation 1 using my 2.6.20.9 
> test kernel without any problems.
> 
> Thank you for the work.

Thanks for your testing.

Someone please merge this in once any remaining issues have
been resolved :-)

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Benjamin Herrenschmidt]

On Tue, 2007-05-22 at 14:53 -0700, David Miller wrote:
> From: Mark Fortescue <mark@mtfhpc.demon.co.uk>
> Date: Tue, 22 May 2007 22:52:13 +0100 (BST)
> 
> > Hi Benjamin,
> > 
> > I have just tested this patch on my Sun4c Sparcstation 1 using my 2.6.20.9 
> > test kernel without any problems.
> > 
> > Thank you for the work.
> 
> Thanks for your testing.
> 
> Someone please merge this in once any remaining issues have
> been resolved :-)

I'll send a patch fixing the couple of x86/ia64 nits & my bad english
spelling (no, I don't happen to raise castrated rams) as soon as I reach
the office later today.

Cheers,
Ben.

Re: [PATCH/RFC] Rework ptep_set_access_flags and fix sun4c

[Benjamin Herrenschmidt]

On Tue, 2007-05-22 at 22:52 +0100, Mark Fortescue wrote:
> Hi Benjamin,
> 
> I have just tested this patch on my Sun4c Sparcstation 1 using my 2.6.20.9 
> test kernel without any problems.
> 
> Thank you for the work.

Wow, there is more than one user of these still ! :-)

Cheers,
Ben.