diff mbox series

[1/2] mm/memory_hotplug: remove head page reference in do_migrate_range

Message ID 20230123202347.317065-1-sidhartha.kumar@oracle.com
State New
Headers
Series [1/2] mm/memory_hotplug: remove head page reference in do_migrate_range |

Commit Message

Sidhartha Kumar Jan. 23, 2023, 8:23 p.m. UTC 
  The head page variable is not needed as we can use folio equivalent
functions.

Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 mm/memory_hotplug.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Matthew Wilcox Jan. 23, 2023, 8:37 p.m. UTC | #1 
On Mon, Jan 23, 2023 at 12:23:46PM -0800, Sidhartha Kumar wrote:
> @@ -1637,14 +1637,13 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>  			continue;
>  		page = pfn_to_page(pfn);
>  		folio = page_folio(page);
> -		head = &folio->page;
>  
> -		if (PageHuge(page)) {
> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
> +		if (folio_test_hugetlb(folio)) {
> +			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>  			isolate_hugetlb(folio, &source);
>  			continue;
> -		} else if (PageTransHuge(page))
> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
> +		} else if (folio_test_transhuge(folio))
> +			pfn = folio_pfn(folio) + thp_nr_pages(page) - 1;

I'm pretty sure those two lines should be...

		} else if (folio_test_large(folio))
			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;

But, erm ... we're doing this before we have a refcount on the page,
right?  So this is unsafe because the page might change which folio
it is in.  And the folio we found earlier might become a tail page
of a different folio.  (As the comment below explains, HWPoison pages
won't, so it's not unsafe for them).

Also, thp_nr_pages(page) is going to return 1 for tail pages.  So this
is a noop, unless page is a head page.

