Skip to contentSkip to conversation

code clean rename alloc_pages_exact_node()

PATCH In review Bob Liu 5 participants 18 messages Apr 10, 2010 – Apr 16, 2010

Subsystems (13)
Memory management x86 PowerPC MM core +9 more subsystems
lore
Share & tools
lore

Bob Liu

author
#0 lore raw

PATCH 1/1 code clean rename alloc_pages_exact_node()

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

Signed-off-by Bob Liu <[email protected]>
---
 arch/ia64/hp/common/sba_iommu.c   |    2 +-
 arch/ia64/kernel/uncached.c       |    2 +-
 arch/ia64/sn/pci/pci_dma.c        |    2 +-
 arch/powerpc/platforms/cell/ras.c |    4 ++--
 arch/x86/kvm/vmx.c                |    3 ++-
 drivers/misc/sgi-xp/xpc_uv.c      |    5 +++--
 include/linux/gfp.h               |    2 +-
 kernel/profile.c                  |    8 ++++----
 mm/filemap.c                      |    2 +-
 mm/hugetlb.c                      |    2 +-
 mm/memory-failure.c               |    2 +-
 mm/mempolicy.c                    |    2 +-
 mm/migrate.c                      |    2 +-
 mm/slab.c                         |    3 ++-
 mm/slob.c                         |    4 ++--
 15 files changed, 24 insertions(+), 21 deletions(-)
