| Message ID | 20181218094137.13732-6-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | NestMMU pte upgrade workaround for mprotect | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 4 checks, 72 lines checked |
On Tue, Dec 18, 2018 at 03:11:37PM +0530, Aneesh Kumar K.V wrote: > +EXPORT_SYMBOL(huge_ptep_modify_prot_start); The only user of this function is the one you added in the last patch in mm/hugetlb.c, so there is no need to export this function. > + > +void huge_ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr, > + pte_t *ptep, pte_t old_pte, pte_t pte) > +{ > + > + if (radix_enabled()) > + return radix__huge_ptep_modify_prot_commit(vma, addr, ptep, > + old_pte, pte); > + set_huge_pte_at(vma->vm_mm, addr, ptep, pte); > +} > +EXPORT_SYMBOL(huge_ptep_modify_prot_commit); Same here.
Christoph Hellwig <hch@infradead.org> writes: > On Tue, Dec 18, 2018 at 03:11:37PM +0530, Aneesh Kumar K.V wrote: >> +EXPORT_SYMBOL(huge_ptep_modify_prot_start); > > The only user of this function is the one you added in the last patch > in mm/hugetlb.c, so there is no need to export this function. > >> + >> +void huge_ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr, >> + pte_t *ptep, pte_t old_pte, pte_t pte) >> +{ >> + >> + if (radix_enabled()) >> + return radix__huge_ptep_modify_prot_commit(vma, addr, ptep, >> + old_pte, pte); >> + set_huge_pte_at(vma->vm_mm, addr, ptep, pte); >> +} >> +EXPORT_SYMBOL(huge_ptep_modify_prot_commit); > > Same here. That was done considering that ptep_modify_prot_start/commit was defined in asm-generic/pgtable.h. I was trying to make sure I didn't break anything with the patch. Also s390 do have that EXPORT_SYMBOL() for the same. hugetlb just inherited that. If you feel strongly about it, I can drop the EXPORT_SYMBOL(). -aneesh
On Wed, Dec 19, 2018 at 08:50:57AM +0530, Aneesh Kumar K.V wrote: > That was done considering that ptep_modify_prot_start/commit was defined > in asm-generic/pgtable.h. I was trying to make sure I didn't break > anything with the patch. Also s390 do have that EXPORT_SYMBOL() for the > same. hugetlb just inherited that. > > If you feel strongly about it, I can drop the EXPORT_SYMBOL(). Yes. And we should probably remove the s390 as well as it isn't used either.
On Wed, 2018-12-19 at 08:50 +0530, Aneesh Kumar K.V wrote: > Christoph Hellwig <hch@infradead.org> writes: > > > On Tue, Dec 18, 2018 at 03:11:37PM +0530, Aneesh Kumar K.V wrote: > > > +EXPORT_SYMBOL(huge_ptep_modify_prot_start); > > > > The only user of this function is the one you added in the last patch > > in mm/hugetlb.c, so there is no need to export this function. > > > > > + > > > +void huge_ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr, > > > + pte_t *ptep, pte_t old_pte, pte_t pte) > > > +{ > > > + > > > + if (radix_enabled()) > > > + return radix__huge_ptep_modify_prot_commit(vma, addr, ptep, > > > + old_pte, pte); > > > + set_huge_pte_at(vma->vm_mm, addr, ptep, pte); > > > +} > > > +EXPORT_SYMBOL(huge_ptep_modify_prot_commit); > > > > Same here. > > That was done considering that ptep_modify_prot_start/commit was defined > in asm-generic/pgtable.h. I was trying to make sure I didn't break > anything with the patch. Also s390 do have that EXPORT_SYMBOL() for the > same. hugetlb just inherited that. > > If you feel strongly about it, I can drop the EXPORT_SYMBOL(). At the very least it should be _GPL Cheers, Ben.
On Thu, Dec 20, 2018 at 11:30:12AM +1100, Benjamin Herrenschmidt wrote: > On Wed, 2018-12-19 at 08:50 +0530, Aneesh Kumar K.V wrote: > > Christoph Hellwig <hch@infradead.org> writes: > > > > > On Tue, Dec 18, 2018 at 03:11:37PM +0530, Aneesh Kumar K.V wrote: > > > > +EXPORT_SYMBOL(huge_ptep_modify_prot_start); > > > > > > The only user of this function is the one you added in the last patch > > > in mm/hugetlb.c, so there is no need to export this function. > > > > > > > + > > > > +void huge_ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr, > > > > + pte_t *ptep, pte_t old_pte, pte_t pte) > > > > +{ > > > > + > > > > + if (radix_enabled()) > > > > + return radix__huge_ptep_modify_prot_commit(vma, addr, ptep, > > > > + old_pte, pte); > > > > + set_huge_pte_at(vma->vm_mm, addr, ptep, pte); > > > > +} > > > > +EXPORT_SYMBOL(huge_ptep_modify_prot_commit); > > > > > > Same here. > > > > That was done considering that ptep_modify_prot_start/commit was defined > > in asm-generic/pgtable.h. I was trying to make sure I didn't break > > anything with the patch. Also s390 do have that EXPORT_SYMBOL() for the > > same. hugetlb just inherited that. > > > > If you feel strongly about it, I can drop the EXPORT_SYMBOL(). > > At the very least it should be _GPL In general yes, but in this case it just needs to go. The s390 maintainers already removed a patch to remove their ptep_modify_prot_start / ptep_modify_prot_commit exports.
diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h index 5b0177733994..66c1e4f88d65 100644 --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h @@ -13,6 +13,10 @@ radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags); +extern void radix__huge_ptep_modify_prot_commit(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep, + pte_t old_pte, pte_t pte); + static inline int hstate_get_psize(struct hstate *hstate) { unsigned long shift; @@ -42,4 +46,12 @@ static inline bool gigantic_page_supported(void) /* hugepd entry valid bit */ #define HUGEPD_VAL_BITS (0x8000000000000000UL) +#define huge_ptep_modify_prot_start huge_ptep_modify_prot_start +extern pte_t huge_ptep_modify_prot_start(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep); + +#define huge_ptep_modify_prot_commit huge_ptep_modify_prot_commit +extern void huge_ptep_modify_prot_commit(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep, + pte_t old_pte, pte_t new_pte); #endif diff --git a/arch/powerpc/mm/hugetlbpage-hash64.c b/arch/powerpc/mm/hugetlbpage-hash64.c index 2e6a8f9345d3..48fe74bfeab1 100644 --- a/arch/powerpc/mm/hugetlbpage-hash64.c +++ b/arch/powerpc/mm/hugetlbpage-hash64.c @@ -121,3 +121,30 @@ int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid, *ptep = __pte(new_pte & ~H_PAGE_BUSY); return 0; } + +pte_t huge_ptep_modify_prot_start(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep) +{ + unsigned long pte_val; + /* + * Clear the _PAGE_PRESENT so that no hardware parallel update is + * possible. Also keep the pte_present true so that we don't take + * wrong fault. + */ + pte_val = pte_update(vma->vm_mm, addr, ptep, + _PAGE_PRESENT, _PAGE_INVALID, 1); + + return __pte(pte_val); +} +EXPORT_SYMBOL(huge_ptep_modify_prot_start); + +void huge_ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr, + pte_t *ptep, pte_t old_pte, pte_t pte) +{ + + if (radix_enabled()) + return radix__huge_ptep_modify_prot_commit(vma, addr, ptep, + old_pte, pte); + set_huge_pte_at(vma->vm_mm, addr, ptep, pte); +} +EXPORT_SYMBOL(huge_ptep_modify_prot_commit); diff --git a/arch/powerpc/mm/hugetlbpage-radix.c b/arch/powerpc/mm/hugetlbpage-radix.c index 2486bee0f93e..11d9ea28a816 100644 --- a/arch/powerpc/mm/hugetlbpage-radix.c +++ b/arch/powerpc/mm/hugetlbpage-radix.c @@ -90,3 +90,20 @@ radix__hugetlb_get_unmapped_area(struct file *file, unsigned long addr, return vm_unmapped_area(&info); } + +void radix__huge_ptep_modify_prot_commit(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep, + pte_t old_pte, pte_t pte) +{ + struct mm_struct *mm = vma->vm_mm; + + /* + * To avoid NMMU hang while relaxing access we need to flush the tlb before + * we set the new value. + */ + if (is_pte_rw_upgrade(pte_val(old_pte), pte_val(pte)) && + (atomic_read(&mm->context.copros) > 0)) + radix__flush_hugetlb_page(vma, addr); + + set_huge_pte_at(vma->vm_mm, addr, ptep, pte); +}
NestMMU requires us to mark the pte invalid and flush the tlb when we do a RW upgrade of pte. We fixed a variant of this in the fault path in commit Fixes: bd5050e38aec ("powerpc/mm/radix: Change pte relax sequence to handle nest MMU hang") Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- arch/powerpc/include/asm/book3s/64/hugetlb.h | 12 +++++++++ arch/powerpc/mm/hugetlbpage-hash64.c | 27 ++++++++++++++++++++ arch/powerpc/mm/hugetlbpage-radix.c | 17 ++++++++++++ 3 files changed, 56 insertions(+)