On Sun, May 05, 2013 at 12:44:35AM +0530, Aneesh Kumar K.V wrote:
> David Gibson <dwg@xxxxxxxxxxx> writes:
> > On Mon, Apr 29, 2013 at 01:21:43AM +0530, Aneesh Kumar K.V wrote:
> >> From: "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
[snip]
> >> diff --git a/arch/powerpc/include/asm/pgtable-ppc64.h b/arch/powerpc/include/asm/pgtable-ppc64.h
> >> index ab84332..20133c1 100644
> >> --- a/arch/powerpc/include/asm/pgtable-ppc64.h
> >> +++ b/arch/powerpc/include/asm/pgtable-ppc64.h
> >> @@ -154,7 +154,7 @@
> >> #define pmd_present(pmd) (pmd_val(pmd) != 0)
> >> #define pmd_clear(pmdp) (pmd_val(*(pmdp)) = 0)
> >> #define pmd_page_vaddr(pmd) (pmd_val(pmd) & ~PMD_MASKED_BITS)
> >> -#define pmd_page(pmd) virt_to_page(pmd_page_vaddr(pmd))
> >> +extern struct page *pmd_page(pmd_t pmd);
> >>
> >> #define pud_set(pudp, pudval) (pud_val(*(pudp)) = (pudval))
> >> #define pud_none(pud) (!pud_val(pud))
> >> @@ -382,4 +382,261 @@ static inline pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea,
> >>
> >> #endif /* __ASSEMBLY__ */
> >>
> >> +#ifndef _PAGE_SPLITTING
> >> +/*
> >> + * THP pages can't be special. So use the _PAGE_SPECIAL
> >> + */
> >> +#define _PAGE_SPLITTING _PAGE_SPECIAL
> >> +#endif
> >> +
> >> +#ifndef _PAGE_THP_HUGE
> >> +/*
> >> + * We need to differentiate between explicit huge page and THP huge
> >> + * page, since THP huge page also need to track real subpage details
> >> + * We use the _PAGE_COMBO bits here as dummy for platform that doesn't
> >> + * support THP.
> >> + */
> >> +#define _PAGE_THP_HUGE 0x10000000
> >
> > So if it's _PAGE_COMBO, use _PAGE_COMBO, instead of the actual number.
>
> We define _PAGE_THP_HUGE value in pte-hash64-64k.h. Now the functions
> below which depends on _PAGE_THP_HUGE are in pgtable-ppc64.h. The above
> #define takes care of compile errors on subarch that doesn't include
> pte-hash64-64k.h We really won't be using these functions at run time,
> because we will not find a transparent huge page on those subarchs.
Nonetheless, duplicated definitions really won't do.
[snip]
> >> +#endif
> >> +
> >> +#define HUGE_PAGE_SIZE (ASM_CONST(1) << 24)
> >> +#define HUGE_PAGE_MASK (~(HUGE_PAGE_SIZE - 1))
> >
> > These constants should be named so its clear they're THP specific.
> > They should also be defined in terms of PMD_SHIFT, instead of
> > directly.
>
> I was not able to use HPAGE_PMD_SIZE because we have that BUILD_BUG_ON
> when THP is not enabled. I will switch them to PMD_SIZE and PMD_MASK ?
That would be ok. THP_PAGE_SIZE or something would also be fine.
[snip]
> >> +static inline unsigned long pmd_pfn(pmd_t pmd)
> >> +{
> >> + /*
> >> + * Only called for hugepage pmd
> >> + */
> >> + return pmd_val(pmd) >> PTE_RPN_SHIFT;
> >> +}
> >> +
> >> +/* We will enable it in the last patch */
> >> +#define has_transparent_hugepage() 0
> >> +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
> >> +
> >> +static inline int pmd_young(pmd_t pmd)
> >> +{
> >> + return pmd_val(pmd) & _PAGE_ACCESSED;
> >> +}
> >
> > It would be clearer to define this function as well as various others
> > that operate on PMDs as PTEs to just cast the parameter and call the
> > corresponding pte_XXX(),
>
> I did what tile arch is done. How about
>
> +#define pmd_pte(pmd) (pmd)
> +#define pte_pmd(pte) (pte)
> +#define pmd_pfn(pmd) pte_pfn(pmd_pte(pmd))
> +#define pmd_young(pmd) pte_young(pmd_pte(pmd))
> +#define pmd_mkold(pmd) pte_pmd(pte_mkold(pmd_pte(pmd)))
> +#define pmd_wrprotect(pmd) pte_pmd(pte_wrprotect(pmd_pte(pmd)))
> +#define pmd_mkdirty(pmd) pte_pmd(pte_mkdirty(pmd_pte(pmd)))
> +#define pmd_mkyoung(pmd) pte_pmd(pte_mkyoung(pmd_pte(pmd)))
> +#define pmd_mkwrite(pmd) pte_pmd(pte_mkwrite(pmd_pte(pmd)))
Probably better for pmd_pte() and pte_pmd() to be inlines, so you
preserve type checking (at least with STRICT_MM_TYPECHECKS), but
otherwise looks ok.
[snip]
> >> +/*
> >> + * We want to put the pgtable in pmd and use pgtable for tracking
> >> + * the base page size hptes
> >> + */
> >> +void pgtable_trans_huge_deposit(struct mm_struct *mm, pmd_t *pmdp,
> >> + pgtable_t pgtable)
> >> +{
> >> + unsigned long *pgtable_slot;
> >> + assert_spin_locked(&mm->page_table_lock);
> >> + /*
> >> + * we store the pgtable in the second half of PMD
> >> + */
> >> + pgtable_slot = pmdp + PTRS_PER_PMD;
> >> + *pgtable_slot = (unsigned long)pgtable;
> >
> > Why not just make pgtable_slot have type (pgtable_t *) and avoid the
> > case.
> >
>
> done. But we would have cast in the above line.
Sure. But in fact the above line would need a cast anyway, if you
turned on STRICT_MM_TYPECHECKS.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: Digital signature