rename alloc_pages_exact()

What I really wanted in the end was a highmem-capable alloc_pages_exact(), so here it is.

It's a bit unfortunate that we have __free_pages_exact() and free_pages_exact(), but that mess already exists with __free_pages() vs. free_pages_exact(). So, at worst, this mirrors the mess that we already have.

I'm also a bit worried that I've not put in something named alloc_pages_exact(), but that behaves differently than it did before this set. I got all of the in-tree cases, but I'm a bit worried about stragglers elsewhere. So, I'm calling this __alloc_pages_exact() for the moment. We can take out the __ some day if it bothers people.

Note that the __get_free_pages() has a !GFP_HIGHMEM check. Now that we are using alloc_pages_exact() instead of __get_free_pages() for get_free_pages_exact(), we had to add a new check in get_free_pages_exact().

This has been compile and boot tested, and I checked that

echo 2 > /sys/kernel/profiling

still works, since it uses get_free_pages_exact().

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/drivers/video/pxafb.c |    4 -
 linux-2.6.git-dave/include/linux/gfp.h   |    4 +
 linux-2.6.git-dave/kernel/profile.c      |    4 -
 linux-2.6.git-dave/mm/page_alloc.c       |   81 +++++++++++++++++++++++--------
 4 files changed, 69 insertions(+), 24 deletions(-)

diff -puN include/linux/gfp.h~make_new_alloc_pages_exact include/linux/gfp.h

include/linux/gfp.h renamed from include/linux/gfp.h~make_new_alloc_pages_exact +1 -0

+/* virtual address version */
 void *get_free_pages_exact(gfp_t gfp_mask, size_t size);
 void free_pages_exact(void *virt, size_t size);
 

diff -puN mm/page_alloc.c~make_new_alloc_pages_exact mm/page_alloc.c

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +5 -5

