[PATCH] mm/page_owner: Align with pageblock_nr pages

[PATCH] mm/page_owner: Align with pageblock_nr pages

[zhongjiang]

From: zhong jiang <zhongjiang@huawei.com>

when pfn_valid(pfn) return false, pfn should be align with
pageblock_nr_pages other than MAX_ORDER_NR_PAGES in
init_pages_in_zone, because the skipped 2M may be valid pfn,
as a result, early allocated count will not be accurate.

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>

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

[Michal Hocko]

On Tue 19-07-16 22:22:16, zhongjiang wrote:
> From: zhong jiang <zhongjiang@huawei.com>
> 
> when pfn_valid(pfn) return false, pfn should be align with
> pageblock_nr_pages other than MAX_ORDER_NR_PAGES in
> init_pages_in_zone, because the skipped 2M may be valid pfn,
> as a result, early allocated count will not be accurate.

I really do not understand this changelog. I thought that
MAX_ORDER_NR_PAGES and pageblock_nr_pages are the same thing but they
might not be for HUGETLB. 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?

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

-- 
Michal Hocko
SUSE Labs

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

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

[Vlastimil Babka]

On 07/20/2016 09:24 AM, Michal Hocko wrote:
> On Tue 19-07-16 22:22:16, zhongjiang wrote:
>> From: zhong jiang <zhongjiang@huawei.com>
>>
>> when pfn_valid(pfn) return false, pfn should be align with
>> pageblock_nr_pages other than MAX_ORDER_NR_PAGES in
>> init_pages_in_zone, because the skipped 2M may be valid pfn,
>> as a result, early allocated count will not be accurate.
>
> I really do not understand this changelog. I thought that
> MAX_ORDER_NR_PAGES and pageblock_nr_pages are the same thing but they
> might not be for HUGETLB.

The common situation on x86 is that pageblock is 512 pages (2MB) and 
MAX_ORDER_NR_PAGES is 1024 (4MB).

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

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

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

[Michal Hocko]

On Thu 21-07-16 14:21:46, Vlastimil Babka wrote:
> On 07/20/2016 09:24 AM, Michal Hocko wrote:
> > On Tue 19-07-16 22:22:16, zhongjiang wrote:
> > > From: zhong jiang <zhongjiang@huawei.com>
> > > 
> > > when pfn_valid(pfn) return false, pfn should be align with
> > > pageblock_nr_pages other than MAX_ORDER_NR_PAGES in
> > > init_pages_in_zone, because the skipped 2M may be valid pfn,
> > > as a result, early allocated count will not be accurate.
> > 
> > I really do not understand this changelog. I thought that
> > MAX_ORDER_NR_PAGES and pageblock_nr_pages are the same thing but they
> > might not be for HUGETLB.
> 
> The common situation on x86 is that pageblock is 512 pages (2MB) and
> MAX_ORDER_NR_PAGES is 1024 (4MB).
> 
> > 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?

If yes is the patch an improvement at all?

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

-- 
Michal Hocko
SUSE Labs

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

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

[Vlastimil Babka]

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>