Using pte_t and pte_xxx functions in mm/hugetlbfs.c for "huge ptes" has
always been confusing and error-prone on architectures that have a
different layout for the pte/pmd/... level entries. That was the reason
for the individual arch header files in <arch>/include/asm/hugetlb.h
containing implementations for various huge_pte_xxx versions of the
original pte_xxx functions, if needed.
Commit abf09bed3c "s390/mm: implement software dirty bits" introduced
another difference in the pte layout vs. the pmd layout on s390,
thoroughly breaking the s390 support for hugetlbfs. This requires
replacing some more pte_xxx functions in mm/hugetlbfs.c with a
huge_pte_xxx version.
This patch introduces those huge_pte_xxx functions and their
implementation on all architectures supporting hugetlbfs. This change
will be a no-op for all architectures other than s390.
I am also thinking about a more complete clean-up patch, replacing all
remaining pte_xxx invocations in mm/hugetlbfs.c and maybe also
introducing a separate type like hpte_t to make this issue more
transparent and prevent future problems. But that may also require some
functional changes, and it probably won't be ready in time for Kernel
3.9. So for now, this patch only fixes the impact of the software dirty
bit changes on s390, hoping that it can be included in Kernel 3.9,
since that will be the first release including the sw dirty bits.
Gerald Schaefer (1):
mm/hugetlb: add more arch-defined huge_pte_xxx functions
arch/ia64/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/mips/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/powerpc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/s390/include/asm/hugetlb.h | 56 +++++++++++++++++++++++++++++++++++++-
arch/s390/include/asm/pgtable.h | 20 --------------
arch/s390/mm/hugetlbpage.c | 2 +-
arch/sh/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/sparc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/tile/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/x86/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
mm/hugetlb.c | 23 ++++++++--------
11 files changed, 320 insertions(+), 33 deletions(-)
--
1.7.12.4
Commit abf09bed3c "s390/mm: implement software dirty bits" introduced
another difference in the pte layout vs. the pmd layout on s390,
thoroughly breaking the s390 support for hugetlbfs. This requires
replacing some more pte_xxx functions in mm/hugetlbfs.c with a
huge_pte_xxx version.
This patch introduces those huge_pte_xxx functions and their
implementation on all architectures supporting hugetlbfs. This change
will be a no-op for all architectures other than s390.
Signed-off-by: Gerald Schaefer <[email protected]>
---
arch/ia64/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/mips/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/powerpc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/s390/include/asm/hugetlb.h | 56 +++++++++++++++++++++++++++++++++++++-
arch/s390/include/asm/pgtable.h | 20 --------------
arch/s390/mm/hugetlbpage.c | 2 +-
arch/sh/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/sparc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/tile/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
arch/x86/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
mm/hugetlb.c | 23 ++++++++--------
11 files changed, 320 insertions(+), 33 deletions(-)
diff --git a/arch/ia64/include/asm/hugetlb.h b/arch/ia64/include/asm/hugetlb.h
index 94eaa5b..716b00b 100644
--- a/arch/ia64/include/asm/hugetlb.h
+++ b/arch/ia64/include/asm/hugetlb.h
@@ -81,4 +81,40 @@ static inline void arch_clear_hugepage_flags(struct page *page)
{
}
+static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
+{
+ return mk_pte(page, pgprot);
+}
+
+static inline int huge_pte_write(pte_t pte)
+{
+ return pte_write(pte);
+}
+
+static inline int huge_pte_dirty(pte_t pte)
+{
+ return pte_dirty(pte);
+}
+
+static inline pte_t huge_pte_mkwrite(pte_t pte)
+{
+ return pte_mkwrite(pte);
+}
+
+static inline pte_t huge_pte_mkdirty(pte_t pte)
+{
+ return pte_mkdirty(pte);
+}
+
+static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
+{
+ return pte_modify(pte, newprot);
+}
+
+static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep)
+{
+ pte_clear(mm, addr, ptep);
+}
+
#endif /* _ASM_IA64_HUGETLB_H */
diff --git a/arch/mips/include/asm/hugetlb.h b/arch/mips/include/asm/hugetlb.h
index ef99db9..f134b1d 100644
--- a/arch/mips/include/asm/hugetlb.h
+++ b/arch/mips/include/asm/hugetlb.h
@@ -126,4 +126,40 @@ static inline void arch_clear_hugepage_flags(struct page *page)
{
}
+static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
+{
+ return mk_pte(page, pgprot);
+}
+
+static inline int huge_pte_write(pte_t pte)
+{
+ return pte_write(pte);
+}
+
+static inline int huge_pte_dirty(pte_t pte)
+{
+ return pte_dirty(pte);
+}
+
+static inline pte_t huge_pte_mkwrite(pte_t pte)
+{
+ return pte_mkwrite(pte);
+}
+
+static inline pte_t huge_pte_mkdirty(pte_t pte)
+{
+ return pte_mkdirty(pte);
+}
+
+static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
+{
+ return pte_modify(pte, newprot);
+}
+
+static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep)
+{
+ pte_clear(mm, addr, ptep);
+}
+
#endif /* __ASM_HUGETLB_H */
diff --git a/arch/powerpc/include/asm/hugetlb.h b/arch/powerpc/include/asm/hugetlb.h
index 62e11a3..91cb56b 100644
--- a/arch/powerpc/include/asm/hugetlb.h
+++ b/arch/powerpc/include/asm/hugetlb.h
@@ -176,4 +176,40 @@ static inline void reserve_hugetlb_gpages(void)
}
#endif
+static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
+{
+ return mk_pte(page, pgprot);
+}
+
+static inline int huge_pte_write(pte_t pte)
+{
+ return pte_write(pte);
+}
+
+static inline int huge_pte_dirty(pte_t pte)
+{
+ return pte_dirty(pte);
+}
+
+static inline pte_t huge_pte_mkwrite(pte_t pte)
+{
+ return pte_mkwrite(pte);
+}
+
+static inline pte_t huge_pte_mkdirty(pte_t pte)
+{
+ return pte_mkdirty(pte);
+}
+
+static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
+{
+ return pte_modify(pte, newprot);
+}
+
+static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep)
+{
+ pte_clear(mm, addr, ptep);
+}
+
#endif /* _ASM_POWERPC_HUGETLB_H */
diff --git a/arch/s390/include/asm/hugetlb.h b/arch/s390/include/asm/hugetlb.h
index 593753e..bd90359 100644
--- a/arch/s390/include/asm/hugetlb.h
+++ b/arch/s390/include/asm/hugetlb.h
@@ -114,7 +114,7 @@ static inline pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
#define huge_ptep_set_wrprotect(__mm, __addr, __ptep) \
({ \
pte_t __pte = huge_ptep_get(__ptep); \
- if (pte_write(__pte)) { \
+ if (huge_pte_write(__pte)) { \
huge_ptep_invalidate(__mm, __addr, __ptep); \
set_huge_pte_at(__mm, __addr, __ptep, \
huge_pte_wrprotect(__pte)); \
@@ -127,4 +127,58 @@ static inline void huge_ptep_clear_flush(struct vm_area_struct *vma,
huge_ptep_invalidate(vma->vm_mm, address, ptep);
}
+static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
+{
+ pte_t pte;
+ pmd_t pmd;
+
+ pmd = mk_pmd_phys(page_to_phys(page), pgprot);
+ pte_val(pte) = pmd_val(pmd);
+ return pte;
+}
+
+static inline int huge_pte_write(pte_t pte)
+{
+ pmd_t pmd;
+
+ pmd_val(pmd) = pte_val(pte);
+ return pmd_write(pmd);
+}
+
+static inline int huge_pte_dirty(pte_t pte)
+{
+ /* No dirty bit in the segment table entry. */
+ return 0;
+}
+
+static inline pte_t huge_pte_mkwrite(pte_t pte)
+{
+ pmd_t pmd;
+
+ pmd_val(pmd) = pte_val(pte);
+ pte_val(pte) = pmd_val(pmd_mkwrite(pmd));
+ return pte;
+}
+
+static inline pte_t huge_pte_mkdirty(pte_t pte)
+{
+ /* No dirty bit in the segment table entry. */
+ return pte;
+}
+
+static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
+{
+ pmd_t pmd;
+
+ pmd_val(pmd) = pte_val(pte);
+ pte_val(pte) = pmd_val(pmd_modify(pmd, newprot));
+ return pte;
+}
+
+static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep)
+{
+ pmd_clear((pmd_t *) ptep);
+}
+
#endif /* _ASM_S390_HUGETLB_H */
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 75b8750..dcc42bd 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -909,26 +909,6 @@ static inline pte_t pte_mkspecial(pte_t pte)
#ifdef CONFIG_HUGETLB_PAGE
static inline pte_t pte_mkhuge(pte_t pte)
{
- /*
- * PROT_NONE needs to be remapped from the pte type to the ste type.
- * The HW invalid bit is also different for pte and ste. The pte
- * invalid bit happens to be the same as the ste _SEGMENT_ENTRY_LARGE
- * bit, so we don't have to clear it.
- */
- if (pte_val(pte) & _PAGE_INVALID) {
- if (pte_val(pte) & _PAGE_SWT)
- pte_val(pte) |= _HPAGE_TYPE_NONE;
- pte_val(pte) |= _SEGMENT_ENTRY_INV;
- }
- /*
- * Clear SW pte bits, there are no SW bits in a segment table entry.
- */
- pte_val(pte) &= ~(_PAGE_SWT | _PAGE_SWX | _PAGE_SWC |
- _PAGE_SWR | _PAGE_SWW);
- /*
- * Also set the change-override bit because we don't need dirty bit
- * tracking for hugetlbfs pages.
- */
pte_val(pte) |= (_SEGMENT_ENTRY_LARGE | _SEGMENT_ENTRY_CO);
return pte;
}
diff --git a/arch/s390/mm/hugetlbpage.c b/arch/s390/mm/hugetlbpage.c
index 532525e..121089d 100644
--- a/arch/s390/mm/hugetlbpage.c
+++ b/arch/s390/mm/hugetlbpage.c
@@ -39,7 +39,7 @@ int arch_prepare_hugepage(struct page *page)
if (!ptep)
return -ENOMEM;
- pte = mk_pte(page, PAGE_RW);
+ pte_val(pte) = addr;
for (i = 0; i < PTRS_PER_PTE; i++) {
set_pte_at(&init_mm, addr + i * PAGE_SIZE, ptep + i, pte);
pte_val(pte) += PAGE_SIZE;
diff --git a/arch/sh/include/asm/hugetlb.h b/arch/sh/include/asm/hugetlb.h
index b3808c7..3a4a0fc 100644
--- a/arch/sh/include/asm/hugetlb.h
+++ b/arch/sh/include/asm/hugetlb.h
@@ -95,4 +95,40 @@ static inline void arch_clear_hugepage_flags(struct page *page)
clear_bit(PG_dcache_clean, &page->flags);
}
+static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
+{
+ return mk_pte(page, pgprot);
+}
+
+static inline int huge_pte_write(pte_t pte)
+{
+ return pte_write(pte);
+}
+
+static inline int huge_pte_dirty(pte_t pte)
+{
+ return pte_dirty(pte);
+}
+
+static inline pte_t huge_pte_mkwrite(pte_t pte)
+{
+ return pte_mkwrite(pte);
+}
+
+static inline pte_t huge_pte_mkdirty(pte_t pte)
+{
+ return pte_mkdirty(pte);
+}
+
+static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
+{
+ return pte_modify(pte, newprot);
+}
+
+static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep)
+{
+ pte_clear(mm, addr, ptep);
+}
+
#endif /* _ASM_SH_HUGETLB_H */
diff --git a/arch/sparc/include/asm/hugetlb.h b/arch/sparc/include/asm/hugetlb.h
index 7eb57d2..1caaed9 100644
--- a/arch/sparc/include/asm/hugetlb.h
+++ b/arch/sparc/include/asm/hugetlb.h
@@ -94,4 +94,40 @@ static inline void arch_clear_hugepage_flags(struct page *page)
{
}
+static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
+{
+ return mk_pte(page, pgprot);
+}
+
+static inline int huge_pte_write(pte_t pte)
+{
+ return pte_write(pte);
+}
+
+static inline int huge_pte_dirty(pte_t pte)
+{
+ return pte_dirty(pte);
+}
+
+static inline pte_t huge_pte_mkwrite(pte_t pte)
+{
+ return pte_mkwrite(pte);
+}
+
+static inline pte_t huge_pte_mkdirty(pte_t pte)
+{
+ return pte_mkdirty(pte);
+}
+
+static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
+{
+ return pte_modify(pte, newprot);
+}
+
+static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep)
+{
+ pte_clear(mm, addr, ptep);
+}
+
#endif /* _ASM_SPARC64_HUGETLB_H */
diff --git a/arch/tile/include/asm/hugetlb.h b/arch/tile/include/asm/hugetlb.h
index 0f885af..87a2d75 100644
--- a/arch/tile/include/asm/hugetlb.h
+++ b/arch/tile/include/asm/hugetlb.h
@@ -131,4 +131,40 @@ enum {
extern int huge_shift[HUGE_SHIFT_ENTRIES];
#endif
+static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
+{
+ return mk_pte(page, pgprot);
+}
+
+static inline int huge_pte_write(pte_t pte)
+{
+ return pte_write(pte);
+}
+
+static inline int huge_pte_dirty(pte_t pte)
+{
+ return pte_dirty(pte);
+}
+
+static inline pte_t huge_pte_mkwrite(pte_t pte)
+{
+ return pte_mkwrite(pte);
+}
+
+static inline pte_t huge_pte_mkdirty(pte_t pte)
+{
+ return pte_mkdirty(pte);
+}
+
+static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
+{
+ return pte_modify(pte, newprot);
+}
+
+static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep)
+{
+ pte_clear(mm, addr, ptep);
+}
+
#endif /* _ASM_TILE_HUGETLB_H */
diff --git a/arch/x86/include/asm/hugetlb.h b/arch/x86/include/asm/hugetlb.h
index bdd35db..b15f9f0 100644
--- a/arch/x86/include/asm/hugetlb.h
+++ b/arch/x86/include/asm/hugetlb.h
@@ -94,4 +94,40 @@ static inline void arch_clear_hugepage_flags(struct page *page)
{
}
+static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
+{
+ return mk_pte(page, pgprot);
+}
+
+static inline int huge_pte_write(pte_t pte)
+{
+ return pte_write(pte);
+}
+
+static inline int huge_pte_dirty(pte_t pte)
+{
+ return pte_dirty(pte);
+}
+
+static inline pte_t huge_pte_mkwrite(pte_t pte)
+{
+ return pte_mkwrite(pte);
+}
+
+static inline pte_t huge_pte_mkdirty(pte_t pte)
+{
+ return pte_mkdirty(pte);
+}
+
+static inline pte_t huge_pte_modify(pte_t pte, pgprot_t newprot)
+{
+ return pte_modify(pte, newprot);
+}
+
+static inline void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep)
+{
+ pte_clear(mm, addr, ptep);
+}
+
#endif /* _ASM_X86_HUGETLB_H */
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index c65a8a5..43425ad 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2243,10 +2243,11 @@ static pte_t make_huge_pte(struct vm_area_struct *vma, struct page *page,
pte_t entry;
if (writable) {
- entry =
- pte_mkwrite(pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
+ entry = huge_pte_mkwrite(huge_pte_mkdirty(mk_huge_pte(page,
+ vma->vm_page_prot)));
} else {
- entry = huge_pte_wrprotect(mk_pte(page, vma->vm_page_prot));
+ entry = huge_pte_wrprotect(mk_huge_pte(page,
+ vma->vm_page_prot));
}
entry = pte_mkyoung(entry);
entry = pte_mkhuge(entry);
@@ -2260,7 +2261,7 @@ static void set_huge_ptep_writable(struct vm_area_struct *vma,
{
pte_t entry;
- entry = pte_mkwrite(pte_mkdirty(huge_ptep_get(ptep)));
+ entry = huge_pte_mkwrite(huge_pte_mkdirty(huge_ptep_get(ptep)));
if (huge_ptep_set_access_flags(vma, address, ptep, entry, 1))
update_mmu_cache(vma, address, ptep);
}
@@ -2375,7 +2376,7 @@ again:
* HWPoisoned hugepage is already unmapped and dropped reference
*/
if (unlikely(is_hugetlb_entry_hwpoisoned(pte))) {
- pte_clear(mm, address, ptep);
+ huge_pte_clear(mm, address, ptep);
continue;
}
@@ -2399,7 +2400,7 @@ again:
pte = huge_ptep_get_and_clear(mm, address, ptep);
tlb_remove_tlb_entry(tlb, ptep, address);
- if (pte_dirty(pte))
+ if (huge_pte_dirty(pte))
set_page_dirty(page);
page_remove_rmap(page);
@@ -2852,7 +2853,7 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
* page now as it is used to determine if a reservation has been
* consumed.
*/
- if ((flags & FAULT_FLAG_WRITE) && !pte_write(entry)) {
+ if ((flags & FAULT_FLAG_WRITE) && !huge_pte_write(entry)) {
if (vma_needs_reservation(h, vma, address) < 0) {
ret = VM_FAULT_OOM;
goto out_mutex;
@@ -2882,12 +2883,12 @@ int hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
if (flags & FAULT_FLAG_WRITE) {
- if (!pte_write(entry)) {
+ if (!huge_pte_write(entry)) {
ret = hugetlb_cow(mm, vma, address, ptep, entry,
pagecache_page);
goto out_page_table_lock;
}
- entry = pte_mkdirty(entry);
+ entry = huge_pte_mkdirty(entry);
}
entry = pte_mkyoung(entry);
if (huge_ptep_set_access_flags(vma, address, ptep, entry,
@@ -2958,7 +2959,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
}
if (absent ||
- ((flags & FOLL_WRITE) && !pte_write(huge_ptep_get(pte)))) {
+ ((flags & FOLL_WRITE) && !huge_pte_write(huge_ptep_get(pte)))) {
int ret;
spin_unlock(&mm->page_table_lock);
@@ -3028,7 +3029,7 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
}
if (!huge_pte_none(huge_ptep_get(ptep))) {
pte = huge_ptep_get_and_clear(mm, address, ptep);
- pte = pte_mkhuge(pte_modify(pte, newprot));
+ pte = pte_mkhuge(huge_pte_modify(pte, newprot));
pte = arch_make_huge_pte(pte, vma, NULL, 0);
set_huge_pte_at(mm, address, ptep, pte);
pages++;
--
1.7.12.4
On 3/12/2013 2:48 PM, Gerald Schaefer wrote:
> Commit abf09bed3c "s390/mm: implement software dirty bits" introduced
> another difference in the pte layout vs. the pmd layout on s390,
> thoroughly breaking the s390 support for hugetlbfs. This requires
> replacing some more pte_xxx functions in mm/hugetlbfs.c with a
> huge_pte_xxx version.
>
> This patch introduces those huge_pte_xxx functions and their
> implementation on all architectures supporting hugetlbfs. This change
> will be a no-op for all architectures other than s390.
>
> [...]
>
> +static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
> +{
> + return mk_pte(page, pgprot);
> +}
Does it make sense to merge this new per-arch function with the existing per-arch arch_make_huge_pte() function? Certainly in the tile case, we could set up our "super" bit in the initial mk_huge_pte() call, and then set "young" and "huge" after that in the platform-independent caller (make_huge_pte). This would allow your change to eliminate some code as well as just introducing code :-)
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
On Tue, Mar 12, 2013 at 07:48:25PM +0100, Gerald Schaefer wrote:
> This patch introduces those huge_pte_xxx functions and their
> implementation on all architectures supporting hugetlbfs. This change
> will be a no-op for all architectures other than s390.
>
..
> arch/ia64/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/mips/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/powerpc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/s390/include/asm/hugetlb.h | 56 +++++++++++++++++++++++++++++++++++++-
> arch/s390/include/asm/pgtable.h | 20 --------------
> arch/s390/mm/hugetlbpage.c | 2 +-
> arch/sh/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/sparc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/tile/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/x86/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> mm/hugetlb.c | 23 ++++++++--------
> 11 files changed, 320 insertions(+), 33 deletions(-)
>
None of these wrappers are doing anything profound for most platforms, so
this would be a good candidate for an asm-generic/hugetlb.h (after which
s390 can continue to be special and no one else has to care).
On Wed, 13 Mar 2013 04:00:12 +0900
Paul Mundt <[email protected]> wrote:
> On Tue, Mar 12, 2013 at 07:48:25PM +0100, Gerald Schaefer wrote:
> > This patch introduces those huge_pte_xxx functions and their
> > implementation on all architectures supporting hugetlbfs. This change
> > will be a no-op for all architectures other than s390.
> >
> ..
>
> > arch/ia64/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/mips/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/powerpc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/s390/include/asm/hugetlb.h | 56 +++++++++++++++++++++++++++++++++++++-
> > arch/s390/include/asm/pgtable.h | 20 --------------
> > arch/s390/mm/hugetlbpage.c | 2 +-
> > arch/sh/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/sparc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/tile/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/x86/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > mm/hugetlb.c | 23 ++++++++--------
> > 11 files changed, 320 insertions(+), 33 deletions(-)
> >
> None of these wrappers are doing anything profound for most platforms, so
> this would be a good candidate for an asm-generic/hugetlb.h (after which
> s390 can continue to be special and no one else has to care).
Yes, that was also my first idea, but I vaguely remembered some discussion
with Andrew when I sent the original s390 hugetlb support patch (which also
went for the asm-generic approach). So I tried to dig out that thread, and
it turned out that the ugliness of ARCH_HAS_xxx actually resulted in my
original patch to be changed into removing lots of those and therefore
creating the individual arch header files, for the sake of readability and
maintainability. So I guess it would be straightforward to extend those
header files now, instead of re-introducing some of the ugliness.
See also here http://marc.info/?l=linux-kernel&m=120536577402075&w=2 and
here http://marc.info/?l=linux-kernel&m=120732788201196&w=2.
Thanks,
Gerald
On Tue, 12 Mar 2013 15:00:37 -0400
Chris Metcalf <[email protected]> wrote:
> On 3/12/2013 2:48 PM, Gerald Schaefer wrote:
> > Commit abf09bed3c "s390/mm: implement software dirty bits" introduced
> > another difference in the pte layout vs. the pmd layout on s390,
> > thoroughly breaking the s390 support for hugetlbfs. This requires
> > replacing some more pte_xxx functions in mm/hugetlbfs.c with a
> > huge_pte_xxx version.
> >
> > This patch introduces those huge_pte_xxx functions and their
> > implementation on all architectures supporting hugetlbfs. This change
> > will be a no-op for all architectures other than s390.
> >
> > [...]
> >
> > +static inline pte_t mk_huge_pte(struct page *page, pgprot_t pgprot)
> > +{
> > + return mk_pte(page, pgprot);
> > +}
>
> Does it make sense to merge this new per-arch function with the existing per-arch arch_make_huge_pte() function? Certainly in the tile case, we could set up our "super" bit in the initial mk_huge_pte() call, and then set "young" and "huge" after that in the platform-independent caller (make_huge_pte). This would allow your change to eliminate some code as well as just introducing code :-)
>
Yes, I guess there is also some potential of optimizing/eliminating
existing code. Apart from the arch_make_huge_pte() that you mentioned,
there is also a pte_mkhuge() left over, which looks like it should be
merged into the new mk_huge_pte().
But that would probably require more modifications than I'd dare to
bring up on rc3+. So the main focus of this patch is to fix the bug
on s390 with sw dirty bits before that bug appears in 3.9, and therefore
I'd like to keep it as simple as possible and w/o functional changes
on any other architecture for now.
Thanks,
Gerald
On Tue 12-03-13 19:48:26, Gerald Schaefer wrote:
> Commit abf09bed3c "s390/mm: implement software dirty bits" introduced
> another difference in the pte layout vs. the pmd layout on s390,
> thoroughly breaking the s390 support for hugetlbfs. This requires
> replacing some more pte_xxx functions in mm/hugetlbfs.c with a
> huge_pte_xxx version.
>
> This patch introduces those huge_pte_xxx functions and their
> implementation on all architectures supporting hugetlbfs. This change
> will be a no-op for all architectures other than s390.
>
> Signed-off-by: Gerald Schaefer <[email protected]>
> ---
> arch/ia64/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/mips/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/powerpc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/s390/include/asm/hugetlb.h | 56 +++++++++++++++++++++++++++++++++++++-
> arch/s390/include/asm/pgtable.h | 20 --------------
> arch/s390/mm/hugetlbpage.c | 2 +-
> arch/sh/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/sparc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/tile/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> arch/x86/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> mm/hugetlb.c | 23 ++++++++--------
> 11 files changed, 320 insertions(+), 33 deletions(-)
Ouch, this adds a lot of code that is almost same for all archs except
for some. Can we just make one common definition and define only those
that differ, please?
[...]
--
Michal Hocko
SUSE Labs
On Thu, Mar 14, 2013 at 9:14 PM, Michal Hocko <[email protected]> wrote:
> Ouch, this adds a lot of code that is almost same for all archs except
> for some. Can we just make one common definition and define only those
> that differ, please?
>
Wonder if he is the guy that added THP for s390, which was a model
work in 2012.
Hillf
On Thu, 14 Mar 2013 14:14:04 +0100
Michal Hocko <[email protected]> wrote:
> On Tue 12-03-13 19:48:26, Gerald Schaefer wrote:
> > Commit abf09bed3c "s390/mm: implement software dirty bits" introduced
> > another difference in the pte layout vs. the pmd layout on s390,
> > thoroughly breaking the s390 support for hugetlbfs. This requires
> > replacing some more pte_xxx functions in mm/hugetlbfs.c with a
> > huge_pte_xxx version.
> >
> > This patch introduces those huge_pte_xxx functions and their
> > implementation on all architectures supporting hugetlbfs. This change
> > will be a no-op for all architectures other than s390.
> >
> > Signed-off-by: Gerald Schaefer <[email protected]>
> > ---
> > arch/ia64/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/mips/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/powerpc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/s390/include/asm/hugetlb.h | 56 +++++++++++++++++++++++++++++++++++++-
> > arch/s390/include/asm/pgtable.h | 20 --------------
> > arch/s390/mm/hugetlbpage.c | 2 +-
> > arch/sh/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/sparc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/tile/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > arch/x86/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > mm/hugetlb.c | 23 ++++++++--------
> > 11 files changed, 320 insertions(+), 33 deletions(-)
>
> Ouch, this adds a lot of code that is almost same for all archs except
> for some. Can we just make one common definition and define only those
> that differ, please?
> [...]
Ok, seems like I misinterpreted the ugliness of HAVE_ARCH_xxx vs. code
duplication. Paul Mundt also suggested going for an asm-generic/hugetlb.h
approach. I'll send a new patch soon.
Thanks,
Gerald
On Thu 14-03-13 15:11:09, Gerald Schaefer wrote:
> On Thu, 14 Mar 2013 14:14:04 +0100
> Michal Hocko <[email protected]> wrote:
>
> > On Tue 12-03-13 19:48:26, Gerald Schaefer wrote:
> > > Commit abf09bed3c "s390/mm: implement software dirty bits" introduced
> > > another difference in the pte layout vs. the pmd layout on s390,
> > > thoroughly breaking the s390 support for hugetlbfs. This requires
> > > replacing some more pte_xxx functions in mm/hugetlbfs.c with a
> > > huge_pte_xxx version.
> > >
> > > This patch introduces those huge_pte_xxx functions and their
> > > implementation on all architectures supporting hugetlbfs. This change
> > > will be a no-op for all architectures other than s390.
> > >
> > > Signed-off-by: Gerald Schaefer <[email protected]>
> > > ---
> > > arch/ia64/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > > arch/mips/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > > arch/powerpc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > > arch/s390/include/asm/hugetlb.h | 56 +++++++++++++++++++++++++++++++++++++-
> > > arch/s390/include/asm/pgtable.h | 20 --------------
> > > arch/s390/mm/hugetlbpage.c | 2 +-
> > > arch/sh/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > > arch/sparc/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > > arch/tile/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > > arch/x86/include/asm/hugetlb.h | 36 ++++++++++++++++++++++++
> > > mm/hugetlb.c | 23 ++++++++--------
> > > 11 files changed, 320 insertions(+), 33 deletions(-)
> >
> > Ouch, this adds a lot of code that is almost same for all archs except
> > for some. Can we just make one common definition and define only those
> > that differ, please?
> > [...]
>
> Ok, seems like I misinterpreted the ugliness of HAVE_ARCH_xxx vs. code
> duplication. Paul Mundt also suggested going for an asm-generic/hugetlb.h
> approach. I'll send a new patch soon.
Oh, I have missed that answer so sorry for duplication. Anyway, having
asm-generic and HAVE_ARCH_xxx sounds much better to me for this case as
the duplication is really big.
Thanks
--
Michal Hocko
SUSE Labs