Skip to contentSkip to conversation

sparc: huge_ptep_set_* functions need to call set_huge_pte_at()

PATCH In review Dave Kleikamp 3 participants 6 messages Dec 17, 2012 – Dec 18, 2012 SPARC + UltraSPARC (sparc/sparc64)

lore
Share & tools
lore

Dave Kleikamp

author
#0 lore raw

PATCH v2 1/1 sparc: huge_ptep_set_* functions need to call set_huge_pte_at()

Quoted 14 lines · David Miller

From: Dave Kleikamp <[email protected]>
Date: Fri, 14 Dec 2012 15:02:00 -0600

Quoted 11 lines

static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,

(quoted diff) +4 -1
 					     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;
 }
Quoted 4 lines
Quoted 1 line

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 <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc [email protected]
---
 arch/sparc/include/asm/hugetlb.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
arch/sparc/include/asm/hugetlb.h +8 -2
index 8c5eed6..9661e9b 100644
@@ -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)
Signature · 1 line
1.8.0.1
#1 lore raw

arch/sparc/include/asm/hugetlb.h static inline pte_t huge_pte_wrprotect(pte_t pte) · line 78

Quoted 12 lines · Dave Kleikamp

static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,

arch/sparc/include/asm/hugetlb.h +6 -1
 					     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?

Signature · 1 line
     2. That which causes joy or happiness.
author
#2 lore raw
In reply to Dave Kleikamp
Quoted 17 lines · Josip Rodin

arch/sparc/include/asm/hugetlb.h static inline pte_t huge_pte_wrprotect(pte_t pte) · line 78

Quoted 12 lines · Dave Kleikamp

static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,

arch/sparc/include/asm/hugetlb.h +6 -1
 					     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.

maintainer
#3 lore raw
In reply to Dave Kleikamp

From: Dave Kleikamp <[email protected]>
Date: Mon, 17 Dec 2012 11:52:47 -0600

Quoted 20 lines · David Miller
Quoted 14 lines

From: Dave Kleikamp <[email protected]>
Date: Fri, 14 Dec 2012 15:02:00 -0600

Quoted 11 lines

static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,

arch/sparc/include/asm/hugetlb.h +4 -1
 					     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;
 }
Quoted 4 lines
Quoted 1 line

static inline pte_t huge_ptep_get(pte_t *ptep)

This lacks the TLB flush after setting the pte, which is really needed.

Quoted 6 lines

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 <[email protected]>

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.

author
#4 lore raw
In reply to Dave Kleikamp
Quoted 6 lines · David Miller

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