@@ -2318,36 +2318,83 @@ void free_pages(unsigned long addr, unsi
 EXPORT_SYMBOL(free_pages);
 
 /**
- * get_free_pages_exact - allocate an exact number physically-contiguous pages.
+ * __alloc_pages_exact - allocate an exact number physically-contiguous pages.
  * @gfp_mask: GFP flags for the allocation
  * @size: the number of bytes to allocate
+ * returns: struct page for allocated memory
  *
- * This function is similar to __get_free_pages(), except that it allocates the
- * minimum number of pages to satisfy the request.  get_free_pages() can only
+ * This function is similar to alloc_pages(), except that it allocates the
+ * minimum number of pages to satisfy the request.  alloc_pages() can only
  * allocate memory in power-of-two pages.
  *
  * This function is also limited by MAX_ORDER.
  *
  * Memory allocated by this function must be released by free_pages_exact().
  */
-void *get_free_pages_exact(gfp_t gfp_mask, size_t size)
+struct page *__alloc_pages_exact(gfp_t gfp_mask, size_t size)
 {
 	unsigned int order = get_order(size);
-	unsigned long addr;

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +6 -4

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

Quoted 2 lines · Dave Hansen

What I really wanted in the end was a highmem-capable alloc_pages_exact(), so here it is.

Perhaps expand upon how the new alloc_pages_exact() works and what it will be used for instead of only talking about how it's named?

include/linux/gfp.h extern struct page *alloc_pages_vma(gfp_ · line 356

Quoted 3 lines

include/linux/gfp.h renamed from include/linux/gfp.h~make_new_alloc_pages_exact +3 -0

+/* 'struct page' version */
+struct page *__alloc_pages_exact(gfp_t, size_t);
+void __free_pages_exact(struct page *, size_t);

They're not required, but these should have the names of the arguments like the other prototypes in this file.

mm/page_alloc.c void free_pages(unsigned long addr, unsi · line 2342

Quoted 8 lines

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +4 -4

-	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 + PAGE_ALIGN(size)/PAGE_SIZE;

Wouldn't it better to call this "unused" rather than "used" since it represents a cursor over pages that we want to free?

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +4 -5

+struct page *__alloc_pages_exact(gfp_t gfp_mask, size_t size)
 {
 	unsigned int order = get_order(size);
-	unsigned long addr;
+	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) {

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +2 -1

 	}
-	return (void *)addr;
+	return page;
+}

Quoted 17 lines · Dave Hansen

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +7 -7

+struct page *__alloc_pages_exact(gfp_t gfp_mask, size_t size)
 {
 	unsigned int order = get_order(size);
-	unsigned long addr;
+	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 + PAGE_ALIGN(size)/PAGE_SIZE;
-		split_page(virt_to_page((void *)addr), order);
+		split_page(page, order);
 		while (used < alloc_end) {
-			free_page(used);

mm/page_alloc.c void free_pages(unsigned long addr, unsi · line 2348

Quoted 4 lines

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +2 -1

-			used += PAGE_SIZE;
+			__free_page(used);
+			used++;
 		}

Have you thought about moving this loop to a separate function, ie. _free_page_range(start, end)? I'm asking because this loop appears in two places and my CMA would also benefit from such a function.

Quoted 7 lines · Dave Hansen

Quoted 6 lines

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +2 -2

 		while (used < alloc_end) {
-			free_page(used);
-			used += PAGE_SIZE;
+			__free_page(used);
+			used++;
 		}

Quoted 3 lines · Michal Nazarewicz

Have you thought about moving this loop to a separate function, ie. _free_page_range(start, end)? I'm asking because this loop appears in two places and my CMA would also benefit from such a function.

Sounds like a good idea to me. Were you thinking start/end 'struct page's as arguments?

On Thu, 07 Apr 2011 19:21:05 +0200, Dave Hansen <[email protected]>

mm/page_alloc.c void free_pages(unsigned long addr, unsi · line 2348

Quoted 10 lines

mm/page_alloc.c renamed from mm/page_alloc.c~make_new_alloc_pages_exact +5 -3

+		struct page *alloc_end = page + (1 << order);
+		struct page *used = page + PAGE_ALIGN(size)/PAGE_SIZE;
-		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++;
 		}

Have you thought about moving this loop to a separate function, ie. _free_page_range(start, end)? I'm asking because this loop appears in two places and my CMA would also benefit from such a function.

alloc_pages_exact() returns a virtual address. But, alloc_pages() returns a 'struct page *'. That makes for very confused kernel hackers.

__get_free_pages(), on the other hand, returns virtual addresses. That makes alloc_pages_exact() a much closer match to __get_free_pages(), so rename it to get_free_pages_exact().

Note that alloc_pages_exact()'s partner, free_pages_exact() already matches free_pages(), so we do not have to touch the free side of things.

Signed-off-by Dave Hansen <[email protected]>

Acked-by Andi Kleen <[email protected]>

---

 linux-2.6.git-dave/drivers/video/fsl-diu-fb.c  |    2 +-
 linux-2.6.git-dave/drivers/video/mxsfb.c       |    2 +-
 linux-2.6.git-dave/drivers/video/pxafb.c       |    4 ++--
 linux-2.6.git-dave/drivers/virtio/virtio_pci.c |    2 +-
 linux-2.6.git-dave/include/linux/gfp.h         |    2 +-
 linux-2.6.git-dave/kernel/profile.c            |    2 +-
 linux-2.6.git-dave/mm/page_alloc.c             |   24 ++++++++++++------------
 linux-2.6.git-dave/mm/page_cgroup.c            |    2 +-
 8 files changed, 20 insertions(+), 20 deletions(-)

diff -puN drivers/video/fsl-diu-fb.c~change-alloc_pages_exact-name drivers/video/fsl-diu-fb.c

drivers/video/fsl-diu-fb.c renamed from drivers/video/fsl-diu-fb.c~change-alloc_pages_exact-name +1 -1

@@ -294,7 +294,7 @@ static void *fsl_diu_alloc(size_t size, 
 
 	pr_debug("size=%zu\n", size);
 
-	virt = alloc_pages_exact(size, GFP_DMA | __GFP_ZERO);
+	virt = get_free_pages_exact(GFP_DMA | __GFP_ZERO, size);
 	if (virt) {
 		*phys = virt_to_phys(virt);
 		pr_debug("virt=%p phys=%llx\n", virt,

diff -puN drivers/video/pxafb.c~change-alloc_pages_exact-name drivers/video/pxafb.c

drivers/video/pxafb.c renamed from drivers/video/pxafb.c~change-alloc_pages_exact-name +1 -1

@@ -1714,7 +1714,7 @@ static int __devinit pxafb_init_video_me
 {
 	int size = PAGE_ALIGN(fbi->video_mem_size);
 
-	fbi->video_mem = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);
+	fbi->video_mem = get_free_pages_exact(GFP_KERNEL | __GFP_ZERO, size);
 	if (fbi->video_mem == NULL)
 		return -ENOMEM;
 

diff -puN drivers/virtio/virtio_pci.c~change-alloc_pages_exact-name drivers/virtio/virtio_pci.c

drivers/virtio/virtio_pci.c renamed from drivers/virtio/virtio_pci.c~change-alloc_pages_exact-name +1 -1

@@ -385,7 +385,7 @@ static struct virtqueue *setup_vq(struct
 	info->msix_vector = msix_vec;
 
 	size = PAGE_ALIGN(vring_size(num, VIRTIO_PCI_VRING_ALIGN));
-	info->queue = alloc_pages_exact(size, GFP_KERNEL|__GFP_ZERO);
+	info->queue = get_free_pages_exact(GFP_KERNEL|__GFP_ZERO, size);
 	if (info->queue == NULL) {
 		err = -ENOMEM;
 		goto out_info;

diff -puN include/linux/gfp.h~change-alloc_pages_exact-name include/linux/gfp.h

include/linux/gfp.h renamed from include/linux/gfp.h~change-alloc_pages_exact-name +1 -1

@@ -351,7 +351,7 @@ extern struct page *alloc_pages_vma(gfp_
 extern unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order);
 extern unsigned long get_zeroed_page(gfp_t gfp_mask);
 
-void *alloc_pages_exact(size_t size, gfp_t gfp_mask);
+void *get_free_pages_exact(gfp_t gfp_mask, size_t size);
 void free_pages_exact(void *virt, size_t size);
 
 #define __get_free_page(gfp_mask) \

diff -puN kernel/profile.c~change-alloc_pages_exact-name kernel/profile.c

Quoted 7 lines

alloc_pages_exact() returns a virtual address. But, alloc_pages() returns a 'struct page *'. That makes for very confused kernel hackers.

__get_free_pages(), on the other hand, returns virtual addresses. That makes alloc_pages_exact() a much closer match to __get_free_pages(), so rename it to get_free_pages_exact().

The patch also reverses the arguments of the function in include/linux/gfp.h, undoubtedly to resemble the (mask, order) appearance of __get_free_pages():

	-void *alloc_pages_exact(size_t size, gfp_t gfp_mask);
	+void *get_free_pages_exact(gfp_t gfp_mask, size_t size);