Messages in this thread | | | Subject | Re: [PATCH 2/3] make new alloc_pages_exact() | From | Dave Hansen <> | Date | Mon, 11 Apr 2011 15:36:00 -0700 |
| |
On Mon, 2011-04-11 at 15:22 -0700, Andrew Morton wrote: > > +/* 'struct page' version */ > > +struct page *__alloc_pages_exact(gfp_t gfp_mask, size_t size); > > +void __free_pages_exact(struct page *page, size_t size); > > The declarations use "size", but the definitions use "nr_pages". > "nr_pages" is way better.
I'll fix that.
> Should it really be size_t? size_t's units are "bytes", usually.
Yeah, the nr_pages one should probably be an unsigned long.
> > -void *get_free_pages_exact(gfp_t gfp_mask, size_t size) > > +struct page *__alloc_pages_exact(gfp_t gfp_mask, size_t nr_pages) > > Most allocation functions are of the form foo(size, gfp_t), but this > one has the args reversed. Was there a reason for that?
I'm trying to make this a clone of alloc_pages(), which does:
#define alloc_pages(gfp_mask, order)
It needs a note in the changelog on why I did it.
> > { > > - unsigned int order = get_order(size); > > - unsigned long addr; > > + unsigned int order = get_order(nr_pages * PAGE_SIZE); > > + struct page *page; > > > > - addr = __get_free_pages(gfp_mask, order); > > - if (addr) { > > - unsigned long alloc_end = addr + (PAGE_SIZE << order); > > - unsigned long used = addr + PAGE_ALIGN(size); > > + page = alloc_pages(gfp_mask, order); > > + if (page) { > > + struct page *alloc_end = page + (1 << order); > > + struct page *used = page + nr_pages; > > > > - split_page(virt_to_page((void *)addr), order); > > + split_page(page, order); > > while (used < alloc_end) { > > - free_page(used); > > - used += PAGE_SIZE; > > + __free_page(used); > > + used++; > > } > > } > > > > - return (void *)addr; > > + return page; > > +} > > +EXPORT_SYMBOL(__alloc_pages_exact); > > + > > +/** > > + * __free_pages_exact - release memory allocated via __alloc_pages_exact() > > + * @virt: the value returned by get_free_pages_exact. > > + * @nr_pages: size in pages, same value as passed to __alloc_pages_exact(). > > + * > > + * Release the memory allocated by a previous call to __alloc_pages_exact(). > > + */ > > +void __free_pages_exact(struct page *page, size_t nr_pages) > > +{ > > + struct page *end = page + nr_pages; > > + > > + while (page < end) { > > Hand-optimised. Old school. Doesn't trust the compiler :)
Hey, ask the dude who put free_pages_exact() in there! :)
> > + __free_page(page); > > + page++; > > + } > > +} > > +EXPORT_SYMBOL(__free_pages_exact); > > Really, this function duplicates release_pages(). release_pages() is > big and fat and complex and is a crime against uniprocessor but it does > make some effort to reduce the spinlocking frequency and in many > situations, release_pages() will cause vastly less locked bus traffic > than your __free_pages_exact(). And who knows, smart use of > release_pages()'s "cold" hint may provide some benefits.
Seems like a decent enough thing to try. I'll give it a shot and make sure it's OK to use.
-- Dave
| |