On Tue, Apr 13, 2010 at 4:32 PM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> 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 ?
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@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href