Re: [PATCH] code clean rename alloc_pages_exact_node()

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

On Tue, Apr 13, 2010 at 4:09 PM, Bob Liu <lliubbo@gmail.com> wrote:
> 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>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

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

-- 
Kind regards,
Minchan Kim

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