RE: [patch] fix ia64 hugetlb_free_pgd_range

RE: [patch] fix ia64 hugetlb_free_pgd_range

[Chen, Kenneth W]

David Gibson wrote on Thursday, February 23, 2006 8:06 PM
> But I don't see how not transforming them sometimes can be correct.
> Suppose 'floor' is only a little way below 'addr' - addr will be
> shifted down, but floor won't, so floor may now be above addr, which
> will cause weird results.
> 
> Afaict the *only* thing floor and ceiling are used for is bounds
> checking the address range we're examining.  How can that ever be
> right if one address has been scaled down, but the other hasn't.

The scale down isn't exactly on every address bits.  Top 3 bits of
virtual address are preserved.

#define htlbpage_to_page(x)    (((unsigned long) REGION_NUMBER(x) << 61)
                                 | (REGION_OFFSET(x) >>
(HPAGE_SHIFT-PAGE_SHIFT)))

So scaled address for a hugetlb address will never be below unscaled
normal page address.  That is adjusted addr will never below unchanged
floor.

- Ken

Re: [patch] fix ia64 hugetlb_free_pgd_range

[David Gibson]

On Thu, Feb 23, 2006 at 10:30:39PM -0800, Chen, Kenneth W wrote:
> David Gibson wrote on Thursday, February 23, 2006 8:06 PM
> > But I don't see how not transforming them sometimes can be correct.
> > Suppose 'floor' is only a little way below 'addr' - addr will be
> > shifted down, but floor won't, so floor may now be above addr, which
> > will cause weird results.
> > 
> > Afaict the *only* thing floor and ceiling are used for is bounds
> > checking the address range we're examining.  How can that ever be
> > right if one address has been scaled down, but the other hasn't.
> 
> The scale down isn't exactly on every address bits.  Top 3 bits of
> virtual address are preserved.

Ah.. yes.

> #define htlbpage_to_page(x)    (((unsigned long) REGION_NUMBER(x) << 61)
>                                  | (REGION_OFFSET(x) >>
> (HPAGE_SHIFT-PAGE_SHIFT)))
> 
> So scaled address for a hugetlb address will never be below unscaled
> normal page address.  That is adjusted addr will never below unchanged
> floor.

Ok.  So in fact it wouldn't matter whether or not addresses outside
the region are scaled, but conceptually they probably should be.

-- 
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

[patch] fix ia64 hugetlb_free_pgd_range

[Chen, Kenneth W]

I've looked at hugetlb_free_pgd_range() right side up, right side
down, up side up, up side down.  And it just doesn't look correct
to me at all.

In that function, we do address transformation before calling
free_pgd_range, so the generic function can traverse to right set
of page table page.  There is no need to do any range check.

Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>

--- ./arch/ia64/mm/hugetlbpage.c.orig	2006-02-23 18:21:28.202422392 -0800
+++ ./arch/ia64/mm/hugetlbpage.c	2006-02-23 18:26:28.256129654 -0800
@@ -125,9 +125,9 @@ void hugetlb_free_pgd_range(struct mmu_g
 
 	addr = htlbpage_to_page(addr);
 	end  = htlbpage_to_page(end);
-	if (is_hugepage_only_range(tlb->mm, floor, HPAGE_SIZE))
+	if (REGION_NUMBER(floor) == RGN_HPAGE)
 		floor = htlbpage_to_page(floor);
-	if (is_hugepage_only_range(tlb->mm, ceiling, HPAGE_SIZE))
+	if (REGION_NUMBER(ceiling) == RGN_HPAGE)
 		ceiling = htlbpage_to_page(ceiling);
 
 	free_pgd_range(tlb, addr, end, floor, ceiling);

Re: [patch] fix ia64 hugetlb_free_pgd_range

['David Gibson']

On Thu, Feb 23, 2006 at 05:45:14PM -0800, Chen, Kenneth W wrote:
> I've looked at hugetlb_free_pgd_range() right side up, right side
> down, up side up, up side down.  And it just doesn't look correct
> to me at all.
> 
> In that function, we do address transformation before calling
> free_pgd_range, so the generic function can traverse to right set
> of page table page.  There is no need to do any range check.

I think you're right - the range check is bogus.  Probably someone
just used it because the range check gave the right answer - but only
by accident in a sense.

However... I suspect in fact that the transformations should be
unconditional.  The whole address scaling thing that ia64 does for
hugepages is extremely confusing, but since floor and ceiling are just
used for bounds checking on the inner functions, shouldn't they be
transformed to the same scale as addr and end, even if that's not
actually a true address (hugepage or otherwise).

-- 
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

RE: [patch] fix ia64 hugetlb_free_pgd_range

[Chen, Kenneth W]

David Gibson wrote on Thursday, February 23, 2006 6:45 PM
> However... I suspect in fact that the transformations should be
> unconditional.

No, that won't be correct.


> The whole address scaling thing that ia64 does for
> hugepages is extremely confusing, but since floor and ceiling are just
> used for bounds checking on the inner functions, shouldn't they be
> transformed to the same scale as addr and end, even if that's not
> actually a true address (hugepage or otherwise).

Think of multiple page size support, the way we do it on ia64 is
effective have different PAGE_SHIFT for normal page and hugetlb
page.  Normal page has page shift of 14 bits (of 16K page size).
Hugetlb page has page shift of 28 bits (of 256MB page size).
For any address, regardless whether it is normal/hugetlb page,
they all have full 3 level (or 4 level page table).  So for hugetlb
address, pgd/pud/pmd/pte index are off by (28-14) bits compare
to normal page.  We just transform the address so all the generic
code can be applied.  It's sort of selling you a wood looking
vinyl car stick shift.

- Ken

Re: [patch] fix ia64 hugetlb_free_pgd_range

['David Gibson']

On Thu, Feb 23, 2006 at 07:05:23PM -0800, Chen, Kenneth W wrote:
> David Gibson wrote on Thursday, February 23, 2006 6:45 PM
> > However... I suspect in fact that the transformations should be
> > unconditional.
> 
> No, that won't be correct.

But I don't see how not transforming them sometimes can be correct.
Suppose 'floor' is only a little way below 'addr' - addr will be
shifted down, but floor won't, so floor may now be above addr, which
will cause weird results.

Afaict the *only* thing floor and ceiling are used for is bounds
checking the address range we're examining.  How can that ever be
right if one address has been scaled down, but the other hasn't.

-- 
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