On Tue, 13 Apr 2010 15:09:42 +0800
Bob Liu <lliubbo@xxxxxxxxx> wrote:
> On 4/13/10, KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> > On Tue, 13 Apr 2010 13:34:52 +0900
> >
> > Minchan Kim <minchan.kim@xxxxxxxxx> wrote:
> >
> >
> > > On Tue, Apr 13, 2010 at 1:43 AM, Mel Gorman <mel@xxxxxxxxx> 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().
>
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 <kamezawa.hiroyu@xxxxxxxxxxxxxx>
Naming issue never needs quick fix. How about repositing as it is ?
Minchan, how do you think ?
Thanks,
-Kame
> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>