Re: [PATCH] code clean rename alloc_pages_exact_node()

[Bob Liu <lliubbo@gmail.com>]

On 4/13/10, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Tue, 13 Apr 2010 13:34:52 +0900
>
> Minchan Kim <minchan.kim@gmail.com> wrote:
>
>
> > On Tue, Apr 13, 2010 at 1:43 AM, Mel Gorman <mel@csn.ul.ie> 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()
>
>  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 <lliubbo@gmail.com>
---
 mm/mempolicy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 08f40a2..fc5ddf5 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -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.
-- 
Regards,
--Bob

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