diff mbox

[v2] sparc: huge_ptep_set_* functions need to call set_huge_pte_at()

Message ID 50CF5BEF.9080009@oracle.com
State Accepted
Delegated to: David Miller
Headers show

Commit Message

Dave Kleikamp Dec. 17, 2012, 5:52 p.m. UTC 
On 12/16/2012 10:52 AM, David Miller wrote:
> From: Dave Kleikamp <dave.kleikamp@oracle.com>
> Date: Fri, 14 Dec 2012 15:02:00 -0600
> 
>>  static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>  					     unsigned long addr, pte_t *ptep,
>>  					     pte_t pte, int dirty)
>>  {
>> -	return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
>> +	int changed = !pte_same(*ptep, pte);
>> +	if (changed)
>> +		set_huge_pte_at(vma->vm_mm, addr, ptep, pte);
>> +	return changed;
>>  }
>>  
>>  static inline pte_t huge_ptep_get(pte_t *ptep)
> 
> This lacks the TLB flush after setting the pte, which is really
> needed.

Modifying the huge pte's requires that all the underlying pte's be
modified.

Version 2: added missing flush_tlb_page()

Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
---
 arch/sparc/include/asm/hugetlb.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Josip Rodin Dec. 17, 2012, 6:24 p.m. UTC | #1 
On Mon, Dec 17, 2012 at 11:52:47AM -0600, Dave Kleikamp wrote:
>  static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>  					     unsigned long addr, pte_t *ptep,
>  					     pte_t pte, int dirty)
>  {
> -	return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
> +	int changed = !pte_same(*ptep, pte);
> +	if (changed) {
> +		set_huge_pte_at(vma->vm_mm, addr, ptep, pte);
> +		flush_tlb_page(vma, addr);
> +	}
> +	return changed;
>  }

I've no idea what this really does, but I noticed that you are ignoring the
'dirty' parameter in the new version. If it's irrelevant, maybe make a note
of it to avoid the impression it got lost?
Dave Kleikamp Dec. 17, 2012, 6:44 p.m. UTC | #2 
On 12/17/2012 12:24 PM, Josip Rodin wrote:
> On Mon, Dec 17, 2012 at 11:52:47AM -0600, Dave Kleikamp wrote:
>>  static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>  					     unsigned long addr, pte_t *ptep,
>>  					     pte_t pte, int dirty)
>>  {
>> -	return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
>> +	int changed = !pte_same(*ptep, pte);
>> +	if (changed) {
>> +		set_huge_pte_at(vma->vm_mm, addr, ptep, pte);
>> +		flush_tlb_page(vma, addr);
>> +	}
>> +	return changed;
>>  }
> 
> I've no idea what this really does, but I noticed that you are ignoring the
> 'dirty' parameter in the new version. If it's irrelevant, maybe make a note
> of it to avoid the impression it got lost?

It seems x86 is the only architecture that pays any attention to this
flag. ptep_set_access_flags() ignores it as well.
--
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
David Miller Dec. 17, 2012, 7:17 p.m. UTC | #3 
From: Dave Kleikamp <dave.kleikamp@oracle.com>
Date: Mon, 17 Dec 2012 11:52:47 -0600

> On 12/16/2012 10:52 AM, David Miller wrote:
>> From: Dave Kleikamp <dave.kleikamp@oracle.com>
>> Date: Fri, 14 Dec 2012 15:02:00 -0600
>> 
>>>  static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>>>  					     unsigned long addr, pte_t *ptep,
>>>  					     pte_t pte, int dirty)
>>>  {
>>> -	return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
>>> +	int changed = !pte_same(*ptep, pte);
>>> +	if (changed)
>>> +		set_huge_pte_at(vma->vm_mm, addr, ptep, pte);
>>> +	return changed;
>>>  }
>>>  
>>>  static inline pte_t huge_ptep_get(pte_t *ptep)
>> 
>> This lacks the TLB flush after setting the pte, which is really
>> needed.
> 
> Modifying the huge pte's requires that all the underlying pte's be
> modified.
> 
> Version 2: added missing flush_tlb_page()
> 
> Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>

Please don't submit new versions of patches as a reply like this.

Always submit new patches as fresh list postings, that way the
maintainer doesn't have to edit out all of the unnecessary
reply verbiage from the resulting commit message during checkin.
--
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
Dave Kleikamp Dec. 17, 2012, 7:30 p.m. UTC | #4 
On 12/17/2012 01:17 PM, David Miller wrote:
> 
> Please don't submit new versions of patches as a reply like this.
> 
> Always submit new patches as fresh list postings, that way the
> maintainer doesn't have to edit out all of the unnecessary
> reply verbiage from the resulting commit message during checkin.

Sorry about that.

Shaggy
--
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
David Miller Dec. 18, 2012, 11:48 p.m. UTC | #5 
From: Dave Kleikamp <dave.kleikamp@oracle.com>
Date: Mon, 17 Dec 2012 11:52:47 -0600

> Modifying the huge pte's requires that all the underlying pte's be
> modified.
> 
> Version 2: added missing flush_tlb_page()
> 
> Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>

Applied and queued up for -stable, thanks.
--
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
diff mbox

Patch

diff --git a/arch/sparc/include/asm/hugetlb.h b/arch/sparc/include/asm/hugetlb.h
index 8c5eed6..9661e9b 100644
--- a/arch/sparc/include/asm/hugetlb.h
+++ b/arch/sparc/include/asm/hugetlb.h
@@ -61,14 +61,20 @@  static inline pte_t huge_pte_wrprotect(pte_t pte)
 static inline void huge_ptep_set_wrprotect(struct mm_struct *mm,
 					   unsigned long addr, pte_t *ptep)
 {
-	ptep_set_wrprotect(mm, addr, ptep);
+	pte_t old_pte = *ptep;
+	set_huge_pte_at(mm, addr, ptep, pte_wrprotect(old_pte));
 }
 
 static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
 					     unsigned long addr, pte_t *ptep,
 					     pte_t pte, int dirty)
 {
-	return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
+	int changed = !pte_same(*ptep, pte);
+	if (changed) {
+		set_huge_pte_at(vma->vm_mm, addr, ptep, pte);
+		flush_tlb_page(vma, addr);
+	}
+	return changed;
 }
 
 static inline pte_t huge_ptep_get(pte_t *ptep)