* [PATCH] mm/page_owner: Align with pageblock_nr pages @ 2016-07-19 14:22 zhongjiang 2016-07-20 7:24 ` Michal Hocko 0 siblings, 1 reply; 5+ messages in thread From: zhongjiang @ 2016-07-19 14:22 UTC (permalink / raw) To: iamjoonsoo.kim, akpm, mhocko; +Cc: linux-mm 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> ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/page_owner: Align with pageblock_nr pages 2016-07-19 14:22 [PATCH] mm/page_owner: Align with pageblock_nr pages zhongjiang @ 2016-07-20 7:24 ` Michal Hocko 2016-07-21 12:21 ` Vlastimil Babka 0 siblings, 1 reply; 5+ messages in thread From: Michal Hocko @ 2016-07-20 7:24 UTC (permalink / raw) To: iamjoonsoo.kim, zhongjiang; +Cc: akpm, linux-mm 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/page_owner: Align with pageblock_nr pages 2016-07-20 7:24 ` Michal Hocko @ 2016-07-21 12:21 ` Vlastimil Babka 2016-07-21 13:43 ` Michal Hocko 0 siblings, 1 reply; 5+ messages in thread From: Vlastimil Babka @ 2016-07-21 12:21 UTC (permalink / raw) To: Michal Hocko, iamjoonsoo.kim, zhongjiang; +Cc: akpm, linux-mm 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/page_owner: Align with pageblock_nr pages 2016-07-21 12:21 ` Vlastimil Babka @ 2016-07-21 13:43 ` Michal Hocko 2016-07-21 13:50 ` Vlastimil Babka 0 siblings, 1 reply; 5+ messages in thread From: Michal Hocko @ 2016-07-21 13:43 UTC (permalink / raw) To: Vlastimil Babka; +Cc: iamjoonsoo.kim, zhongjiang, akpm, linux-mm 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/page_owner: Align with pageblock_nr pages 2016-07-21 13:43 ` Michal Hocko @ 2016-07-21 13:50 ` Vlastimil Babka 0 siblings, 0 replies; 5+ messages in thread From: Vlastimil Babka @ 2016-07-21 13:50 UTC (permalink / raw) To: Michal Hocko; +Cc: iamjoonsoo.kim, zhongjiang, akpm, linux-mm 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-07-21 13:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-19 14:22 [PATCH] mm/page_owner: Align with pageblock_nr pages zhongjiang 2016-07-20 7:24 ` Michal Hocko 2016-07-21 12:21 ` Vlastimil Babka 2016-07-21 13:43 ` Michal Hocko 2016-07-21 13:50 ` Vlastimil Babka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).