On Wed, Mar 28, 2018 at 01:58:53PM +0200, Michal Hocko wrote:
>On Wed 28-03-18 11:47:52, Wei Yang wrote:
>[...]
>> +/**
>> + * first_mem_pfn - get the first memory pfn
>> + * @i: an integer used as an indicator
>> + * @nid: node selector, %MAX_NUMNODES for all nodes
>> + * @p_first: ptr to ulong for first pfn of the range, can be %NULL
>> + */
>> +#define first_mem_pfn(i, nid, p_first) \
>> + __next_mem_pfn_range(&i, nid, p_first, NULL, NULL)
>> +
>
>Is this really something that we want to export to all users? And if
>that is the case is the documenation really telling user how to use it?
>
Yep, I am not good at the documentation. I really struggled a while on it, but
you know it still looks not that good.
How about changing the document to the following in case this macro is still
alive.
/**
* first_mem_pfn - get the first memory pfn after index i on node nid
* @i: index to memblock.memory.regions
* @nid: node selector, %MAX_NUMNODES for all nodes
* @p_first: ptr to ulong for first pfn of the range, can be %NULL
*/
>> /**
>> * for_each_mem_pfn_range - early memory pfn range iterator
>> * @i: an integer used as loop variable
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 635d7dd29d7f..8c964dcc3a9e 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -6365,14 +6365,16 @@ unsigned long __init node_map_pfn_alignment(void)
>> /* Find the lowest pfn for a node */
>> static unsigned long __init find_min_pfn_for_node(int nid)
>> {
>> - unsigned long min_pfn = ULONG_MAX;
>> - unsigned long start_pfn;
>> - int i;
>> + unsigned long min_pfn;
>> + int i = -1;
>>
>> - for_each_mem_pfn_range(i, nid, &start_pfn, NULL, NULL)
>> - min_pfn = min(min_pfn, start_pfn);
>> + /*
>> + * The first pfn on nid node is the minimal one, as the pfn's are
>> + * stored in ascending order.
>> + */
>> + first_mem_pfn(i, nid, &min_pfn);
>>
>> - if (min_pfn == ULONG_MAX) {
>> + if (i == -1) {
>> pr_warn("Could not find start_pfn for node %d\n", nid);
>> return 0;
>> }
>
>I would just open code it. Other than that I strongly suspect this will
>not have any measurable impact becauser we usually only have handfull of
>memory ranges but why not. Just make the new implementation less ugly
>than it is cuurrently - e.g. opencode first_mem_pfn and you can add
Open code here means use __next_mem_pfn_range() directly instead of using
first_mem_pfn()?
>Acked-by: Michal Hocko <mhocko@xxxxxxxx>
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
