Re: [PATCH] mm/page_owner: Align with pageblock_nr pages

[Vlastimil Babka <vbabka@suse.cz>]

On 07/21/2016 03:43 PM, Michal Hocko wrote:
> On Thu 21-07-16 14:21:46, Vlastimil Babka wrote:
>> On 07/20/2016 09:24 AM, Michal Hocko wrote:
>>
>>> Should init_pages_in_zone depend on something
>>> like HUGETLB? Is this even correct I would have expected that we should
>>> initialize in the page block steps so MAX_ORDER_NR_PAGES. Could you
>>> clarify Joonsoo, please?
>>
>> On !CONFIG_HOLES_IN_ZONE systems, pfn_valid() should give the same outcome
>> within MAX_ORDER_NR_PAGES blocks (modulo zone boundaries). So the ALIGN
>> using MAX_ORDER_NR_PAGES is correct for these systems. What's somewhat weird
>> is that the rest of the for loop uses pageblock_nr_pages, but it doesn't
>> affect the outcome.
>>
>> On CONFIG_HOLES_IN_ZONE the situation is less clear. The hole can be
>> theoretically anywhere within MAX_ORDER_NR_PAGES, including the first pfn.
>> If it's the first pfn, init_pages_in_zone() will skip MAX_ORDER_NR_PAGES.
>> The patch helps if the hole is e.g. the first 2MB of a 4MB pageblock... then
>> the second 2MB will be picked up after this patch. But it's still not
>> thorough in all situations. Strictly speaking, one these systems one would
>> have to avoid the MAX_ORDER_NR_PAGES skip completely, and just check each
>> pfn one by one to be sure nothing is missed.
>>
>> But that's potentially costly, so for example, __pageblock_pfn_to_page()
>> (that originated in compaction) assumes that the hole is in the middle, and
>> checks first and last pfn of pageblock. So it has a pageblock granularity
>> like this patch, but still is more restrictive.
>>
>> I wish there was a better solution that would get used everywhere...
>> possibly making the CONFIG_HOLES_IN_ZONE configs also declare the
>> granularity of holes, so we don't need to check each pfn...
>
> Ehm, head spins... So this suggests that MAX_ORDER_NR_PAGES sounds like
> a better iterator for systems without holes and neither
> pageblock_nr_pages nor MAX_ORDER_NR_PAGES for reliably for systems with
> holes. Did I get it right?

Yes, AFAIU.

> If yes is the patch an improvement at all?

Only marginal. It does make the function more consistent, and will 
improve cases where hole is restricted to first pageblock within 
MAX_ORDER block. On systems without sub-MAX_ORDER holes, it will cause 
more iterations to skip the >=MAX_ORDER holes, but that's negligible.
So I'm not against the patch, but would like to hear if it solved a 
problem in practice (i.e. pageblock-sized holes are common), or is just 
theoretical from code review.

>>>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>>>> ---
>>>>  mm/page_owner.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/page_owner.c b/mm/page_owner.c
>>>> index c6cda3e..aa2c486 100644
>>>> --- a/mm/page_owner.c
>>>> +++ b/mm/page_owner.c
>>>> @@ -310,7 +310,7 @@ static void init_pages_in_zone(pg_data_t *pgdat, struct zone *zone)
>>>>  	 */
>>>>  	for (; pfn < end_pfn; ) {
>>>>  		if (!pfn_valid(pfn)) {
>>>> -			pfn = ALIGN(pfn + 1, MAX_ORDER_NR_PAGES);
>>>> +			pfn = ALIGN(pfn + 1, pageblock_nr_pages);
>>>>  			continue;
>>>>  		}
>>>>
>>>> --
>>>> 1.8.3.1
>>>
>>
>> --
>> 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>
>

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