On Fri, Mar 02, 2007 at 03:46:07PM -0800, Nishanth Aravamudan wrote:
> On 02.03.2007 [18:24:26 +1100], David Gibson wrote:
> > On Wed, Feb 28, 2007 at 08:22:36AM -0800, Nishanth Aravamudan wrote:
> > > On 27.02.2007 [10:47:31 +1100], David Gibson wrote:
> > > > On Mon, Feb 26, 2007 at 03:21:26PM -0800, Nishanth Aravamudan wrote:
> > > > > Author: Nishanth Aravamudan <[EMAIL PROTECTED]>
> > > > > Date: Mon Feb 26 15:17:54 2007 -0800
> > > > >
> > > > > hugeutils: add address parameter to hugetlbfs_vaddr_granularity
> > > > >
> > > > > On ppc64, the actual granularity of hugepage requests depends on the
> > > > > address in question. Specify the three cases (0-4G, 4G-1TB, 1TB+) in
> > > > > the
> > > > > function. This is useful for the partial_remap code, as it would
> > > > > (effectively) never run on ppc64 otherwise (would need a 1TB or larger
> > > > > segment, without relinking).
> > > >
> > > > I've thought about doing something like this before. However, the
> > > > semantics as they stand are a little bit fuzzy. What I'd prefer, if
> > > > you want to get finer-grained information about the page-size regions
> > > > is a pair of functions to find the end and beginning of a page-size
> > > > slice, given an address. So:
> > > >
> > > > void *hugetlbfs_slice_end(void *addr);
> > > > void *hugetlbfs_slice_start(void *addr);
> > > >
> > > > The "slice" terminology is something BenH has started using for these
> > > > address space regionsin some recent kernel patches.
> > >
> > > I'm willing to believe I'm doing something incredibly stupid, but here's
> > > my patch. I had to use a bunch of macros, as I was getting a headache
> > > from all the casts and the constants. How do I make this cleaner? Or do
> > > I give up on this?
> >
> > Well, for starters, you want to get rid of
> > hugetlbfs_vaddr_granularity() entirely.
>
> <snip>
>
> > Except.. I think hugetlbfs_slice_end() will actually need to return
> > the end of the slice, not the beginning of the next one as it does
> > now. I don't like that semantic on the whole, but without it, for
> > 32-bit programs hugetlbfs_slice_end() will return 0 for sufficiently
> > high addresses. I'd prefer to reserve the case where
> > hugetlbfs_slice_end(adde) < addr to mean that the given address is
> > already outside the potentially valid range for hugepage addresses (so
> > if we ever support ia64, slice_start() and slice_end() end would
> > return the constant addresses marking the ends of their fixed hugepage
> > region).
>
> Thanks for the review David. I'm not entirely sure about the semantics
> still. I got what you were saying, but wasn't sure of the best way to
> implement it.
>
> So does something like the following look better?
>
> Signed-off-by: Nishanth Aravamudan <[EMAIL PROTECTED]>
>
> diff --git a/hugetlbfs.h b/hugetlbfs.h
> index 966e57c..d0ad9a2 100644
> --- a/hugetlbfs.h
> +++ b/hugetlbfs.h
> @@ -22,7 +22,8 @@
> #define HUGETLBFS_MAGIC 0x958458f6
>
> long gethugepagesize(void);
> -long hugetlbfs_vaddr_granularity(void);
> +void *hugetlbfs_slice_start(void *);
> +void *hugetlbfs_slice_end(void *);
Come to think of it, since the sole user of these functions is working
in terms of unsigned long, not void *, we should probably make these
functions work accordingly.
> int hugetlbfs_test_path(const char *mount);
> const char *hugetlbfs_find_path(void);
> int hugetlbfs_unlinked_fd(void);
> diff --git a/hugeutils.c b/hugeutils.c
> index 9af92b9..d8fb679 100644
> --- a/hugeutils.c
> +++ b/hugeutils.c
> @@ -114,14 +114,36 @@ long gethugepagesize(void)
> return hpage_size;
> }
>
> -long hugetlbfs_vaddr_granularity(void)
> -{
> +/*
> + * Return the address of the start and end of the hugetlb slice
> + * containing @addr. A slice is a range of addresses, start inclusive
> + * and end exclusive.
> + */
> +void *hugetlbfs_slice_start(void *addr) {
> #if defined(__powerpc64__)
> - return (1L << 40);
> + if (addr < (void *)SLICE_LOW_TOP)
> + return ALIGN_PTR_DOWN(addr, SLICE_LOW_SIZE);
> + else if (addr < (void *)SLICE_HIGH_SIZE)
> + return (void *)SLICE_LOW_TOP;
> + else
> + return ALIGN_PTR_DOWN(addr, SLICE_HIGH_SIZE);
> +#elif defined(__powerpc__)
> + return ALIGN_PTR_DOWN(addr, SLICE_LOW_SIZE);
> +#else
> + return ALIGN_PTR_DOWN(addr, gethugepagesize());
> +#endif
> +}
> +
> +void *hugetlbfs_slice_end(void *addr) {
> +#if defined(__powerpc64__)
> + if (addr < (void *)SLICE_LOW_TOP)
> + return ALIGN_PTR_UP(addr, SLICE_LOW_SIZE) - 1;
> + else
> + return ALIGN_PTR_UP(addr, SLICE_HIGH_SIZE) - 1;
> #elif defined(__powerpc__)
> - return (1L << 28);
> + return ALIGN_PTR_UP(addr, SLICE_LOW_SIZE) - 1;
Yes, I think these implement the semantics I meant. Although if we
change to unsigned long throughout, we get the added bonus of not
needing new pointerized versions of the ALIGN() macros.
> #else
> - return gethugepagesize();
> + return ALIGN_PTR_UP(addr, gethugepagesize()) - 1;
> #endif
> }
>
> diff --git a/libhugetlbfs_internal.h b/libhugetlbfs_internal.h
> index 40da66a..8e5f6fe 100644
> --- a/libhugetlbfs_internal.h
> +++ b/libhugetlbfs_internal.h
> @@ -26,7 +26,18 @@
> #define stringify_1(x) #x
> #define stringify(x) stringify_1(x)
>
> -#define ALIGN(x, a) (((x) + (a) - 1) & ~((a) - 1))
> +#define SLICE_LOW_SHIFT 28
> +#define SLICE_HIGH_SHIFT 40
> +
> +#define SLICE_LOW_TOP (0x100000000UL)
> +#define SLICE_LOW_SIZE (1UL << SLICE_LOW_SHIFT)
> +#define SLICE_HIGH_SIZE (1UL << SLICE_HIGH_SHIFT)
> +
> +#define ALIGN_UP(x, a) (((x) + (a) - 1) & ~((a) - 1))
> +#define ALIGN_DOWN(x, a) ((x) & ~((a) - 1))
> +#define ALIGN(x,a) ALIGN_UP(x,a)
> +#define ALIGN_PTR_UP(x, a) (void *)ALIGN_UP((unsigned long)x, a)
> +#define ALIGN_PTR_DOWN(x,a) (void *)ALIGN_DOWN((unsigned long)x, a)
>
> extern int __hugetlbfs_verbose;
>
> diff --git a/morecore.c b/morecore.c
> index 25c2d27..ba4fcb0 100644
> --- a/morecore.c
> +++ b/morecore.c
> @@ -166,7 +166,7 @@ static void __attribute__((constructor))
> setup_morecore(void)
> }
> } else {
> heapaddr = (unsigned long)sbrk(0);
> - heapaddr = ALIGN(heapaddr, hugetlbfs_vaddr_granularity());
> + heapaddr = ALIGN(heapaddr, (unsigned
> long)hugetlbfs_slice_end((void *)heapaddr) + 1);
You don't need the ALIGN() here any more. What this is trying to do
is to move heapaddr into the next slice from where it started. So you
just need:
heapaddr = hugetlbfs_slice_end(heapaddr) + 1;
> }
>
> DEBUG("setup_morecore(): heapaddr = 0x%lx\n", heapaddr);
>
--
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
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Libhugetlbfs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
