On Fri, Jul 10, 2015 at 12:35:55AM -0700, Andrew Morton wrote:
>On Fri, 10 Jul 2015 14:26:21 +0800 Wei Yang <weiyang@xxxxxxxxxxxxxxxxxx> wrote:
>
>> nr_node_ids records the highest possible node id, which is calculated by
>> scanning the bitmap node_states[N_POSSIBLE]. Current implementation scan
>> the bitmap from the beginning, which will scan the whole bitmap.
>>
>> This patch reverse the order by scanning from the end. By doing so, this
>> will save some time whose worst case is the best case of current
>> implementation.
>
>It hardly matters - setup_nr_node_ids() is called a single time, at boot.
Hi, Andrew,
Glad to receive your comment :-)
Yes, the hardly matters on the performance side, while scanning from the end is
a better way to me. Hope you like it.
>
>> --- a/include/linux/nodemask.h
>> +++ b/include/linux/nodemask.h
>> @@ -253,6 +253,12 @@ static inline int __first_node(const nodemask_t *srcp)
>> return min_t(int, MAX_NUMNODES, find_first_bit(srcp->bits, MAX_NUMNODES));
>> }
>>
>> +#define last_node(src) __last_node(&(src))
>> +static inline int __last_node(const nodemask_t *srcp)
>> +{
>> + return min_t(int, MAX_NUMNODES, find_last_bit(srcp->bits, MAX_NUMNODES));
>> +}
>
>hm. Why isn't this just
>
> return find_last_bit(srcp->bits, MAX_NUMNODES);
>
>?
I found this comment in the code:
/* FIXME: better would be to fix all architectures to never return
> MAX_NUMNODES, then the silly min_ts could be dropped. */
While I didn't find the original commit for this change, so not dear to change
the related code format.
>
>> @@ -360,10 +366,20 @@ static inline void __nodes_fold(nodemask_t *dstp, const nodemask_t *origp,
>> for ((node) = first_node(mask); \
>> (node) < MAX_NUMNODES; \
>> (node) = next_node((node), (mask)))
>> +
>> +static inline int highest_node_id(const nodemask_t possible)
>> +{
>> + return last_node(possible);
>> +}
>
>`possible' isn't a good identifier. This function doesn't *know* that
>its caller passed node_possible_map. Another caller could pass some
>other nodemask.
>
Agree. I would change it.
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5453,8 +5453,7 @@ void __init setup_nr_node_ids(void)
>> unsigned int node;
>> unsigned int highest = 0;
>
>The "= 0" can now be removed.
>
>> - for_each_node_mask(node, node_possible_map)
>> - highest = node;
>> + highest = highest_node_id(node_possible_map);
>
>I suspect we can just open-code a find_last_bit() here and all the
>infrastructure isn't needed.
>
This is reasonable. If so, the code would be more clear.
>> nr_node_ids = highest + 1;
>> }
>
>
>And I suspect the "#if MAX_NUMNODES > 1" around setup_nr_node_ids() can
>be removed. Because if MAX_NUMNODES is ever <= 1 when
>CONFIG_HAVE_MEMBLOCK_NODE_MAP=y, the kernel won't compile.
Hmm... for this one, I am not sure.
#define MAX_NUMNODES (1 << NODES_SHIFT)
#define NODES_SHIFT CONFIG_NODES_SHIFT
CONFIG_NODES_SHIFT depends on CONFIG_NEED_MULTIPLE_NODES, which depends on
CONFIG_DISCONTIGMEM or CONFIG_NUMA.
And I grep the kernel tree, not see other configuration would depend on
HAVE_MEMBLOCK_NODE_MAP.
So I am don't get a clear clue for this suspection.
--
Richard Yang
Help you, Help me
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>