| Message ID | 20210317111251.17808-6-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make alloc_contig_range handle Hugetlb pages | expand |
On 17.03.21 12:12, Oscar Salvador wrote: > pfn_range_valid_contig() bails out when it finds an in-use page or a > hugetlb page, among other things. > We can drop the in-use page check since __alloc_contig_pages can migrate > away those pages, and the hugetlb page check can go too since > isolate_migratepages_range is now capable of dealing with hugetlb pages. > Either way, those checks are racy so let the end function handle it > when the time comes. > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > Suggested-by: David Hildenbrand <david@redhat.com> > --- > mm/page_alloc.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 4cb455355f6d..50d73e68b79e 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8685,12 +8685,6 @@ static bool pfn_range_valid_contig(struct zone *z, unsigned long start_pfn, > > if (PageReserved(page)) > return false; > - > - if (page_count(page) > 0) > - return false; > - > - if (PageHuge(page)) > - return false; > } > return true; > } > Reviewed-by: David Hildenbrand <david@redhat.com>
On Wed 17-03-21 12:12:51, Oscar Salvador wrote: > pfn_range_valid_contig() bails out when it finds an in-use page or a > hugetlb page, among other things. > We can drop the in-use page check since __alloc_contig_pages can migrate > away those pages, and the hugetlb page check can go too since > isolate_migratepages_range is now capable of dealing with hugetlb pages. > Either way, those checks are racy so let the end function handle it > when the time comes. I haven't realized PageHuge check is done this early. This means that previous patches are not actually active until now which is not really greate for bisectability. Can we remove the HugePage check earlier? Act to the page_count check removal. We should rely on migrate_pages here. > Signed-off-by: Oscar Salvador <osalvador@suse.de> > Suggested-by: David Hildenbrand <david@redhat.com> > --- > mm/page_alloc.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 4cb455355f6d..50d73e68b79e 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8685,12 +8685,6 @@ static bool pfn_range_valid_contig(struct zone *z, unsigned long start_pfn, > > if (PageReserved(page)) > return false; > - > - if (page_count(page) > 0) > - return false; > - > - if (PageHuge(page)) > - return false; > } > return true; > } > -- > 2.16.3
On 17.03.21 15:31, Michal Hocko wrote: > On Wed 17-03-21 12:12:51, Oscar Salvador wrote: >> pfn_range_valid_contig() bails out when it finds an in-use page or a >> hugetlb page, among other things. >> We can drop the in-use page check since __alloc_contig_pages can migrate >> away those pages, and the hugetlb page check can go too since >> isolate_migratepages_range is now capable of dealing with hugetlb pages. >> Either way, those checks are racy so let the end function handle it >> when the time comes. > > I haven't realized PageHuge check is done this early. This means that > previous patches are not actually active until now which is not really > greate for bisectability. Can we remove the HugePage check earlier? alloc_contig_pages() vs. alloc_contig_range(). The patches are active for virtio-mem and CMA AFAIKS.
On Wed 17-03-21 15:36:35, David Hildenbrand wrote: > On 17.03.21 15:31, Michal Hocko wrote: > > On Wed 17-03-21 12:12:51, Oscar Salvador wrote: > > > pfn_range_valid_contig() bails out when it finds an in-use page or a > > > hugetlb page, among other things. > > > We can drop the in-use page check since __alloc_contig_pages can migrate > > > away those pages, and the hugetlb page check can go too since > > > isolate_migratepages_range is now capable of dealing with hugetlb pages. > > > Either way, those checks are racy so let the end function handle it > > > when the time comes. > > > > I haven't realized PageHuge check is done this early. This means that > > previous patches are not actually active until now which is not really > > greate for bisectability. Can we remove the HugePage check earlier? > > alloc_contig_pages() vs. alloc_contig_range(). The patches are active for > virtio-mem and CMA AFAIKS. yeah, I meant to say "are not actually fully active".
On Wed, Mar 17, 2021 at 04:03:06PM +0100, Michal Hocko wrote: > > alloc_contig_pages() vs. alloc_contig_range(). The patches are active for > > virtio-mem and CMA AFAIKS. > > yeah, I meant to say "are not actually fully active". We could place this patch earlier in this patchset. The only thing is that we would lose the prevalidation (at leat for HugeTLB page) which is done upfront to find later on that we do not support hugetlb handling in isolate_migratepates_block. So the bad thing about placing it earlier, is that wrt. hugetlb pages, alloc_gigantic_page will take longer to fail (when we already know that will fail). Then we have the page_count check, which is also racy and isolate_migratepages_block will take care of it. So I guess can think of this patch as a preparatory patch that removes racy checks that will be re-checked later on in the end function which does the actual handling. What do you think?
On Thu 18-03-21 09:44:19, Oscar Salvador wrote: > On Wed, Mar 17, 2021 at 04:03:06PM +0100, Michal Hocko wrote: > > > alloc_contig_pages() vs. alloc_contig_range(). The patches are active for > > > virtio-mem and CMA AFAIKS. > > > > yeah, I meant to say "are not actually fully active". > > We could place this patch earlier in this patchset. > The only thing is that we would lose the prevalidation (at leat for > HugeTLB page) which is done upfront to find later on that we do not > support hugetlb handling in isolate_migratepates_block. > So the bad thing about placing it earlier, is that wrt. hugetlb pages, > alloc_gigantic_page will take longer to fail (when we already know that > will fail). From a bisactability POV this shouldn't be a major concern. If you are too worried then just drop the HugePage check in the patch allowing to migrate free hugetlb pages. It is unlikely that somebody will run with that patch alone but if the said patch introduces some sort of bug it would be good to bisect down to it. > Then we have the page_count check, which is also racy and > isolate_migratepages_block will take care of it. > So I guess can think of this patch as a preparatory patch that removes racy > checks that will be re-checked later on in the end function which does > the actual handling. TBH, I do not care much about the page count check. It is comletely orthogonal to the migration changes in this series. So both preparatory and follow up are ok.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 4cb455355f6d..50d73e68b79e 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -8685,12 +8685,6 @@ static bool pfn_range_valid_contig(struct zone *z, unsigned long start_pfn, if (PageReserved(page)) return false; - - if (page_count(page) > 0) - return false; - - if (PageHuge(page)) - return false; } return true; }
pfn_range_valid_contig() bails out when it finds an in-use page or a hugetlb page, among other things. We can drop the in-use page check since __alloc_contig_pages can migrate away those pages, and the hugetlb page check can go too since isolate_migratepages_range is now capable of dealing with hugetlb pages. Either way, those checks are racy so let the end function handle it when the time comes. Signed-off-by: Oscar Salvador <osalvador@suse.de> Suggested-by: David Hildenbrand <david@redhat.com> --- mm/page_alloc.c | 6 ------ 1 file changed, 6 deletions(-)