On Mon, Mar 04, 2013 at 05:11:53PM +0530, Aneesh Kumar K.V wrote:
> Paul Mackerras <paulus@xxxxxxxxx> writes:
> >> +static inline int hpte_actual_psize(struct hash_pte *hptep, int psize)
> >> +{
> >> + unsigned int mask;
> >> + int i, penc, shift;
> >> + /* Look at the 8 bit LP value */
> >> + unsigned int lp = (hptep->r >> LP_SHIFT) & ((1 << LP_BITS) - 1);
> >> +
> >> + penc = 0;
> >> + for (i = 0; i < MMU_PAGE_COUNT; i++) {
> >> + /* valid entries have a shift value */
> >> + if (!mmu_psize_defs[i].shift)
> >> + continue;
> >> +
> >> + /* encoding bits per actual page size */
> >> + shift = mmu_psize_defs[i].shift - 11;
> >> + if (shift > 9)
> >> + shift = 9;
> >> + mask = (1 << shift) - 1;
> >> + if ((lp & mask) == mmu_psize_defs[psize].penc[i])
> >> + return i;
> >> + }
> >> + return -1;
> >> +}
> >
> > This doesn't look right to me. First, it's not clear what the 11 and
> > 9 refer to, and I think the 9 should be LP_BITS (i.e. 8). Secondly,
> > the mask for the comparison needs to depend on the actual page size
> > not the base page size.
>
> That 11 should be 12.That depends on the fact that we have below mapping
And the 12 should be LP_SHIFT, shouldn't it?
> rrrr rrrz ≥8KB
>
> Yes, that 9 should be LP_BITs.
>
> We are generating mask based on actual page size above (variable i in
> the for loop).
OK, yes, you're right.
> > I don't see where in this function you set the penc[] elements for
> > invalid actual page sizes to -1.
>
> We do the below
>
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -125,7 +125,7 @@ static struct mmu_psize_def mmu_psize_defaults_old[] = {
> [MMU_PAGE_4K] = {
> .shift = 12,
> .sllp = 0,
> - .penc = 0,
> + .penc = { [0 ... MMU_PAGE_COUNT - 1] = -1 },
> .avpnm = 0,
Yes, which sets them for the entries you initialize, but not for the
others. For example, the entry for MMU_PAGE_64K will initially be all
zeroes. Then we find an entry in the ibm,segment-page-sizes property
for 64k pages, so we set mmu_psize_defs[MMU_PAGE_64K].shift to 16,
making that entry valid, but we never set any of the .penc[] entries
to -1, leading your other code to think that it can do (say) 1M pages
in a 64k segment using an encoding of 0.
Also, I noticed that the code in the if (base_idx < 0) statement is
wrong. It needs to advance prop (and decrease size) by 2 * lpnum,
not just 2.
Paul.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>