On Thu, Nov 06, 2008 at 10:49:35AM +0000, Mel Gorman wrote: > On Thu, Nov 06, 2008 at 12:06:23PM +1100, David Gibson wrote: > > On Thu, Nov 06, 2008 at 12:51:54AM +0000, Mel Gorman wrote: > > > On Thu, Nov 06, 2008 at 10:15:54AM +1100, David Gibson wrote: > > > > On Wed, Nov 05, 2008 at 09:43:40AM -0600, Adam Litke wrote: > > > > > On Wed, 2008-11-05 at 14:02 +0000, Mel Gorman wrote: > > > > > > @@ -127,6 +140,34 @@ void *get_huge_pages(size_t len, ghp_t flags) > > > > > > return NULL; > > > > > > } > > > > > > > > > > > > +offset: > > > > > > > > > > This cache-coloring code (below) is nice. Could we move it into it's > > > > > own function though? I like the idea of caching the sysconf value, > > > > > but > > > > > perhaps you could do that as a static variable within the new > > > > > function. > > > > > > > > Yeah, I don't think this belongs in get_huge_pages(). > > > > get_huge_pages() is supposed to be the "raw" low-level interface to > > > > just grab some hugepages. Offsetting within the page should be up to > > > > the caller, if it cares. > > > > > > > > > > Lets take an average programmer who is tasked with converting the > > > customer allocator used in OpenMPI[1] to use large pages. He's very > > > unlikely to take the cache alignment into account and instead be > > > faced by weird slowdowns after porting the code. I don't think > > > they'll dig through documentation, oprofile or anything else looking > > > for the answer. They'll just assume hugepages are stupid and move > > > on. Even if they know about cache line coloring, they'll assume the > > > allocator is already doing it. > > > > > > A caller that is really looking for X number of hugepages is going to > > > align > > > the length resulting in no wastage and no colouring. If for some reason > > > they are not aligning their length but need the start hugepage-aligned, > > > they would use GHP_ALIGN. > > > > No, I still don't buy it - making assumptions about what the program's > > intending based on details of the parameters is just dangerous > > territory, even if you're right most of the time. i > > Thing is, functions like malloc already do things already. In the kernel > it's vaguely similar. kmalloc() for example colours on behalf of the > caller because it's the right thing to do where possible.
Yes, functions like malloc(). I'm saying that get_huge_pages() isn't a function like malloc(), and I don't think it should become one. > > The basic problem > > here is that you're making get_huge_pages() do two different things: > > first act as a low-level "grab some huge pages" function, and second > > act as a general handy "grab some memory that's in hugepages". This > > extension is appropriate for the second, but not the first. > > > > I view it more as the latter - i.e. it's a function for grabbing memory that > is in hugepages. Certainly when I was doing a straight-forward converting > of STREAM to get_huge_pages(), I got caught by this as the impact of not > colouring was so severe. Look at this In which case we should change the name. If this was in a function called hugepage_alloc(), I'd have no problem with it. > http://www.csn.ul.ie/~mel/comparison-align-Add.png > > 1-get_huge_pages() calls get_huge_pages() once and places the 3 arrays used > by STREAM in it. 3-get_huge_page-coloured calls get_huge_pages() once for > each array - which is what a programmer is most likely to do and what I > started with. Note how they perform similarly because this has my patch > applied and uses cache coloring. Yeah, I'm not saying that this isn't a good idea. I just think we need a new entry point for it. > 3-get_huge_page-aligned uses GHP_ALIGN and behaves as get_huge_pages() > in 2.1-pre4 currently does. It's performance *blows* and it took me a few > hours to pin the problem on lack of colouring because I'm used of allocators > handling that sort of issue on my behalf. Instead I chased down blind alleys. > > > Having the second is a reasonable thing - although to a certain extent > > just using malloc() with HUGEPAGE_MORECORE enabled accomplishes this - > > Ordinarily yes but there is at least one case where a custom allocator is > involved and using HUGEPAGE_MORECORE does nothing. get_huge_pages() was > meant to help those guys port. Hrm. If there's a custom allocator in play, shouldn't *it* understand cache-colouring and the backing allocator's page size? -- 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 ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ Libhugetlbfs-devel mailing list Libhugetlbfs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
