On Sun, May 05, 2013 at 12:44:35AM +0530, Aneesh Kumar K.V wrote: > David Gibson <[email protected]> writes: > > On Mon, Apr 29, 2013 at 01:21:43AM +0530, Aneesh Kumar K.V wrote: > >> From: "Aneesh Kumar K.V" <[email protected]> [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
signature.asc
Description: Digital signature
_______________________________________________ Linuxppc-dev mailing list [email protected] https://lists.ozlabs.org/listinfo/linuxppc-dev
