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.
> Signed-off-by: Nishanth Aravamudan <[EMAIL PROTECTED]>
>
> diff --git a/hugetlbfs.h b/hugetlbfs.h
> index 966e57c..1a5d8b7 100644
> --- a/hugetlbfs.h
> +++ b/hugetlbfs.h
> @@ -22,7 +22,9 @@
> #define HUGETLBFS_MAGIC 0x958458f6
>
> long gethugepagesize(void);
> -long hugetlbfs_vaddr_granularity(void);
> +void *hugetlbfs_slice_start(void *);
> +void *hugetlbfs_slice_end(void *);
> +long hugetlbfs_vaddr_granularity(void *addr);
> 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..c030daf 100644
> --- a/hugeutils.c
> +++ b/hugeutils.c
> @@ -114,12 +114,68 @@ long gethugepagesize(void)
> return hpage_size;
> }
>
> -long hugetlbfs_vaddr_granularity(void)
> +#define twofiftysixMB (1UL << 28)
> +#define fourGB (1UL << 32)
> +#define oneTB (1UL << 40)
I'd suggest getting rid of fourGB entirely, just use 0x100000000 as a
literal. The other two I'd call, SEGMENT_SIZE and HIGH_SLICE_SIZE or
something similar, rather than naming them by value. Check for the
names of similar constants in BenH's slice patch for the kernel.
> +#define ALIGN_PTR_DOWN(x, a) \
> + ((void *)((unsigned long)(x) & ~((a) - 1)))
> +#define ALIGN_PTR_UP(x, a) \
> + ((void *)(((unsigned long)(x) + (a) - 1) & ~((a) - 1)))
There's already an ALIGN() macro in libhugetlbfs_internal.h. We can
extend that to add align down, and pointer typed versions.
> +/*
> + * 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__)
> + if (addr < (void *)fourGB)
> + return ALIGN_PTR_DOWN(addr, twofiftysixMB);
> + else if (addr < (void *)oneTB)
> + return (void *)fourGB;
> + else
> + return ALIGN_PTR_DOWN(addr, oneTB);
> +#elif defined(__powerpc__)
> + return ALIGN_PTR_DOWN(addr, twofiftysixMB);
> +#else
> + long hpage_size = gethugepagesize();
> + if (hpage_size)
> + return ALIGN_PTR_DOWN(addr, hpage_size);
> + else
> + return NULL;
No need for the if here, I think. If we get to this point with
gethugepagesize() returning zero, we're pretty much completely stuffed
anyway. Plus, NULL is not really good as an error return value,
because it's correct and valid in some circumstances (like for any
address below 256MB on powerpc).
> +#endif
> +}
> +
> +void *hugetlbfs_slice_end(void *addr) {
> +#if defined(__powerpc64__)
> + if (addr < (void *)fourGB)
> + return ALIGN_PTR_UP(addr, twofiftysixMB);
> + else if (addr < (void *)oneTB);
> + return (void *)oneTB;
Middle case is redundant here, ALIGN_UP(.., 1TB) will give the correct
value for things between 4GB and 1TB.
> + else
> + return ALIGN_PTR_UP(addr, oneTB);
> +#elif defined(__powerpc__)
> + return ALIGN_PTR_UP(addr, twofiftysixMB);
> +#else
> + long hpage_size = gethugepagesize();
> + if (hpage_size)
> + return ALIGN_PTR_UP(addr, hpage_size);
> + else
> + return NULL;
> +#endif
> +}
> +
> +long hugetlbfs_vaddr_granularity(void *addr)
> {
> #if defined(__powerpc64__)
> - return (1L << 40);
> + if (addr < (void *)fourGB)
> + return twofiftysixMB;
> + else if (addr < (void *)oneTB)
> + return oneTB - fourGB;
> + else
> + return 1TB;
> #elif defined(__powerpc__)
> - return (1L << 28);
> + return twofiftysixMB;
> #else
> return gethugepagesize();
> #endif
Kill this old interface off entirely.
> diff --git a/morecore.c b/morecore.c
> index 25c2d27..7efb174 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, hugetlbfs_vaddr_granularity((void
> *)heapaddr));
And here you just want:
heapaddr = hugetlbfs_slice_end(heapaddr);
No need for the granularity function.
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).
--
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