arch/ia64/hp/common/sba_iommu.c +1 -1
index e14c492..e578f08 100644
@@ -1140,7 +1140,7 @@ sba_alloc_coherent (struct device *dev, size_t size, dma_addr_t *dma_handle, gfp
 #ifdef CONFIG_NUMA
 	{
 		struct page *page;
-		page = alloc_pages_exact_node(ioc->node == MAX_NUMNODES ?
+		page = alloc_pages_from_valid_node(ioc->node == MAX_NUMNODES ?
 		                        numa_node_id() : ioc->node, flags,
 		                        get_order(size));
arch/ia64/kernel/uncached.c +1 -1
index c4696d2..ab8c085 100644
@@ -98,7 +98,7 @@ static int uncached_add_chunk(struct uncached_pool *uc_pool, int nid)
 
 	/* attempt to allocate a granule's worth of cached memory pages */
 
-	page = alloc_pages_exact_node(nid,
+	page = alloc_pages_from_valid_node(nid,
 				GFP_KERNEL | __GFP_ZERO | GFP_THISNODE,
 				IA64_GRANULE_SHIFT-PAGE_SHIFT);
 	if (!page) {
arch/ia64/sn/pci/pci_dma.c +1 -1
index a9d310d..e09ccf7 100644
@@ -91,7 +91,7 @@ static void *sn_dma_alloc_coherent(struct device *dev, size_t size,
 	 */
 	node = pcibus_to_node(pdev->bus);
 	if (likely(node >=0)) {
-		struct page *p = alloc_pages_exact_node(node,
+		struct page *p = alloc_pages_from_valid_node(node,
 						flags, get_order(size));
 
 		if (likely(p))
arch/powerpc/platforms/cell/ras.c +2 -2
index 1d3c4ef..6d32594 100644
@@ -123,8 +123,8 @@ static int __init cbe_ptcal_enable_on_node(int nid, int order)
 
 	area->nid = nid;
 	area->order = order;
-	area->pages = alloc_pages_exact_node(area->nid, GFP_KERNEL|GFP_THISNODE,
-						area->order);
+	area->pages = alloc_pages_from_valid_node(area->nid,
+			GFP_KERNEL | GFP_THISNODE, area->order);
 
 	if (!area->pages) {
 		printk(KERN_WARNING "%s: no page on node %d\n",
arch/x86/kvm/vmx.c +2 -1
index 753ffc2..f554b8c 100644
@@ -1405,7 +1405,8 @@ static struct vmcs *alloc_vmcs_cpu(int cpu)
 	struct page *pages;
 	struct vmcs *vmcs;
 
-	pages = alloc_pages_exact_node(node, GFP_KERNEL, vmcs_config.order);
+	pages = alloc_pages_from_valid_node(node, GFP_KERNEL,
+			vmcs_config.order);
 	if (!pages)
 		return NULL;
 	vmcs = page_address(pages);
drivers/misc/sgi-xp/xpc_uv.c +3 -2
index 1f59ee2..ba3544e 100644
@@ -238,8 +238,9 @@ xpc_create_gru_mq_uv(unsigned int mq_size, int cpu, char *irq_name,
 	mq->mmr_blade = uv_cpu_to_blade_id(cpu);
 
 	nid = cpu_to_node(cpu);
-	page = alloc_pages_exact_node(nid, GFP_KERNEL | __GFP_ZERO | GFP_THISNODE,
-				pg_order);
+	page = alloc_pages_from_valid_node(nid,
+			GFP_KERNEL | __GFP_ZERO | GFP_THISNODE, pg_order);
+
 	if (page == NULL) {
 		dev_err(xpc_part, "xpc_create_gru_mq_uv() failed to alloc %d "
 			"bytes of memory on nid=%d for GRU mq\n", mq_size, nid);
include/linux/gfp.h +1 -1
index 4c6d413..c94f2ed 100644
@@ -288,7 +288,7 @@ static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
 	return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
 }
 
-static inline struct page *alloc_pages_exact_node(int nid, gfp_t gfp_mask,
+static inline struct page *alloc_pages_from_valid_node(int nid, gfp_t gfp_mask,
 						unsigned int order)
 {
 	VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
kernel/profile.c +4 -4
index a55d3a3..4bf82da 100644
@@ -366,7 +366,7 @@ static int __cpuinit profile_cpu_callback(struct notifier_block *info,
 		node = cpu_to_node(cpu);
 		per_cpu(cpu_profile_flip, cpu) = 0;
 		if (!per_cpu(cpu_profile_hits, cpu)[1]) {
-			page = alloc_pages_exact_node(node,
+			page = alloc_pages_from_valid_node(node,
 					GFP_KERNEL | __GFP_ZERO,
 					0);
 			if (!page)

@@ -374,7 +374,7 @@ static int __cpuinit profile_cpu_callback(struct notifier_block *info,
 			per_cpu(cpu_profile_hits, cpu)[1] = page_address(page);
 		}
 		if (!per_cpu(cpu_profile_hits, cpu)[0]) {
-			page = alloc_pages_exact_node(node,
+			page = alloc_pages_from_valid_node(node,
 					GFP_KERNEL | __GFP_ZERO,
 					0);
 			if (!page)

@@ -568,14 +568,14 @@ static int create_hash_tables(void)
 		int node = cpu_to_node(cpu);
 		struct page *page;
 
-		page = alloc_pages_exact_node(node,
+		page = alloc_pages_from_valid_node(node,
 				GFP_KERNEL | __GFP_ZERO | GFP_THISNODE,
 				0);
 		if (!page)
 			goto out_cleanup;
 		per_cpu(cpu_profile_hits, cpu)[1]
 				= (struct profile_hit *)page_address(page);
-		page = alloc_pages_exact_node(node,
+		page = alloc_pages_from_valid_node(node,
 				GFP_KERNEL | __GFP_ZERO | GFP_THISNODE,
 				0);
 		if (!page)
mm/filemap.c +1 -1
index 140ebda..0424c3b 100644
@@ -463,7 +463,7 @@ struct page *__page_cache_alloc(gfp_t gfp)
 {
 	if (cpuset_do_page_mem_spread()) {
 		int n = cpuset_mem_spread_node();
-		return alloc_pages_exact_node(n, gfp, 0);
+		return alloc_pages_from_valid_node(n, gfp, 0);
 	}
 	return alloc_pages(gfp, 0);
 }
mm/hugetlb.c +1 -1
index 6034dc9..01dd9c6 100644
@@ -607,7 +607,7 @@ static struct page *alloc_fresh_huge_page_node(struct hstate *h, int nid)
 	if (h->order >= MAX_ORDER)
 		return NULL;
 
-	page = alloc_pages_exact_node(nid,
+	page = alloc_pages_from_valid_node(nid,
 		htlb_alloc_mask|__GFP_COMP|__GFP_THISNODE|
 						__GFP_REPEAT|__GFP_NOWARN,
 		huge_page_order(h));
mm/memory-failure.c +1 -1
index 620b0b4..43abda1 100644
@@ -1126,7 +1126,7 @@ EXPORT_SYMBOL(unpoison_memory);
 static struct page *new_page(struct page *p, unsigned long private, int **x)
 {
 	int nid = page_to_nid(p);
-	return alloc_pages_exact_node(nid, GFP_HIGHUSER_MOVABLE, 0);
+	return alloc_pages_from_valid_node(nid, GFP_HIGHUSER_MOVABLE, 0);
 }
 
 /*
mm/mempolicy.c +1 -1
index 08f40a2..6838cd8 100644
@@ -842,7 +842,7 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 
 static struct page *new_node_page(struct page *page, unsigned long node, int **x)
 {
-	return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+	return alloc_pages_from_valid_node(node, GFP_HIGHUSER_MOVABLE, 0);
 }
 
 /*
mm/migrate.c +1 -1
index d3f3f7f..a057a1a 100644
@@ -770,7 +770,7 @@ static struct page *new_page_node(struct page *p, unsigned long private,
 
 	*result = &pm->status;
 
-	return alloc_pages_exact_node(pm->node,
+	return alloc_pages_from_valid_node(pm->node,
 				GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
 }
mm/slab.c +2 -1
index 730d45b..4f71736 100644
@@ -1717,7 +1717,8 @@ static void *kmem_getpages(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 	if (cachep->flags & SLAB_RECLAIM_ACCOUNT)
 		flags |= __GFP_RECLAIMABLE;
 
-	page = alloc_pages_exact_node(nodeid, flags | __GFP_NOTRACK, cachep->gfporder);
+	page = alloc_pages_from_valid_node(nodeid, flags | __GFP_NOTRACK,
+			cachep->gfporder);
 	if (!page)
 		return NULL;
mm/slob.c +2 -2
index 837ebd6..9e3f95b 100644
@@ -46,7 +46,7 @@
  * NUMA support in SLOB is fairly simplistic, pushing most of the real
  * logic down to the page allocator, and simply doing the node accounting
  * on the upper levels. In the event that a node id is explicitly
- * provided, alloc_pages_exact_node() with the specified node id is used
+ * provided, alloc_pages_from_valid_node() with the specified node id is used
  * instead. The common case (or when the node id isn't explicitly provided)
  * will default to the current node, as per numa_node_id().
  *

@@ -244,7 +244,7 @@ static void *slob_new_pages(gfp_t gfp, int order, int node)
 
 #ifdef CONFIG_NUMA
 	if (node != -1)
-		page = alloc_pages_exact_node(node, gfp, order);
+		page = alloc_pages_from_valid_node(node, gfp, order);
 	else
 #endif
 		page = alloc_pages(gfp, order);
Signature · 7 lines
1.5.6.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#1 lore raw

PATCH 1/1 add alloc_pages_exact_node()

Add alloc_pages_exact_node() to allocate pages from exact node.

Signed-off-by Bob Liu <[email protected]>
---
 arch/powerpc/platforms/cell/ras.c |    4 ++--
 include/linux/gfp.h               |    7 +++++++
 mm/mempolicy.c                    |    2 +-
 mm/migrate.c                      |    3 +--
 4 files changed, 11 insertions(+), 5 deletions(-)
arch/powerpc/platforms/cell/ras.c +2 -2
index 6d32594..93a5afd 100644
@@ -123,8 +123,8 @@ static int __init cbe_ptcal_enable_on_node(int nid, int order)
 
 	area->nid = nid;
 	area->order = order;
-	area->pages = alloc_pages_from_valid_node(area->nid,
-			GFP_KERNEL | GFP_THISNODE, area->order);
+	area->pages = alloc_pages_exact_node(area->nid, GFP_KERNEL,
+				area->order);
 
 	if (!area->pages) {
 		printk(KERN_WARNING "%s: no page on node %d\n",
include/linux/gfp.h +7 -0
index c94f2ed..70cf2ae 100644
@@ -296,6 +296,13 @@ static inline struct page *alloc_pages_from_valid_node(int nid, gfp_t gfp_mask,
 	return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
 }
 
+static inline struct page *alloc_pages_exact_node(int nid, gfp_t gfp_mask,
+						unsigned int order)
+{
+	return alloc_pages_from_valid_node(nid, gfp_mask | GFP_THISNODE,
+			order);
+}
+
 #ifdef CONFIG_NUMA
 extern struct page *alloc_pages_current(gfp_t gfp_mask, unsigned order);
mm/mempolicy.c +1 -1
index 6838cd8..08f40a2 100644
@@ -842,7 +842,7 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 
 static struct page *new_node_page(struct page *page, unsigned long node, int **x)
 {
-	return alloc_pages_from_valid_node(node, GFP_HIGHUSER_MOVABLE, 0);
+	return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
 }
 
 /*
mm/migrate.c +1 -2
index a057a1a..17330c5 100644
@@ -770,8 +770,7 @@ static struct page *new_page_node(struct page *p, unsigned long private,
 
 	*result = &pm->status;
 
-	return alloc_pages_from_valid_node(pm->node,
-				GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
+	return alloc_pages_exact_node(pm->node, GFP_HIGHUSER_MOVABLE, 0);
 }
 
 /*
Signature · 7 lines
1.5.6.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#2 lore raw

Re: [PATCH] add alloc_pages_exact_node()

Hi, Bob.

Quoted 49 lines · Bob Liu

Add alloc_pages_exact_node() to allocate pages from exact node.

Signed-off-by Bob Liu <[email protected]>

---
 arch/powerpc/platforms/cell/ras.c |    4 ++--
 include/linux/gfp.h               |    7 +++++++
 mm/mempolicy.c                    |    2 +-
 mm/migrate.c                      |    3 +--

 4 files changed, 11 insertions(+), 5 deletions(-)
arch/powerpc/platforms/cell/ras.c +2 -2
index 6d32594..93a5afd 100644
@@ -123,8 +123,8 @@ static int __init cbe_ptcal_enable_on_node(int nid, int order)
 
        area->nid = nid;
        area->order = order;
-       area->pages = alloc_pages_from_valid_node(area->nid,
-                       GFP_KERNEL | GFP_THISNODE, area->order);
+       area->pages = alloc_pages_exact_node(area->nid, GFP_KERNEL,
+                               area->order);
 
        if (!area->pages) {
                printk(KERN_WARNING "%s: no page on node %d\n",
include/linux/gfp.h +7 -0
index c94f2ed..70cf2ae 100644
@@ -296,6 +296,13 @@ static inline struct page *alloc_pages_from_valid_node(int nid, gfp_t gfp_mask,
        return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
  }
 
+static inline struct page *alloc_pages_exact_node(int nid, gfp_t gfp_mask,
+                                               unsigned int order)
+{
+       return alloc_pages_from_valid_node(nid, gfp_mask | GFP_THISNODE,
+                       order);
+}
+
  #ifdef CONFIG_NUMA
  extern struct page *alloc_pages_current(gfp_t gfp_mask, unsigned order);
mm/mempolicy.c +0 -0
index 6838cd8..08f40a2 100644

mm/mempolicy.c static void migrate_page_add(struct page *page, struct list_head *pagelist, · line 846

Quoted 6 lines
mm/mempolicy.c +1 -1
@@ -842,7 +842,7 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 
  static struct page *new_node_page(struct page *page, unsigned long node, int **x)
  {
-       return alloc_pages_from_valid_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
  }

It's behavior change. Please, write down why you want to change behavior in log.
Although I knew it, you need to explain it for others and git log,

Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#3 lore raw

Re: [PATCH] add alloc_pages_exact_node()

Quoted 53 lines · Minchan Kim

Hi, Bob.

Quoted 50 lines · Bob Liu

Add alloc_pages_exact_node() to allocate pages from exact node.

Signed-off-by Bob Liu <[email protected]>

---
 arch/powerpc/platforms/cell/ras.c |    4 ++--
 include/linux/gfp.h               |    7 +++++++
 mm/mempolicy.c                    |    2 +-
 mm/migrate.c                      |    3 +--

 4 files changed, 11 insertions(+), 5 deletions(-)
arch/powerpc/platforms/cell/ras.c +2 -2
index 6d32594..93a5afd 100644
@@ -123,8 +123,8 @@ static int __init cbe_ptcal_enable_on_node(int nid, int order)
 
        area->nid = nid;
        area->order = order;
-       area->pages = alloc_pages_from_valid_node(area->nid,
-                       GFP_KERNEL | GFP_THISNODE, area->order);
+       area->pages = alloc_pages_exact_node(area->nid, GFP_KERNEL,
+                               area->order);
 
        if (!area->pages) {
                printk(KERN_WARNING "%s: no page on node %d\n",
include/linux/gfp.h +7 -0
index c94f2ed..70cf2ae 100644
@@ -296,6 +296,13 @@ static inline struct page *alloc_pages_from_valid_node(int nid, gfp_t gfp_mask,
        return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
  }
 
+static inline struct page *alloc_pages_exact_node(int nid, gfp_t gfp_mask,
+                                               unsigned int order)
+{
+       return alloc_pages_from_valid_node(nid, gfp_mask | GFP_THISNODE,
+                       order);
+}
+
  #ifdef CONFIG_NUMA
  extern struct page *alloc_pages_current(gfp_t gfp_mask, unsigned order);
mm/mempolicy.c +0 -0
index 6838cd8..08f40a2 100644
@@ -842,7 +842,7 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
Quoted 10 lines

mm/mempolicy.c static void migrate_page_add(struct page *page, struct list_head *pagelist, · line 846

Quoted 5 lines
mm/mempolicy.c +1 -1
  static struct page *new_node_page(struct page *page, unsigned long node, int **x)
  {
-       return alloc_pages_from_valid_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
  }

It's behavior change. Please, write down why you want to change behavior in log.
Although I knew it, you need to explain it for others and git log,

Hm, ok.
I will add the log later. Thank you for your suggestion.

Signature · 8 lines
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#4 lore raw

Re: [PATCH] add alloc_pages_exact_node()

In reply to Bob Liu
Quoted 3 lines · Bob Liu

Add alloc_pages_exact_node() to allocate pages from exact node.

It feels a bit overkill to add a new API that adds one self-explanatory flag.

Quoted 13 lines
Signed-off-by Bob Liu <[email protected]>
---
 arch/powerpc/platforms/cell/ras.c |    4 ++--
 include/linux/gfp.h               |    7 +++++++
 mm/mempolicy.c                    |    2 +-
 mm/migrate.c                      |    3 +--
 4 files changed, 11 insertions(+), 5 deletions(-)
arch/powerpc/platforms/cell/ras.c +0 -0
index 6d32594..93a5afd 100644

arch/powerpc/platforms/cell/ras.c static int __init cbe_ptcal_enable_on_node(int nid, int order) · line 128

Quoted 8 lines
arch/powerpc/platforms/cell/ras.c +2 -2
@@ -123,8 +123,8 @@ static int __init cbe_ptcal_enable_on_node(int nid, int order)
 
 	area->nid = nid;
 	area->order = order;
-	area->pages = alloc_pages_from_valid_node(area->nid,
-			GFP_KERNEL | GFP_THISNODE, area->order);
+	area->pages = alloc_pages_exact_node(area->nid, GFP_KERNEL,
+				area->order);

This behaves differently now. If this is a bug fix, it needs to be a standalone patch. It's the same for other call sites. You are actually changing behaviour and offhand, I don't know why.

Quoted 50 lines
arch/powerpc/platforms/cell/ras.c +0 -0
 	if (!area->pages) {
 		printk(KERN_WARNING "%s: no page on node %d\n",
include/linux/gfp.h +7 -0
index c94f2ed..70cf2ae 100644
@@ -296,6 +296,13 @@ static inline struct page *alloc_pages_from_valid_node(int nid, gfp_t gfp_mask,
 	return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
 }
 
+static inline struct page *alloc_pages_exact_node(int nid, gfp_t gfp_mask,
+						unsigned int order)
+{
+	return alloc_pages_from_valid_node(nid, gfp_mask | GFP_THISNODE,
+			order);
+}
+
 #ifdef CONFIG_NUMA
 extern struct page *alloc_pages_current(gfp_t gfp_mask, unsigned order);
mm/mempolicy.c +1 -1
index 6838cd8..08f40a2 100644
@@ -842,7 +842,7 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 
 static struct page *new_node_page(struct page *page, unsigned long node, int **x)
 {
-	return alloc_pages_from_valid_node(node, GFP_HIGHUSER_MOVABLE, 0);
+	return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
 }
 
 /*
mm/migrate.c +1 -2
index a057a1a..17330c5 100644
@@ -770,8 +770,7 @@ static struct page *new_page_node(struct page *p, unsigned long private,
 
 	*result = &pm->status;
 
-	return alloc_pages_from_valid_node(pm->node,
-				GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
+	return alloc_pages_exact_node(pm->node, GFP_HIGHUSER_MOVABLE, 0);
 }
 
 /*
Signature · 1 line
1.5.6.3
Signature · 9 lines
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Reviewed-by on v1 1/1
#5 lore raw
In reply to Bob Liu
Quoted 6 lines · Bob Liu

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

Signed-off-by Bob Liu <[email protected]>
Reviewed-by Minchan Kim <[email protected]>

I like this naming.

Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#6 lore raw
Quoted 9 lines · Minchan Kim
Quoted 6 lines · Bob Liu

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

Signed-off-by Bob Liu <[email protected]>
Reviewed-by Minchan Kim <[email protected]>

Thanks!

Signature · 8 lines
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#7 lore raw
In reply to Bob Liu
Quoted 5 lines · Bob Liu

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

I don't know about this change either but as I introduced the original function name, I am biased. My reading of it is - allocate me pages and I know exactly which node I need. I see how it it could be read as "allocate me pages from exactly this node" but I don't feel the new naming is that much clearer either.

I recognise I'm not the best at naming though so I don't object to a namechange if people really feel the new name is better.

Quoted 259 lines
Signed-off-by Bob Liu <[email protected]>
---
 arch/ia64/hp/common/sba_iommu.c   |    2 +-
 arch/ia64/kernel/uncached.c       |    2 +-
 arch/ia64/sn/pci/pci_dma.c        |    2 +-
 arch/powerpc/platforms/cell/ras.c |    4 ++--
 arch/x86/kvm/vmx.c                |    3 ++-
 drivers/misc/sgi-xp/xpc_uv.c      |    5 +++--
 include/linux/gfp.h               |    2 +-
 kernel/profile.c                  |    8 ++++----
 mm/filemap.c                      |    2 +-
 mm/hugetlb.c                      |    2 +-
 mm/memory-failure.c               |    2 +-
 mm/mempolicy.c                    |    2 +-
 mm/migrate.c                      |    2 +-
 mm/slab.c                         |    3 ++-
 mm/slob.c                         |    4 ++--
 15 files changed, 24 insertions(+), 21 deletions(-)
arch/ia64/hp/common/sba_iommu.c +1 -1
index e14c492..e578f08 100644
@@ -1140,7 +1140,7 @@ sba_alloc_coherent (struct device *dev, size_t size, dma_addr_t *dma_handle, gfp
 #ifdef CONFIG_NUMA
 	{
 		struct page *page;
-		page = alloc_pages_exact_node(ioc->node == MAX_NUMNODES ?
+		page = alloc_pages_from_valid_node(ioc->node == MAX_NUMNODES ?
 		                        numa_node_id() : ioc->node, flags,
 		                        get_order(size));
arch/ia64/kernel/uncached.c +1 -1
index c4696d2..ab8c085 100644
@@ -98,7 +98,7 @@ static int uncached_add_chunk(struct uncached_pool *uc_pool, int nid)
 
 	/* attempt to allocate a granule's worth of cached memory pages */
 
-	page = alloc_pages_exact_node(nid,
+	page = alloc_pages_from_valid_node(nid,
 				GFP_KERNEL | __GFP_ZERO | GFP_THISNODE,
 				IA64_GRANULE_SHIFT-PAGE_SHIFT);
 	if (!page) {
arch/ia64/sn/pci/pci_dma.c +1 -1
index a9d310d..e09ccf7 100644
@@ -91,7 +91,7 @@ static void *sn_dma_alloc_coherent(struct device *dev, size_t size,
 	 */
 	node = pcibus_to_node(pdev->bus);
 	if (likely(node >=0)) {
-		struct page *p = alloc_pages_exact_node(node,
+		struct page *p = alloc_pages_from_valid_node(node,
 						flags, get_order(size));
 
 		if (likely(p))
arch/powerpc/platforms/cell/ras.c +2 -2
index 1d3c4ef..6d32594 100644
@@ -123,8 +123,8 @@ static int __init cbe_ptcal_enable_on_node(int nid, int order)
 
 	area->nid = nid;
 	area->order = order;
-	area->pages = alloc_pages_exact_node(area->nid, GFP_KERNEL|GFP_THISNODE,
-						area->order);
+	area->pages = alloc_pages_from_valid_node(area->nid,
+			GFP_KERNEL | GFP_THISNODE, area->order);
 
 	if (!area->pages) {
 		printk(KERN_WARNING "%s: no page on node %d\n",
arch/x86/kvm/vmx.c +2 -1
index 753ffc2..f554b8c 100644
@@ -1405,7 +1405,8 @@ static struct vmcs *alloc_vmcs_cpu(int cpu)
 	struct page *pages;
 	struct vmcs *vmcs;
 
-	pages = alloc_pages_exact_node(node, GFP_KERNEL, vmcs_config.order);
+	pages = alloc_pages_from_valid_node(node, GFP_KERNEL,
+			vmcs_config.order);
 	if (!pages)
 		return NULL;
 	vmcs = page_address(pages);
drivers/misc/sgi-xp/xpc_uv.c +3 -2
index 1f59ee2..ba3544e 100644
@@ -238,8 +238,9 @@ xpc_create_gru_mq_uv(unsigned int mq_size, int cpu, char *irq_name,
 	mq->mmr_blade = uv_cpu_to_blade_id(cpu);
 
 	nid = cpu_to_node(cpu);
-	page = alloc_pages_exact_node(nid, GFP_KERNEL | __GFP_ZERO | GFP_THISNODE,
-				pg_order);
+	page = alloc_pages_from_valid_node(nid,
+			GFP_KERNEL | __GFP_ZERO | GFP_THISNODE, pg_order);
+
 	if (page == NULL) {
 		dev_err(xpc_part, "xpc_create_gru_mq_uv() failed to alloc %d "
 			"bytes of memory on nid=%d for GRU mq\n", mq_size, nid);
include/linux/gfp.h +1 -1
index 4c6d413..c94f2ed 100644
@@ -288,7 +288,7 @@ static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
 	return __alloc_pages(gfp_mask, order, node_zonelist(nid, gfp_mask));
 }
 
-static inline struct page *alloc_pages_exact_node(int nid, gfp_t gfp_mask,
+static inline struct page *alloc_pages_from_valid_node(int nid, gfp_t gfp_mask,
 						unsigned int order)
 {
 	VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
kernel/profile.c +4 -4
index a55d3a3..4bf82da 100644
@@ -366,7 +366,7 @@ static int __cpuinit profile_cpu_callback(struct notifier_block *info,
 		node = cpu_to_node(cpu);
 		per_cpu(cpu_profile_flip, cpu) = 0;
 		if (!per_cpu(cpu_profile_hits, cpu)[1]) {
-			page = alloc_pages_exact_node(node,
+			page = alloc_pages_from_valid_node(node,
 					GFP_KERNEL | __GFP_ZERO,
 					0);
 			if (!page)

@@ -374,7 +374,7 @@ static int __cpuinit profile_cpu_callback(struct notifier_block *info,
 			per_cpu(cpu_profile_hits, cpu)[1] = page_address(page);
 		}
 		if (!per_cpu(cpu_profile_hits, cpu)[0]) {
-			page = alloc_pages_exact_node(node,
+			page = alloc_pages_from_valid_node(node,
 					GFP_KERNEL | __GFP_ZERO,
 					0);
 			if (!page)

@@ -568,14 +568,14 @@ static int create_hash_tables(void)
 		int node = cpu_to_node(cpu);
 		struct page *page;
 
-		page = alloc_pages_exact_node(node,
+		page = alloc_pages_from_valid_node(node,
 				GFP_KERNEL | __GFP_ZERO | GFP_THISNODE,
 				0);
 		if (!page)
 			goto out_cleanup;
 		per_cpu(cpu_profile_hits, cpu)[1]
 				= (struct profile_hit *)page_address(page);
-		page = alloc_pages_exact_node(node,
+		page = alloc_pages_from_valid_node(node,
 				GFP_KERNEL | __GFP_ZERO | GFP_THISNODE,
 				0);
 		if (!page)
mm/filemap.c +1 -1
index 140ebda..0424c3b 100644
@@ -463,7 +463,7 @@ struct page *__page_cache_alloc(gfp_t gfp)
 {
 	if (cpuset_do_page_mem_spread()) {
 		int n = cpuset_mem_spread_node();
-		return alloc_pages_exact_node(n, gfp, 0);
+		return alloc_pages_from_valid_node(n, gfp, 0);
 	}
 	return alloc_pages(gfp, 0);
 }
mm/hugetlb.c +1 -1
index 6034dc9..01dd9c6 100644
@@ -607,7 +607,7 @@ static struct page *alloc_fresh_huge_page_node(struct hstate *h, int nid)
 	if (h->order >= MAX_ORDER)
 		return NULL;
 
-	page = alloc_pages_exact_node(nid,
+	page = alloc_pages_from_valid_node(nid,
 		htlb_alloc_mask|__GFP_COMP|__GFP_THISNODE|
 						__GFP_REPEAT|__GFP_NOWARN,
 		huge_page_order(h));
mm/memory-failure.c +1 -1
index 620b0b4..43abda1 100644
@@ -1126,7 +1126,7 @@ EXPORT_SYMBOL(unpoison_memory);
 static struct page *new_page(struct page *p, unsigned long private, int **x)
 {
 	int nid = page_to_nid(p);
-	return alloc_pages_exact_node(nid, GFP_HIGHUSER_MOVABLE, 0);
+	return alloc_pages_from_valid_node(nid, GFP_HIGHUSER_MOVABLE, 0);
 }
 
 /*
mm/mempolicy.c +1 -1
index 08f40a2..6838cd8 100644
@@ -842,7 +842,7 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 
 static struct page *new_node_page(struct page *page, unsigned long node, int **x)
 {
-	return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+	return alloc_pages_from_valid_node(node, GFP_HIGHUSER_MOVABLE, 0);
 }
 
 /*
mm/migrate.c +1 -1
index d3f3f7f..a057a1a 100644
@@ -770,7 +770,7 @@ static struct page *new_page_node(struct page *p, unsigned long private,
 
 	*result = &pm->status;
 
-	return alloc_pages_exact_node(pm->node,
+	return alloc_pages_from_valid_node(pm->node,
 				GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
 }
mm/slab.c +2 -1
index 730d45b..4f71736 100644
@@ -1717,7 +1717,8 @@ static void *kmem_getpages(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 	if (cachep->flags & SLAB_RECLAIM_ACCOUNT)
 		flags |= __GFP_RECLAIMABLE;
 
-	page = alloc_pages_exact_node(nodeid, flags | __GFP_NOTRACK, cachep->gfporder);
+	page = alloc_pages_from_valid_node(nodeid, flags | __GFP_NOTRACK,
+			cachep->gfporder);
 	if (!page)
 		return NULL;
mm/slob.c +2 -2
index 837ebd6..9e3f95b 100644
@@ -46,7 +46,7 @@
  * NUMA support in SLOB is fairly simplistic, pushing most of the real
  * logic down to the page allocator, and simply doing the node accounting
  * on the upper levels. In the event that a node id is explicitly
- * provided, alloc_pages_exact_node() with the specified node id is used
+ * provided, alloc_pages_from_valid_node() with the specified node id is used
  * instead. The common case (or when the node id isn't explicitly provided)
  * will default to the current node, as per numa_node_id().
  *

@@ -244,7 +244,7 @@ static void *slob_new_pages(gfp_t gfp, int order, int node)
 
 #ifdef CONFIG_NUMA
 	if (node != -1)
-		page = alloc_pages_exact_node(node, gfp, order);
+		page = alloc_pages_from_valid_node(node, gfp, order);
 	else
 #endif
 		page = alloc_pages(gfp, order);
Signature · 1 line
1.5.6.3
Signature · 9 lines
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#8 lore raw
Quoted 7 lines · Mel Gorman
Quoted 5 lines · Bob Liu

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

Quoted 5 lines

I don't know about this change either but as I introduced the original function name, I am biased. My reading of it is - allocate me pages and I know exactly which node I need. I see how it it could be read as "allocate me pages from exactly this node" but I don't feel the new naming is that much clearer either.

Tend to agree.
Then, don't change function name but add some comment?

/*
* allow pages from fallback if page allocator can't find free page in your nid.
* If you want to allocate page from exact node, please use
__GFP_THISNODE flags with
* gfp_mask.
*/
static inline struct page *alloc_pages_exact_node(....

Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#9 lore raw
Quoted 17 lines · On Tue, 13 Apr 2010 13:34:52 +0900 Minchan Kim <[email protected]> wrote:
Quoted 7 lines · Mel Gorman
Quoted 5 lines · Bob Liu

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

Quoted 5 lines

I don't know about this change either but as I introduced the original function name, I am biased. My reading of it is - allocate me pages and I know exactly which node I need. I see how it it could be read as "allocate me pages from exactly this node" but I don't feel the new naming is that much clearer either.

Tend to agree.
Then, don't change function name but add some comment?

Quoted 8 lines

/*
* allow pages from fallback if page allocator can't find free page in your nid.
* If you want to allocate page from exact node, please use
__GFP_THISNODE flags with
* gfp_mask.
*/
static inline struct page *alloc_pages_exact_node(....

I vote for this rather than renaming.

There are two functions
allo_pages_node()
alloc_pages_exact_node().

Sane progmrammers tend to see implementation details if there are 2 similar functions.

If I name the function,
alloc_pages_node_verify_nid() ?

I think /* This doesn't support nid=-1, automatic behavior. */ is necessary as comment.

OFF_TOPIC

If you want renaming, I think we should define NID=-1 as

#define ARBITRARY_NID (-1) or
#define CURRENT_NID (-1) or
#define AUTO_NID (-1)

or some. Then, we'll have concensus of NID=-1 support. (Maybe some amount of programmers don't know what NID=-1 means.)

The function will be
alloc_pages_node_no_auto_nid() /* AUTO_NID is not supported by this */
or
alloc_pages_node_veryfy_nid()

Maybe patch will be bigger and may fail after discussion. But it seems worth to try.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#10 lore raw
Quoted 57 lines · KAMEZAWA Hiroyuki

On Tue, 13 Apr 2010 13:34:52 +0900

Quoted 17 lines · Minchan Kim
Quoted 7 lines · Mel Gorman
Quoted 5 lines · Bob Liu

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

Quoted 5 lines

I don't know about this change either but as I introduced the original function name, I am biased. My reading of it is - allocate me pages and I know exactly which node I need. I see how it it could be read as "allocate me pages from exactly this node" but I don't feel the new naming is that much clearer either.

Tend to agree.
Then, don't change function name but add some comment?

Quoted 8 lines

/*
* allow pages from fallback if page allocator can't find free page in your nid.
* If you want to allocate page from exact node, please use
__GFP_THISNODE flags with
* gfp_mask.
*/
static inline struct page *alloc_pages_exact_node(....

I vote for this rather than renaming.

There are two functions
allo_pages_node()
alloc_pages_exact_node().

Sane progmrammers tend to see implementation details if there are 2 similar functions.

If I name the function,
alloc_pages_node_verify_nid() ?

I think /* This doesn't support nid=-1, automatic behavior. */ is necessary as comment.

OFF_TOPIC

If you want renaming, I think we should define NID=-1 as

 #define ARBITRARY_NID           (-1) or
 #define CURRENT_NID             (-1) or
 #define AUTO_NID                (-1)

or some. Then, we'll have concensus of NID=-1 support. (Maybe some amount of programmers don't know what NID=-1 means.)

Quoted 8 lines

The function will be
alloc_pages_node_no_auto_nid() /* AUTO_NID is not supported by this */
or
alloc_pages_node_veryfy_nid()

Maybe patch will be bigger and may fail after discussion. But it seems worth to try.

Hm..It's a bit bigger.
Actually, what I want to do was in my original mail several days ago, the title is "mempolicy:add GFP_THISNODE when allocing new page"

What I concern is *just* we shouldn't fallback to other nodes if the dest node haven't enough free pages during migrate_pages().

The detail is below:
In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>
---
 mm/mempolicy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
mm/mempolicy.c +0 -0
index 08f40a2..fc5ddf5 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page,

struct list_head *pagelist,

static struct page *new_node_page(struct page *page, unsigned long
node, int **x)
{
- return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+ return alloc_pages_exact_node(node,
+ GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
}

Thanks.

Signature · 8 lines
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Acked-by
#11 lore raw
Quoted 67 lines · On Tue, 13 Apr 2010 15:09:42 +0800 Bob Liu <[email protected]> wrote:
Quoted 57 lines · KAMEZAWA Hiroyuki

On Tue, 13 Apr 2010 13:34:52 +0900

Quoted 17 lines · Minchan Kim
Quoted 7 lines · Mel Gorman
Quoted 5 lines · Bob Liu

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

Quoted 5 lines

I don't know about this change either but as I introduced the original function name, I am biased. My reading of it is - allocate me pages and I know exactly which node I need. I see how it it could be read as "allocate me pages from exactly this node" but I don't feel the new naming is that much clearer either.

Tend to agree.
Then, don't change function name but add some comment?

Quoted 8 lines

/*
* allow pages from fallback if page allocator can't find free page in your nid.
* If you want to allocate page from exact node, please use
__GFP_THISNODE flags with
* gfp_mask.
*/
static inline struct page *alloc_pages_exact_node(....

I vote for this rather than renaming.

There are two functions
allo_pages_node()
alloc_pages_exact_node().

Sane progmrammers tend to see implementation details if there are 2 similar functions.

If I name the function,
alloc_pages_node_verify_nid() ?

I think /* This doesn't support nid=-1, automatic behavior. */ is necessary as comment.

OFF_TOPIC

If you want renaming, I think we should define NID=-1 as

 #define ARBITRARY_NID           (-1) or
 #define CURRENT_NID             (-1) or
 #define AUTO_NID                (-1)

or some. Then, we'll have concensus of NID=-1 support. (Maybe some amount of programmers don't know what NID=-1 means.)

Quoted 8 lines

The function will be
alloc_pages_node_no_auto_nid() /* AUTO_NID is not supported by this */
or
alloc_pages_node_veryfy_nid()

Maybe patch will be bigger and may fail after discussion. But it seems worth to try.

Quoted 7 lines

Hm..It's a bit bigger.
Actually, what I want to do was in my original mail several days ago, the title is "mempolicy:add GFP_THISNODE when allocing new page"

What I concern is *just* we shouldn't fallback to other nodes if the dest node haven't enough free pages during migrate_pages().

Hmm. your patch for mempolicy seems good and it's BUGFIX. So, this patch should go as it is.

If you want to add comments to alloc_pages_exact_node(), please do.

But I think it's better to divide BUGFIX and CLEANUP patches.

I'll ack your patch for mempolicy.

Acked-by KAMEZAWA Hiroyuki <[email protected]>

Naming issue never needs quick fix. How about repositing as it is ? Minchan, how do you think ?

Thanks,
-Kame

Quoted 26 lines

The detail is below:
In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>
---
 mm/mempolicy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
mm/mempolicy.c +2 -1
index 08f40a2..fc5ddf5 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page,
 struct list_head *pagelist,
 
 static struct page *new_node_page(struct page *page, unsigned long
 node, int **x)
 {
-       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node,
+                               GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
 }
Quoted 5 lines

Thanks.

Signature · 2 lines
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#12 lore raw
Quoted 83 lines · KAMEZAWA Hiroyuki
Quoted 67 lines · On Tue, 13 Apr 2010 15:09:42 +0800 Bob Liu <[email protected]> wrote:
Quoted 57 lines · KAMEZAWA Hiroyuki

On Tue, 13 Apr 2010 13:34:52 +0900

Quoted 1 line · Minchan Kim

On Tue, Apr 13, 2010 at 1:43 AM, Mel Gorman <[email protected]> wrote:

 > > On Sat, Apr 10, 2010 at 07:49:32PM +0800, Bob Liu wrote:
 > >> Since alloc_pages_exact_node() is not for allocate page from
 > >> exact node but just for removing check of node's valid,
 > >> rename it to alloc_pages_from_valid_node(). Else will make
 > >> people misunderstanding.
 > >>
 > >
 > > I don't know about this change either but as I introduced the original > > function name, I am biased. My reading of it is - allocate me pages and > > I know exactly which node I need. I see how it it could be read as > > "allocate me pages from exactly this node" but I don't feel the new > > naming is that much clearer either.
 >
 > Tend to agree.
 > Then, don't change function name but add some comment?
 >
 > /*
 >  * allow pages from fallback if page allocator can't find free page in your nid. >  * If you want to allocate page from exact node, please use
 > __GFP_THISNODE flags with
 >  * gfp_mask.
 >  */
 > static inline struct page *alloc_pages_exact_node(....
 >

I vote for this rather than renaming.

 There are two functions
        allo_pages_node()
        alloc_pages_exact_node().

 Sane progmrammers tend to see implementation details if there are 2 similar functions.

 If I name the function,
        alloc_pages_node_verify_nid() ?

 I think /* This doesn't support nid=-1, automatic behavior. */ is necessary as comment.

 OFF_TOPIC

 If you want renaming,  I think we should define NID=-1 as

 #define ARBITRARY_NID           (-1) or
 #define CURRENT_NID             (-1) or
 #define AUTO_NID                (-1)

 or some. Then, we'll have concensus of NID=-1 support. (Maybe some amount of programmers don't know what NID=-1 means.)

Quoted 8 lines

 The function will be
        alloc_pages_node_no_auto_nid() /* AUTO_NID is not supported by this */ or
        alloc_pages_node_veryfy_nid()

 Maybe patch will be bigger and may fail after discussion. But it seems worth to try.

Quoted 7 lines

Hm..It's a bit bigger.
Actually, what I want to do was in my original mail several days ago, the title is "mempolicy:add GFP_THISNODE when allocing new page"

What I concern is *just* we shouldn't fallback to other nodes if the dest node haven't enough free pages during migrate_pages().

Hmm. your patch for mempolicy seems good and it's BUGFIX. So, this patch should go as it is.

If you want to add comments to alloc_pages_exact_node(), please do.

Quoted 7 lines

But I think it's better to divide BUGFIX and CLEANUP patches.

I'll ack your patch for mempolicy.

Acked-by KAMEZAWA Hiroyuki <[email protected]>

Naming issue never needs quick fix. How about repositing as it is ? Minchan, how do you think ?

I feel it would be better to weed out function usage when I saw alloc_slab_page at least.

/*
* Slab allocation and freeing
*/
static inline struct page *alloc_slab_page(gfp_t flags, int node,
struct kmem_cache_order_objects oo)

{
       ...
       if (node == -1)
                return alloc_pages(flags, order);
        else
                return alloc_pages_node(node, flags, order);
}
Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#13 lore raw
In reply to Bob Liu
Quoted 63 lines · Bob Liu
Quoted 62 lines · KAMEZAWA Hiroyuki

On Tue, 13 Apr 2010 13:34:52 +0900

Quoted 1 line · Minchan Kim

On Tue, Apr 13, 2010 at 1:43 AM, Mel Gorman <[email protected]> wrote:

 > > On Sat, Apr 10, 2010 at 07:49:32PM +0800, Bob Liu wrote:
 > >> Since alloc_pages_exact_node() is not for allocate page from
 > >> exact node but just for removing check of node's valid,
 > >> rename it to alloc_pages_from_valid_node(). Else will make
 > >> people misunderstanding.
 > >>
 > >
 > > I don't know about this change either but as I introduced the original > > function name, I am biased. My reading of it is - allocate me pages and > > I know exactly which node I need. I see how it it could be read as > > "allocate me pages from exactly this node" but I don't feel the new > > naming is that much clearer either.
 >
 > Tend to agree.
 > Then, don't change function name but add some comment?
 >
 > /*
 >  * allow pages from fallback if page allocator can't find free page in your nid. >  * If you want to allocate page from exact node, please use
 > __GFP_THISNODE flags with
 >  * gfp_mask.
 >  */
 > static inline struct page *alloc_pages_exact_node(....
 >

I vote for this rather than renaming.

 There are two functions
        allo_pages_node()
        alloc_pages_exact_node().

 Sane progmrammers tend to see implementation details if there are 2 similar functions.

 If I name the function,
        alloc_pages_node_verify_nid() ?

 I think /* This doesn't support nid=-1, automatic behavior. */ is necessary as comment.

 OFF_TOPIC

 If you want renaming,  I think we should define NID=-1 as

 #define ARBITRARY_NID           (-1) or
 #define CURRENT_NID             (-1) or
 #define AUTO_NID                (-1)

 or some. Then, we'll have concensus of NID=-1 support. (Maybe some amount of programmers don't know what NID=-1 means.)

 The function will be
        alloc_pages_node_no_auto_nid() /* AUTO_NID is not supported by this */ or
        alloc_pages_node_veryfy_nid()

Quoted 7 lines
Quoted 3 lines

 Maybe patch will be bigger and may fail after discussion. But it seems worth to try.

Hm..It's a bit bigger.
Actually, what I want to do was in my original mail several days ago, the title is "mempolicy:add GFP_THISNODE when allocing new page"

Yes. At that time, I suggested new naming.
I agreed this patch and Mel didn't opposed it if others think new naming is better.

I still open my mind with new naming if you suggest better naming. How about 'allocate_pages_explicit_node?'

Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Reviewed-by
#14 lore raw
In reply to Bob Liu
Quoted 74 lines · Bob Liu
Quoted 57 lines · KAMEZAWA Hiroyuki

On Tue, 13 Apr 2010 13:34:52 +0900

Quoted 1 line · Minchan Kim

On Tue, Apr 13, 2010 at 1:43 AM, Mel Gorman <[email protected]> wrote:

 > > On Sat, Apr 10, 2010 at 07:49:32PM +0800, Bob Liu wrote:
 > >> Since alloc_pages_exact_node() is not for allocate page from
 > >> exact node but just for removing check of node's valid,
 > >> rename it to alloc_pages_from_valid_node(). Else will make
 > >> people misunderstanding.
 > >>
 > >
 > > I don't know about this change either but as I introduced the original > > function name, I am biased. My reading of it is - allocate me pages and > > I know exactly which node I need. I see how it it could be read as > > "allocate me pages from exactly this node" but I don't feel the new > > naming is that much clearer either.
 >
 > Tend to agree.
 > Then, don't change function name but add some comment?
 >
 > /*
 >  * allow pages from fallback if page allocator can't find free page in your nid. >  * If you want to allocate page from exact node, please use
 > __GFP_THISNODE flags with
 >  * gfp_mask.
 >  */
 > static inline struct page *alloc_pages_exact_node(....
 >

I vote for this rather than renaming.

 There are two functions
        allo_pages_node()
        alloc_pages_exact_node().

 Sane progmrammers tend to see implementation details if there are 2 similar functions.

 If I name the function,
        alloc_pages_node_verify_nid() ?

 I think /* This doesn't support nid=-1, automatic behavior. */ is necessary as comment.

 OFF_TOPIC

 If you want renaming,  I think we should define NID=-1 as

 #define ARBITRARY_NID           (-1) or
 #define CURRENT_NID             (-1) or
 #define AUTO_NID                (-1)

 or some. Then, we'll have concensus of NID=-1 support. (Maybe some amount of programmers don't know what NID=-1 means.)

Quoted 8 lines

 The function will be
        alloc_pages_node_no_auto_nid() /* AUTO_NID is not supported by this */ or
        alloc_pages_node_veryfy_nid()

 Maybe patch will be bigger and may fail after discussion. But it seems worth to try.

Hm..It's a bit bigger.
Actually, what I want to do was in my original mail several days ago, the title is "mempolicy:add GFP_THISNODE when allocing new page"

What I concern is *just* we shouldn't fallback to other nodes if the dest node haven't enough free pages during migrate_pages().

Quoted 7 lines

The detail is below:
In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>
Reviewed-by Minchan Kim <[email protected]>

Long time ago, I already reviewed this patch with Bob. :)

Signature · 8 lines
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#15 lore raw
In reply to Bob Liu
Quoted 63 lines · Bob Liu
Quoted 62 lines · KAMEZAWA Hiroyuki

On Tue, 13 Apr 2010 13:34:52 +0900

Quoted 17 lines · Minchan Kim
Quoted 7 lines · Mel Gorman
Quoted 5 lines · Bob Liu

Since alloc_pages_exact_node() is not for allocate page from exact node but just for removing check of node's valid, rename it to alloc_pages_from_valid_node(). Else will make people misunderstanding.

Quoted 5 lines

I don't know about this change either but as I introduced the original function name, I am biased. My reading of it is - allocate me pages and I know exactly which node I need. I see how it it could be read as "allocate me pages from exactly this node" but I don't feel the new naming is that much clearer either.

Tend to agree.
Then, don't change function name but add some comment?

Quoted 8 lines

/*
* allow pages from fallback if page allocator can't find free page in your nid.
* If you want to allocate page from exact node, please use
__GFP_THISNODE flags with
* gfp_mask.
*/
static inline struct page *alloc_pages_exact_node(....

I vote for this rather than renaming.

There are two functions
allo_pages_node()
alloc_pages_exact_node().

Sane progmrammers tend to see implementation details if there are 2 similar functions.

If I name the function,
alloc_pages_node_verify_nid() ?

I think /* This doesn't support nid=-1, automatic behavior. */ is necessary as comment.

OFF_TOPIC

If you want renaming, I think we should define NID=-1 as

 #define ARBITRARY_NID           (-1) or
 #define CURRENT_NID             (-1) or
 #define AUTO_NID                (-1)

or some. Then, we'll have concensus of NID=-1 support. (Maybe some amount of programmers don't know what NID=-1 means.)

The function will be
alloc_pages_node_no_auto_nid() /* AUTO_NID is not supported by this */
or
alloc_pages_node_veryfy_nid()

Quoted 8 lines
Quoted 3 lines

Maybe patch will be bigger and may fail after discussion. But it seems worth to try.

Hm..It's a bit bigger.
Actually, what I want to do was in my original mail several days ago, the title is "mempolicy:add GFP_THISNODE when allocing new page"

Sorry Bob, I still haven't actually read that thread. There has been a lot going on :(

Quoted 10 lines

What I concern is *just* we shouldn't fallback to other nodes if the dest node haven't enough free pages during migrate_pages().

The detail is below:
In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

This appears to be a valid bug fix. I agree that the way things are structured that __GFP_THISNODE should be used in new_node_page(). But maybe a follow-on patch is also required. The behaviour is now;

o new_node_page will not return NULL if the target node is empty (fine).
o migrate_pages will translate this into -ENOMEM (fine)
o do_migrate_pages breaks early if it gets -ENOMEM ?

It's the last part I'd like you to double check. migrate_pages() takes a nodemask of allowed nodes to migrate to. Rather than sending this down to the allocator, it iterates over the nodes allowed in the mask. If one of those nodes is full, it returns -ENOMEM.

If -ENOMEM is returned from migrate_pages, should it not move to the next node?

Quoted 20 lines
---
 mm/mempolicy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
mm/mempolicy.c +2 -1
index 08f40a2..fc5ddf5 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page,
 struct list_head *pagelist,
 
 static struct page *new_node_page(struct page *page, unsigned long
 node, int **x)
 {
-       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node,
+                               GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
 }

Thanks.

Signature · 9 lines
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#16 lore raw
Quoted 3 lines · Mel Gorman

This appears to be a valid bug fix. I agree that the way things are structured that __GFP_THISNODE should be used in new_node_page(). But maybe a follow-on patch is also required. The behaviour is now;

What bug is being fixed? migrate_pages() is a best effort approach. __GFP_THISNODE is used when allocation on a specific node is absolutely required for correctness (as in SLAB).

Quoted 1 line

If -ENOMEM is returned from migrate_pages, should it not move to the next node?

Which it does now if you dont apply the "bugfix".

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>