Re: [PATCH 2/6] change alloc function in pcpu_alloc_pages

[Minchan Kim <minchan.kim@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3916 bytes --]

Hi, Tejun.

On Mon, Apr 19, 2010 at 6:22 AM, Tejun Heo <tj@kernel.org> wrote:
> On 04/19/2010 12:54 AM, Minchan Kim wrote:
>>> alloc_pages is the same as alloc_pages_any_node so why have it?
>>
>> I don't want to force using '_node' postfix on UMA users.
>> Maybe they don't care getting page from any node and event don't need to
>> know about _NODE_.
>
> Yeah, then, remove alloc_pages_any_node().  I can't really see the
> point of any_/exact_node.  alloc_pages() and alloc_pages_node() are
> fine and in line with other functions.  Why change it?
>
>>> Why remove it? If you want to get rid of -1 handling then check all the
>>
>> alloc_pages_node have multiple meaning as you said. So some of users
>> misuses that API. I want to clear intention of user.
>
> The name is fine.  Just clean up the users and make the intended usage
> clear in documentation and implementation (ie. trigger a big fat
> warning) and make all the callers use named constants instead of -1
> for special meanings.
>
> Thanks.

Let's tidy my table.

I made quick patch to show the concept with one example of pci-dma.
(Sorry but I attach patch since web gmail's mangling.)

On UMA, we can change alloc_pages with
alloc_pages_exact_node(numa_node_id(),....)
(Actually, the patch is already merged mmotm)

on NUMA, alloc_pages is some different meaning, so I don't want to change it.
on NUMA, alloc_pages_node means _ANY_NODE_.
So let's remove nid argument and change naming with alloc_pages_any_node.

Then, whole users of alloc_pages_node can be changed between
alloc_pages_exact_node and alloc_pages_any_node.

It was my intention. What's your concern?

Thanks for your interest, Tejun. :)

diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index a4ac764..dc511cb 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -152,12 +152,21 @@ void *dma_generic_alloc_coherent(struct device
*dev, size_t size,
        unsigned long dma_mask;
        struct page *page;
        dma_addr_t addr;
+       int nid;

        dma_mask = dma_alloc_coherent_mask(dev, flag);

        flag |= __GFP_ZERO;
 again:
-       page = alloc_pages_node(dev_to_node(dev), flag, get_order(size));
+       nid = dev_to_node(dev);
+       /*
+        * If pci-dma maintainer makes sure nid never has NUMA_NO_NODE
+        * we can remove this ugly checking.
+        */
+       if (nid == NUMA_NO_NODE)
+               page = alloc_pages_any_node(flag, get_order(size));
+       else
+               page = alloc_pages_exact_node(nid, flag, get_order(size));
        if (!page)
                return NULL;

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 4c6d413..47fba21 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -278,13 +278,10 @@ __alloc_pages(gfp_t gfp_mask, unsigned int order,
        return __alloc_pages_nodemask(gfp_mask, order, zonelist, NULL);
 }

-static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
+static inline struct page *alloc_pagse_any_node(gfp_t gfp_mask,
                                                unsigned int order)
 {
-       /* Unknown node is current node */
-       if (nid < 0)
-               nid = numa_node_id();
-
+       int nid = numa_node_id();
        return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
 }

@@ -308,7 +305,7 @@ extern struct page *alloc_page_vma(gfp_t gfp_mask,
                        struct vm_area_struct *vma, unsigned long addr);
 #else
 #define alloc_pages(gfp_mask, order) \
-               alloc_pages_node(numa_node_id(), gfp_mask, order)
+               alloc_pages_exact_node(numa_node_id(), gfp_mask, order)
 #define alloc_page_vma(gfp_mask, vma, addr) alloc_pages(gfp_mask, 0)
 #endif
 #define alloc_page(gfp_mask) alloc_pages(gfp_mask, 0)
~

-- 
Kind regards,
Minchan Kim

[-- Attachment #2: change_alloc_functions_naming.patch --]
[-- Type: text/x-diff, Size: 1844 bytes --]

diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index a4ac764..dc511cb 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -152,12 +152,21 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
 	unsigned long dma_mask;
 	struct page *page;
 	dma_addr_t addr;
+	int nid;
 
 	dma_mask = dma_alloc_coherent_mask(dev, flag);
 
 	flag |= __GFP_ZERO;
 again:
-	page = alloc_pages_node(dev_to_node(dev), flag, get_order(size));
+	nid = dev_to_node(dev);
+	/*
+	 * If pci-dma maintainer makes sure nid never has NUMA_NO_NODE
+	 * we can remove this ugly checking.
+	 */
+	if (nid == NUMA_NO_NODE)
+		page = alloc_pages_any_node(flag, get_order(size));
+	else
+		page = alloc_pages_exact_node(nid, flag, get_order(size));
 	if (!page)
 		return NULL;
 
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 4c6d413..47fba21 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -278,13 +278,10 @@ __alloc_pages(gfp_t gfp_mask, unsigned int order,
 	return __alloc_pages_nodemask(gfp_mask, order, zonelist, NULL);
 }
 
-static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
+static inline struct page *alloc_pagse_any_node(gfp_t gfp_mask,
 						unsigned int order)
 {
-	/* Unknown node is current node */
-	if (nid < 0)
-		nid = numa_node_id();
-
+	int nid = numa_node_id();
 	return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
 }
 
@@ -308,7 +305,7 @@ extern struct page *alloc_page_vma(gfp_t gfp_mask,
 			struct vm_area_struct *vma, unsigned long addr);
 #else
 #define alloc_pages(gfp_mask, order) \
-		alloc_pages_node(numa_node_id(), gfp_mask, order)
+		alloc_pages_exact_node(numa_node_id(), gfp_mask, order)
 #define alloc_page_vma(gfp_mask, vma, addr) alloc_pages(gfp_mask, 0)
 #endif
 #define alloc_page(gfp_mask) alloc_pages(gfp_mask, 0)