Re: [PATCH] code clean rename alloc_pages_exact_node()

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

On Tue, Apr 13, 2010 at 4:32 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Tue, 13 Apr 2010 15:09:42 +0800
> 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().
>>
>
> 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@jp.fujitsu.com>
>
> 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);
}


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