| Message ID | 20201127141900.43348-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/page_alloc: Do not isolate redundant pageblock | expand |
On 11/27/20 3:19 PM, Muchun Song wrote: > Current pageblock isolation logic could isolate each pageblock individually > since commit d9dddbf55667 ("mm/page_alloc: prevent merging between isolated > and other pageblocks"). So we not need to concern about page allocator > merges buddies from different pageblocks and changes MIGRATE_ISOLATE to > some other migration type. Yeah, that should be the case now. > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > mm/page_alloc.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index cefbef32bf4a..608a2c2b8ab7 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8313,16 +8313,14 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > } > > #ifdef CONFIG_CONTIG_ALLOC > -static unsigned long pfn_max_align_down(unsigned long pfn) > +static unsigned long pfn_align_down(unsigned long pfn) > { > - return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES, > - pageblock_nr_pages) - 1); > + return pfn & ~(pageblock_nr_pages - 1); > } > > -static unsigned long pfn_max_align_up(unsigned long pfn) > +static unsigned long pfn_align_up(unsigned long pfn) > { > - return ALIGN(pfn, max_t(unsigned long, MAX_ORDER_NR_PAGES, > - pageblock_nr_pages)); > + return ALIGN(pfn, pageblock_nr_pages); > } How bout just removing these wrappers completely and using ALIGN and ALIGN_DOWN directly, as there are just two uses for each? > /* [start, end) must belong to a single zone. */ > @@ -8415,14 +8413,6 @@ int alloc_contig_range(unsigned long start, unsigned long end, > INIT_LIST_HEAD(&cc.migratepages); > > /* > - * What we do here is we mark all pageblocks in range as > - * MIGRATE_ISOLATE. Because pageblock and max order pages may > - * have different sizes, and due to the way page allocator > - * work, we align the range to biggest of the two pages so > - * that page allocator won't try to merge buddies from > - * different pageblocks and change MIGRATE_ISOLATE to some > - * other migration type. > - * > * Once the pageblocks are marked as MIGRATE_ISOLATE, we > * migrate the pages from an unaligned range (ie. pages that > * we are interested in). This will put all the pages in > @@ -8438,8 +8428,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, > * put back to page allocator so that buddy can use them. > */ > > - ret = start_isolate_page_range(pfn_max_align_down(start), > - pfn_max_align_up(end), migratetype, 0); > + ret = start_isolate_page_range(pfn_align_down(start), pfn_align_up(end), > + migratetype, 0); > if (ret) > return ret; > > @@ -8522,8 +8512,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, > free_contig_range(end, outer_end - end); > > done: > - undo_isolate_page_range(pfn_max_align_down(start), > - pfn_max_align_up(end), migratetype); > + undo_isolate_page_range(pfn_align_down(start), pfn_align_up(end), > + migratetype); > return ret; > } > EXPORT_SYMBOL(alloc_contig_range); >
On Fri, Nov 27, 2020 at 11:17 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 11/27/20 3:19 PM, Muchun Song wrote: > > Current pageblock isolation logic could isolate each pageblock individually > > since commit d9dddbf55667 ("mm/page_alloc: prevent merging between isolated > > and other pageblocks"). So we not need to concern about page allocator > > merges buddies from different pageblocks and changes MIGRATE_ISOLATE to > > some other migration type. > > Yeah, that should be the case now. > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > mm/page_alloc.c | 26 ++++++++------------------ > > 1 file changed, 8 insertions(+), 18 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index cefbef32bf4a..608a2c2b8ab7 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -8313,16 +8313,14 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > > } > > > > #ifdef CONFIG_CONTIG_ALLOC > > -static unsigned long pfn_max_align_down(unsigned long pfn) > > +static unsigned long pfn_align_down(unsigned long pfn) > > { > > - return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES, > > - pageblock_nr_pages) - 1); > > + return pfn & ~(pageblock_nr_pages - 1); > > } > > > > -static unsigned long pfn_max_align_up(unsigned long pfn) > > +static unsigned long pfn_align_up(unsigned long pfn) > > { > > - return ALIGN(pfn, max_t(unsigned long, MAX_ORDER_NR_PAGES, > > - pageblock_nr_pages)); > > + return ALIGN(pfn, pageblock_nr_pages); > > } > > How bout just removing these wrappers completely and using ALIGN and ALIGN_DOWN > directly, as there are just two uses for each? Sounds good to me. Will do. > > > /* [start, end) must belong to a single zone. */ > > @@ -8415,14 +8413,6 @@ int alloc_contig_range(unsigned long start, unsigned long end, > > INIT_LIST_HEAD(&cc.migratepages); > > > > /* > > - * What we do here is we mark all pageblocks in range as > > - * MIGRATE_ISOLATE. Because pageblock and max order pages may > > - * have different sizes, and due to the way page allocator > > - * work, we align the range to biggest of the two pages so > > - * that page allocator won't try to merge buddies from > > - * different pageblocks and change MIGRATE_ISOLATE to some > > - * other migration type. > > - * > > * Once the pageblocks are marked as MIGRATE_ISOLATE, we > > * migrate the pages from an unaligned range (ie. pages that > > * we are interested in). This will put all the pages in > > @@ -8438,8 +8428,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, > > * put back to page allocator so that buddy can use them. > > */ > > > > - ret = start_isolate_page_range(pfn_max_align_down(start), > > - pfn_max_align_up(end), migratetype, 0); > > + ret = start_isolate_page_range(pfn_align_down(start), pfn_align_up(end), > > + migratetype, 0); > > if (ret) > > return ret; > > > > @@ -8522,8 +8512,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, > > free_contig_range(end, outer_end - end); > > > > done: > > - undo_isolate_page_range(pfn_max_align_down(start), > > - pfn_max_align_up(end), migratetype); > > + undo_isolate_page_range(pfn_align_down(start), pfn_align_up(end), > > + migratetype); > > return ret; > > } > > EXPORT_SYMBOL(alloc_contig_range); > > >
On 27.11.20 15:19, Muchun Song wrote: > Current pageblock isolation logic could isolate each pageblock individually > since commit d9dddbf55667 ("mm/page_alloc: prevent merging between isolated > and other pageblocks"). So we not need to concern about page allocator > merges buddies from different pageblocks and changes MIGRATE_ISOLATE to > some other migration type. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > mm/page_alloc.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index cefbef32bf4a..608a2c2b8ab7 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8313,16 +8313,14 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, > } > > #ifdef CONFIG_CONTIG_ALLOC > -static unsigned long pfn_max_align_down(unsigned long pfn) > +static unsigned long pfn_align_down(unsigned long pfn) > { > - return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES, > - pageblock_nr_pages) - 1); > + return pfn & ~(pageblock_nr_pages - 1); > } > > -static unsigned long pfn_max_align_up(unsigned long pfn) > +static unsigned long pfn_align_up(unsigned long pfn) > { > - return ALIGN(pfn, max_t(unsigned long, MAX_ORDER_NR_PAGES, > - pageblock_nr_pages)); > + return ALIGN(pfn, pageblock_nr_pages); > } > > /* [start, end) must belong to a single zone. */ > @@ -8415,14 +8413,6 @@ int alloc_contig_range(unsigned long start, unsigned long end, > INIT_LIST_HEAD(&cc.migratepages); > > /* > - * What we do here is we mark all pageblocks in range as > - * MIGRATE_ISOLATE. Because pageblock and max order pages may > - * have different sizes, and due to the way page allocator > - * work, we align the range to biggest of the two pages so > - * that page allocator won't try to merge buddies from > - * different pageblocks and change MIGRATE_ISOLATE to some > - * other migration type. > - * > * Once the pageblocks are marked as MIGRATE_ISOLATE, we > * migrate the pages from an unaligned range (ie. pages that > * we are interested in). This will put all the pages in > @@ -8438,8 +8428,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, > * put back to page allocator so that buddy can use them. > */ > > - ret = start_isolate_page_range(pfn_max_align_down(start), > - pfn_max_align_up(end), migratetype, 0); > + ret = start_isolate_page_range(pfn_align_down(start), pfn_align_up(end), > + migratetype, 0); > if (ret) > return ret; > > @@ -8522,8 +8512,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, > free_contig_range(end, outer_end - end); > > done: > - undo_isolate_page_range(pfn_max_align_down(start), > - pfn_max_align_up(end), migratetype); > + undo_isolate_page_range(pfn_align_down(start), pfn_align_up(end), > + migratetype); > return ret; > } > EXPORT_SYMBOL(alloc_contig_range); > Last time I checked, set_migratetype_isolate()->has_unmovable_pages() was not prepared for that in case of !CMA and !ZONE_MOVABLE. Assume you have an unmovable MAX_ORDER - 1 page that spans two pageblocks (e.g., x86-64). Assume you try to isolate the second pageblock. IIRC, you would answer "yes", as the refcount of all involved pages is 0. How did you test this works as expected?
On 27.11.20 17:54, David Hildenbrand wrote: > On 27.11.20 15:19, Muchun Song wrote: >> Current pageblock isolation logic could isolate each pageblock individually >> since commit d9dddbf55667 ("mm/page_alloc: prevent merging between isolated >> and other pageblocks"). So we not need to concern about page allocator >> merges buddies from different pageblocks and changes MIGRATE_ISOLATE to >> some other migration type. >> >> Signed-off-by: Muchun Song <songmuchun@bytedance.com> >> --- >> mm/page_alloc.c | 26 ++++++++------------------ >> 1 file changed, 8 insertions(+), 18 deletions(-) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index cefbef32bf4a..608a2c2b8ab7 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -8313,16 +8313,14 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, >> } >> >> #ifdef CONFIG_CONTIG_ALLOC >> -static unsigned long pfn_max_align_down(unsigned long pfn) >> +static unsigned long pfn_align_down(unsigned long pfn) >> { >> - return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES, >> - pageblock_nr_pages) - 1); >> + return pfn & ~(pageblock_nr_pages - 1); >> } >> >> -static unsigned long pfn_max_align_up(unsigned long pfn) >> +static unsigned long pfn_align_up(unsigned long pfn) >> { >> - return ALIGN(pfn, max_t(unsigned long, MAX_ORDER_NR_PAGES, >> - pageblock_nr_pages)); >> + return ALIGN(pfn, pageblock_nr_pages); >> } >> >> /* [start, end) must belong to a single zone. */ >> @@ -8415,14 +8413,6 @@ int alloc_contig_range(unsigned long start, unsigned long end, >> INIT_LIST_HEAD(&cc.migratepages); >> >> /* >> - * What we do here is we mark all pageblocks in range as >> - * MIGRATE_ISOLATE. Because pageblock and max order pages may >> - * have different sizes, and due to the way page allocator >> - * work, we align the range to biggest of the two pages so >> - * that page allocator won't try to merge buddies from >> - * different pageblocks and change MIGRATE_ISOLATE to some >> - * other migration type. >> - * >> * Once the pageblocks are marked as MIGRATE_ISOLATE, we >> * migrate the pages from an unaligned range (ie. pages that >> * we are interested in). This will put all the pages in >> @@ -8438,8 +8428,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, >> * put back to page allocator so that buddy can use them. >> */ >> >> - ret = start_isolate_page_range(pfn_max_align_down(start), >> - pfn_max_align_up(end), migratetype, 0); >> + ret = start_isolate_page_range(pfn_align_down(start), pfn_align_up(end), >> + migratetype, 0); >> if (ret) >> return ret; >> >> @@ -8522,8 +8512,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, >> free_contig_range(end, outer_end - end); >> >> done: >> - undo_isolate_page_range(pfn_max_align_down(start), >> - pfn_max_align_up(end), migratetype); >> + undo_isolate_page_range(pfn_align_down(start), pfn_align_up(end), >> + migratetype); >> return ret; >> } >> EXPORT_SYMBOL(alloc_contig_range); >> > > Last time I checked, set_migratetype_isolate()->has_unmovable_pages() > was not prepared for that in case of !CMA and !ZONE_MOVABLE. > > Assume you have an unmovable MAX_ORDER - 1 page that spans two > pageblocks (e.g., x86-64). Assume you try to isolate the second > pageblock. IIRC, you would answer "yes", as the refcount of all involved > pages is 0. > > > How did you test this works as expected? > And I forgot to mention, in case it's a movable MAX_ORDER - 1 page (or simply free), would __alloc_contig_migrate_range() do the right thing and migrate the whole page?
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index cefbef32bf4a..608a2c2b8ab7 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -8313,16 +8313,14 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page, } #ifdef CONFIG_CONTIG_ALLOC -static unsigned long pfn_max_align_down(unsigned long pfn) +static unsigned long pfn_align_down(unsigned long pfn) { - return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES, - pageblock_nr_pages) - 1); + return pfn & ~(pageblock_nr_pages - 1); } -static unsigned long pfn_max_align_up(unsigned long pfn) +static unsigned long pfn_align_up(unsigned long pfn) { - return ALIGN(pfn, max_t(unsigned long, MAX_ORDER_NR_PAGES, - pageblock_nr_pages)); + return ALIGN(pfn, pageblock_nr_pages); } /* [start, end) must belong to a single zone. */ @@ -8415,14 +8413,6 @@ int alloc_contig_range(unsigned long start, unsigned long end, INIT_LIST_HEAD(&cc.migratepages); /* - * What we do here is we mark all pageblocks in range as - * MIGRATE_ISOLATE. Because pageblock and max order pages may - * have different sizes, and due to the way page allocator - * work, we align the range to biggest of the two pages so - * that page allocator won't try to merge buddies from - * different pageblocks and change MIGRATE_ISOLATE to some - * other migration type. - * * Once the pageblocks are marked as MIGRATE_ISOLATE, we * migrate the pages from an unaligned range (ie. pages that * we are interested in). This will put all the pages in @@ -8438,8 +8428,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, * put back to page allocator so that buddy can use them. */ - ret = start_isolate_page_range(pfn_max_align_down(start), - pfn_max_align_up(end), migratetype, 0); + ret = start_isolate_page_range(pfn_align_down(start), pfn_align_up(end), + migratetype, 0); if (ret) return ret; @@ -8522,8 +8512,8 @@ int alloc_contig_range(unsigned long start, unsigned long end, free_contig_range(end, outer_end - end); done: - undo_isolate_page_range(pfn_max_align_down(start), - pfn_max_align_up(end), migratetype); + undo_isolate_page_range(pfn_align_down(start), pfn_align_up(end), + migratetype); return ret; } EXPORT_SYMBOL(alloc_contig_range);
Current pageblock isolation logic could isolate each pageblock individually since commit d9dddbf55667 ("mm/page_alloc: prevent merging between isolated and other pageblocks"). So we not need to concern about page allocator merges buddies from different pageblocks and changes MIGRATE_ISOLATE to some other migration type. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/page_alloc.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)