It's all a bit confusing, and being memory-hotplug, it's not well
tested.  More thought needed.
Sidhartha Kumar Jan. 23, 2023, 9:08 p.m. UTC | #2 
On 1/23/23 12:37 PM, Matthew Wilcox wrote:
> On Mon, Jan 23, 2023 at 12:23:46PM -0800, Sidhartha Kumar wrote:
>> @@ -1637,14 +1637,13 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   			continue;
>>   		page = pfn_to_page(pfn);
>>   		folio = page_folio(page);
>> -		head = &folio->page;
>>   
>> -		if (PageHuge(page)) {
>> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
>> +		if (folio_test_hugetlb(folio)) {
>> +			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>   			isolate_hugetlb(folio, &source);
>>   			continue;
>> -		} else if (PageTransHuge(page))
>> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>> +		} else if (folio_test_transhuge(folio))
>> +			pfn = folio_pfn(folio) + thp_nr_pages(page) - 1;
> 
> I'm pretty sure those two lines should be...
> 
> 		} else if (folio_test_large(folio) > 			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> 
> But, erm ... we're doing this before we have a refcount on the page,
> right?  So this is unsafe because the page might change which folio
> it is in.  And the folio we found earlier might become a tail page
> of a different folio.  (As the comment below explains, HWPoison pages
> won't, so it's not unsafe for them).
> 

Thanks for the explanation of why this is unsafe. Would it be worth to 
put this code block inside the

		if (!get_page_unless_zero(page))
			continue;

		put_page(page);

block found lower? My motivation for this series is the HPageMigratable 
call in patch 2 is the last user of the huge page flag test macros so a 
conversion would allow for the removal of the macro. I thought I could 
also remove the head page references found in this function, but if that 
would cause too much churn in a complicated sub-system it can be dropped.

Thanks,
Sidhartha Kumar
> Also, thp_nr_pages(page) is going to return 1 for tail pages.  So this
> is a noop, unless page is a head page.
> 
> It's all a bit confusing, and being memory-hotplug, it's not well
> tested.  More thought needed.
Matthew Wilcox Jan. 24, 2023, 2:32 a.m. UTC | #3 
On Mon, Jan 23, 2023 at 01:08:49PM -0800, Sidhartha Kumar wrote:
> On 1/23/23 12:37 PM, Matthew Wilcox wrote:
> > On Mon, Jan 23, 2023 at 12:23:46PM -0800, Sidhartha Kumar wrote:
> > > @@ -1637,14 +1637,13 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> > >   			continue;
> > >   		page = pfn_to_page(pfn);
> > >   		folio = page_folio(page);
> > > -		head = &folio->page;
> > > -		if (PageHuge(page)) {
> > > -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
> > > +		if (folio_test_hugetlb(folio)) {
> > > +			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> > >   			isolate_hugetlb(folio, &source);
> > >   			continue;
> > > -		} else if (PageTransHuge(page))
> > > -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
> > > +		} else if (folio_test_transhuge(folio))
> > > +			pfn = folio_pfn(folio) + thp_nr_pages(page) - 1;
> > 
> > I'm pretty sure those two lines should be...
> > 
> > 		} else if (folio_test_large(folio) > 			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> > 
> > But, erm ... we're doing this before we have a refcount on the page,
> > right?  So this is unsafe because the page might change which folio
> > it is in.  And the folio we found earlier might become a tail page
> > of a different folio.  (As the comment below explains, HWPoison pages
> > won't, so it's not unsafe for them).
> > 
> 
> Thanks for the explanation of why this is unsafe. Would it be worth to put
> this code block inside the
> 
> 		if (!get_page_unless_zero(page))
> 			continue;
> 
> 		put_page(page);
> 
> block found lower? My motivation for this series is the HPageMigratable call
> in patch 2 is the last user of the huge page flag test macros so a
> conversion would allow for the removal of the macro. I thought I could also
> remove the head page references found in this function, but if that would
> cause too much churn in a complicated sub-system it can be dropped.

I think we just have to be very careful when working without a page ref.

Now, specifically to the matter of converting HPageMigratable(), I think
that's fine.  Your folio_test_hugetlb_##flname macro does not have a
VM_BUG_ON_PGFLAGS(PageTail(page), page) in it, unlike folio_flags().
So it looks like even if your folio becomes a tail page in the middle
of scan_movable_pages(), you won't hit a BUG().

Now, should we go further?  Possibly.  But I'm more concerned that we
haven't really figured out which functions should be checking this.
Maybe we should drop the BUG entirely and rely more on the type system
(and people not casting) to prevent errors.

We could go a lot further with the type system and define a new type for
"might be a folio but we don't have a refcount on it".  But we don't
do a lot of work with unreferenced folios, so I'm not inclined to go to
all that effort.

Perhaps we want a folio_maybe_X() series of functions that don't warn
if the folio has morphed into not-a-folio.  I don't know.
David Hildenbrand Jan. 24, 2023, 10:17 a.m. UTC | #4 
On 23.01.23 21:37, Matthew Wilcox wrote:
> On Mon, Jan 23, 2023 at 12:23:46PM -0800, Sidhartha Kumar wrote:
>> @@ -1637,14 +1637,13 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>>   			continue;
>>   		page = pfn_to_page(pfn);
>>   		folio = page_folio(page);
>> -		head = &folio->page;
>>   
>> -		if (PageHuge(page)) {
>> -			pfn = page_to_pfn(head) + compound_nr(head) - 1;
>> +		if (folio_test_hugetlb(folio)) {
>> +			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>   			isolate_hugetlb(folio, &source);
>>   			continue;
>> -		} else if (PageTransHuge(page))
>> -			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>> +		} else if (folio_test_transhuge(folio))
>> +			pfn = folio_pfn(folio) + thp_nr_pages(page) - 1;
> 
> I'm pretty sure those two lines should be...
> 
> 		} else if (folio_test_large(folio))
> 			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
> 
> But, erm ... we're doing this before we have a refcount on the page,
> right?  So this is unsafe because the page might change which folio
> it is in.  And the folio we found earlier might become a tail page
> of a different folio.  (As the comment below explains, HWPoison pages
> won't, so it's not unsafe for them).
> 
> Also, thp_nr_pages(page) is going to return 1 for tail pages.  So this
> is a noop, unless page is a head page.
> 
> It's all a bit confusing, and being memory-hotplug, it's not well
> tested.  More thought needed.

Ehm, it is fairly well tested ;)

As memory offlining keeps retrying, temporarily making wrong assumptions 
about a folio is acceptable, as long as we don't run into BUGs.

It's certainly worth a big comment in a code, that this is all racy and 
that page migration code will stabilize.

Now, we could temporarily take a reference, but ... common migration 
code will try taking its own ref to stabilize the page and would be 
confused about yet another ref (-> migration will fail).

So we have to be careful about grabbing references on these pages, and 
how long we're going to hold them. Otherwise we'll break memory 
offlining completely :)
diff mbox series

Patch

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a1e8c3e9ab08..ad09189786b1 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1624,7 +1624,7 @@  static int
 do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 {
 	unsigned long pfn;
-	struct page *page, *head;
+	struct page *page;
 	int ret = 0;
 	LIST_HEAD(source);
 	static DEFINE_RATELIMIT_STATE(migrate_rs, DEFAULT_RATELIMIT_INTERVAL,
@@ -1637,14 +1637,13 @@  do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 			continue;
 		page = pfn_to_page(pfn);
 		folio = page_folio(page);
-		head = &folio->page;
 
-		if (PageHuge(page)) {
-			pfn = page_to_pfn(head) + compound_nr(head) - 1;
+		if (folio_test_hugetlb(folio)) {
+			pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
 			isolate_hugetlb(folio, &source);
 			continue;
-		} else if (PageTransHuge(page))
-			pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
+		} else if (folio_test_transhuge(folio))
+			pfn = folio_pfn(folio) + thp_nr_pages(page) - 1;
 
 		/*
 		 * HWPoison pages have elevated reference counts so the migration would