On Wed, Apr 10, 2013 at 01:41:16PM +0530, Aneesh Kumar K.V wrote:
> David Gibson <dwg@xxxxxxxxxxx> writes:
>
> > On Thu, Apr 04, 2013 at 11:27:46AM +0530, Aneesh Kumar K.V wrote:
> >> From: "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> >>
> >> We look at both the segment base page size and actual page size and store
> >> the pte-lp-encodings in an array per base page size.
> >>
> >> We also update all relevant functions to take actual page size argument
> >> so that we can use the correct PTE LP encoding in HPTE. This should also
> >> get the basic Multiple Page Size per Segment (MPSS) support. This is needed
> >> to enable THP on ppc64.
> >>
>
> ....
>
> >> +static inline int hpte_actual_psize(struct hash_pte *hptep, int psize)
> >> +{
> >> + int i, shift;
> >> + unsigned int mask;
> >> + /* Look at the 8 bit LP value */
> >> + unsigned int lp = (hptep->r >> LP_SHIFT) & ((1 << LP_BITS) - 1);
> >> +
> >> + if (!(hptep->v & HPTE_V_VALID))
> >> + return -1;
> >
> > Folding the validity check into the size check seems confusing to me.
>
> We do end up with invalid hpte with which we call
> hpte_actual_psize. So that check is needed. I can either move to caller,
> but then i will have to replicate it in all the call sites.
>
>
> >> + /* First check if it is large page */
> >> + if (!(hptep->v & HPTE_V_LARGE))
> >> + return MMU_PAGE_4K;
> >> +
> >> + /* start from 1 ignoring MMU_PAGE_4K */
> >> + for (i = 1; i < MMU_PAGE_COUNT; i++) {
> >> + /* valid entries have a shift value */
> >> + if (!mmu_psize_defs[i].shift)
> >> + continue;
> >
> > Isn't this check redundant with the one below?
>
> Yes. I guess we can safely assume that if penc is valid then we do
> support that specific large page.
>
> I will drop this and keep the penc check. That is more correct check
>
> >> + /* invalid penc */
> >> + if (mmu_psize_defs[psize].penc[i] == -1)
> >> + continue;
> >> + /*
> >> + * encoding bits per actual page size
> >> + * PTE LP actual page size
> >> + * rrrr rrrz >=8KB
> >> + * rrrr rrzz >=16KB
> >> + * rrrr rzzz >=32KB
> >> + * rrrr zzzz >=64KB
> >> + * .......
> >> + */
> >> + shift = mmu_psize_defs[i].shift - LP_SHIFT;
> >> + if (shift > LP_BITS)
> >> + shift = LP_BITS;
> >> + mask = (1 << shift) - 1;
> >> + if ((lp & mask) == mmu_psize_defs[psize].penc[i])
> >> + return i;
> >> + }
> >
> > Shouldn't we have a BUG() or something here. If we get here we've
> > somehow created a PTE with LP bits we can't interpret, yes?
>
> I don't know. Is BUG() the right thing to do ?
Well, it's a situation that should never occur, and it's not clear
what we can do to fix it if it does, so, yeah, I think BUG() is appropriate.
> >> + return -1;
> >> +}
> >> +
> >> static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> >> unsigned long vpn, int psize, int ssize,
> >> int local)
> >> @@ -251,6 +294,7 @@ static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> >> struct hash_pte *hptep = htab_address + slot;
> >> unsigned long hpte_v, want_v;
> >> int ret = 0;
> >> + int actual_psize;
> >>
> >> want_v = hpte_encode_avpn(vpn, psize, ssize);
> >>
> >> @@ -260,9 +304,13 @@ static long native_hpte_updatepp(unsigned long slot, unsigned long newpp,
> >> native_lock_hpte(hptep);
> >>
> >> hpte_v = hptep->v;
> >> -
> >> + actual_psize = hpte_actual_psize(hptep, psize);
> >> + if (actual_psize < 0) {
> >> + native_unlock_hpte(hptep);
> >> + return -1;
> >> + }
> >
> > Wouldn't it make more sense to only do the psize lookup once you've
> > found a matching hpte?
>
> But we need to do psize lookup even if V_COMPARE fail, because we want
> to do tlbie in both the case.
Ah, yes. Sorry, misunderstood what this code was doing.
[snip]
> >> @@ -388,19 +444,26 @@ static void hpte_decode(struct hash_pte *hpte, unsigned long slot,
> >> penc = LP_MASK(i+1) >> LP_SHIFT;
> >> for (size = 0; size < MMU_PAGE_COUNT; size++) {
> >
> >>
> >> - /* 4K pages are not represented by LP */
> >> - if (size == MMU_PAGE_4K)
> >> - continue;
> >> -
> >> /* valid entries have a shift value */
> >> if (!mmu_psize_defs[size].shift)
> >> continue;
> >> + for (a_size = 0; a_size < MMU_PAGE_COUNT; a_size++) {
> >
> > Can't you resize hpte_actual_psize() here instead of recoding the
> > lookup?
>
> I thought about that, but re-coding avoided some repeated check. But
> then, if I follow your review comments of avoiding hpte valid check etc, may
> be I can reuse the hpte_actual_psize. Will try this.
hpte_decode() is only used in the kexec() path so some repeated simple
tests don't really matter.
--
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