[PATCH 1/2] mm/migrate_device: further convert migrate_device_unmap() to folios

[PATCH 1/2] mm/migrate_device: further convert migrate_device_unmap() to folios

[Sidhartha Kumar]

migrate_device_unmap() already has a folio, we can use the folio
versions of is_zone_device_page() and putback_lru_page.

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

diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index b6c27c76e1a0b..9152a329b0a68 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -377,33 +377,33 @@ static unsigned long migrate_device_unmap(unsigned long *src_pfns,
 			continue;
 		}
 
+		folio = page_folio(page);
 		/* ZONE_DEVICE pages are not on LRU */
-		if (!is_zone_device_page(page)) {
-			if (!PageLRU(page) && allow_drain) {
+		if (!folio_is_zone_device(folio)) {
+			if (!folio_test_lru(folio) && allow_drain) {
 				/* Drain CPU's lru cache */
 				lru_add_drain_all();
 				allow_drain = false;
 			}
 
-			if (!isolate_lru_page(page)) {
+			if (!folio_isolate_lru(folio)) {
 				src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
 				restore++;
 				continue;
 			}
 
 			/* Drop the reference we took in collect */
-			put_page(page);
+			folio_put(folio);
 		}
 
-		folio = page_folio(page);
 		if (folio_mapped(folio))
 			try_to_migrate(folio, 0);
 
-		if (page_mapped(page) ||
+		if (folio_mapped(folio) ||
 		    !migrate_vma_check_page(page, fault_page)) {
-			if (!is_zone_device_page(page)) {
-				get_page(page);
-				putback_lru_page(page);
+			if (!folio_is_zone_device(folio)) {
+				folio_get(folio);
+				folio_putback_lru(folio);
 			}
 
 			src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
-- 
2.42.0

[PATCH 2/2] mm/migrate_device: further convert migrate_device_finalize() to folios

[Sidhartha Kumar]

Use folio api functions from the already defined src and dst folio
variables.

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

diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index 9152a329b0a68..a48d5cdb28553 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -843,17 +843,17 @@ void migrate_device_finalize(unsigned long *src_pfns,
 		remove_migration_ptes(src, dst, false);
 		folio_unlock(src);
 
-		if (is_zone_device_page(page))
-			put_page(page);
+		if (folio_is_zone_device(src))
+			folio_put(src);
 		else
-			putback_lru_page(page);
+			folio_putback_lru(src);
 
 		if (newpage != page) {
-			unlock_page(newpage);
-			if (is_zone_device_page(newpage))
-				put_page(newpage);
+			folio_unlock(dst);
+			if (folio_is_zone_device(dst))
+				folio_put(dst);
 			else
-				putback_lru_page(newpage);
+				folio_putback_lru(dst);
 		}
 	}
 }
-- 
2.42.0

Re: [PATCH 1/2] mm/migrate_device: further convert migrate_device_unmap() to folios

[Alistair Popple]

Sidhartha Kumar <sidhartha.kumar@oracle.com> writes:

> migrate_device_unmap() already has a folio, we can use the folio
> versions of is_zone_device_page() and putback_lru_page.
>
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  mm/migrate_device.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index b6c27c76e1a0b..9152a329b0a68 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -377,33 +377,33 @@ static unsigned long migrate_device_unmap(unsigned long *src_pfns,
>  			continue;
>  		}
>  
> +		folio = page_folio(page);

Instead of open coding the migrate pfn to folio conversion I think we
should define a migrate_pfn_to_folio() and get rid of the intermediate
local variable. This would also allow a minor clean up to the final for
loop in migrate_device_unmap().

>  		/* ZONE_DEVICE pages are not on LRU */
> -		if (!is_zone_device_page(page)) {
> -			if (!PageLRU(page) && allow_drain) {
> +		if (!folio_is_zone_device(folio)) {
> +			if (!folio_test_lru(folio) && allow_drain) {
>  				/* Drain CPU's lru cache */
>  				lru_add_drain_all();
>  				allow_drain = false;
>  			}
>  
> -			if (!isolate_lru_page(page)) {
> +			if (!folio_isolate_lru(folio)) {
>  				src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;
>  				restore++;
>  				continue;
>  			}
>  
>  			/* Drop the reference we took in collect */
> -			put_page(page);
> +			folio_put(folio);
>  		}
>  
> -		folio = page_folio(page);
>  		if (folio_mapped(folio))
>  			try_to_migrate(folio, 0);
>  
> -		if (page_mapped(page) ||
> +		if (folio_mapped(folio) ||
>  		    !migrate_vma_check_page(page, fault_page)) {
> -			if (!is_zone_device_page(page)) {
> -				get_page(page);
> -				putback_lru_page(page);
> +			if (!folio_is_zone_device(folio)) {
> +				folio_get(folio);
> +				folio_putback_lru(folio);
>  			}
>  
>  			src_pfns[i] &= ~MIGRATE_PFN_MIGRATE;

Re: [PATCH 2/2] mm/migrate_device: further convert migrate_device_finalize() to folios

[Alistair Popple]

Sidhartha Kumar <sidhartha.kumar@oracle.com> writes:

> Use folio api functions from the already defined src and dst folio
> variables.
>
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>  mm/migrate_device.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 9152a329b0a68..a48d5cdb28553 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -843,17 +843,17 @@ void migrate_device_finalize(unsigned long *src_pfns,
>  		remove_migration_ptes(src, dst, false);
>  		folio_unlock(src);
>  
> -		if (is_zone_device_page(page))
> -			put_page(page);
> +		if (folio_is_zone_device(src))
> +			folio_put(src);
>  		else
> -			putback_lru_page(page);
> +			folio_putback_lru(src);
>  
>  		if (newpage != page) {
> -			unlock_page(newpage);
> -			if (is_zone_device_page(newpage))
> -				put_page(newpage);

Defining migrate_pfn_to_folio() would also allow the removal of the
newpage and page variables entirely which I think would make this
clearer.

As an aside is there any motivation for making these changes other than
as a general cleanup? I ask only because I have been looking at allowing
device pages with order > 0 so have some of these clean-ups in a local
tree as they're a pre-requisite for that.

 - Alistair

> +			folio_unlock(dst);
> +			if (folio_is_zone_device(dst))
> +				folio_put(dst);
>  			else
> -				putback_lru_page(newpage);
> +				folio_putback_lru(dst);
>  		}
>  	}
>  }

Re: [PATCH 2/2] mm/migrate_device: further convert migrate_device_finalize() to folios

[Sidhartha Kumar]

On 2/14/24 2:45 PM, Alistair Popple wrote:
> 
> Sidhartha Kumar <sidhartha.kumar@oracle.com> writes:
> 
>> Use folio api functions from the already defined src and dst folio
>> variables.
>>
>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> ---
>>   mm/migrate_device.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>> index 9152a329b0a68..a48d5cdb28553 100644
>> --- a/mm/migrate_device.c
>> +++ b/mm/migrate_device.c
>> @@ -843,17 +843,17 @@ void migrate_device_finalize(unsigned long *src_pfns,
>>   		remove_migration_ptes(src, dst, false);
>>   		folio_unlock(src);
>>   
>> -		if (is_zone_device_page(page))
>> -			put_page(page);
>> +		if (folio_is_zone_device(src))
>> +			folio_put(src);
>>   		else
>> -			putback_lru_page(page);
>> +			folio_putback_lru(src);
>>   
>>   		if (newpage != page) {
>> -			unlock_page(newpage);
>> -			if (is_zone_device_page(newpage))
>> -				put_page(newpage);
> 
> Defining migrate_pfn_to_folio() would also allow the removal of the
> newpage and page variables entirely which I think would make this
> clearer.
> 
> As an aside is there any motivation for making these changes other than
> as a general cleanup? I ask only because I have been looking at allowing
> device pages with order > 0 so have some of these clean-ups in a local
> tree as they're a pre-requisite for that.
> 
>   - Alistair
> 

Hello,

The motivation is just general cleanup. In folio-compat.c I saw that 
putback_lru_page() does not have much users left so I could convert them and 
then just get rid of putback_lru_page(). Should I still continue with a v2 that 
will include defining a migrate_pfn_to_folio() or wait for your clean-ups?

Thanks,
Sid

>> +			folio_unlock(dst);
>> +			if (folio_is_zone_device(dst))
>> +				folio_put(dst);
>>   			else
>> -				putback_lru_page(newpage);
>> +				folio_putback_lru(dst);
>>   		}
>>   	}
>>   }
> 

Re: [PATCH 2/2] mm/migrate_device: further convert migrate_device_finalize() to folios

[Alistair Popple]

Sidhartha Kumar <sidhartha.kumar@oracle.com> writes:

> On 2/14/24 2:45 PM, Alistair Popple wrote:
>> Sidhartha Kumar <sidhartha.kumar@oracle.com> writes:
>> 
>>> Use folio api functions from the already defined src and dst folio
>>> variables.
>>>
>>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>>> ---
>>>   mm/migrate_device.c | 14 +++++++-------
>>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>>> index 9152a329b0a68..a48d5cdb28553 100644
>>> --- a/mm/migrate_device.c
>>> +++ b/mm/migrate_device.c
>>> @@ -843,17 +843,17 @@ void migrate_device_finalize(unsigned long *src_pfns,
>>>   		remove_migration_ptes(src, dst, false);
>>>   		folio_unlock(src);
>>>   -		if (is_zone_device_page(page))
>>> -			put_page(page);
>>> +		if (folio_is_zone_device(src))
>>> +			folio_put(src);
>>>   		else
>>> -			putback_lru_page(page);
>>> +			folio_putback_lru(src);
>>>     		if (newpage != page) {
>>> -			unlock_page(newpage);
>>> -			if (is_zone_device_page(newpage))
>>> -				put_page(newpage);
>> Defining migrate_pfn_to_folio() would also allow the removal of the
>> newpage and page variables entirely which I think would make this
>> clearer.
>> As an aside is there any motivation for making these changes other
>> than
>> as a general cleanup? I ask only because I have been looking at allowing
>> device pages with order > 0 so have some of these clean-ups in a local
>> tree as they're a pre-requisite for that.
>>   - Alistair
>> 
>
> Hello,
>
> The motivation is just general cleanup. In folio-compat.c I saw that
> putback_lru_page() does not have much users left so I could convert
> them and then just get rid of putback_lru_page(). Should I still
> continue with a v2 that will include defining a migrate_pfn_to_folio()
> or wait for your clean-ups?

No, don't wait for mine. Please continue with defining
migrate_pfn_to_folio() for v2, I was just curious in case you had some
larger goal in mind.

Thanks.

 - Alistair

> Thanks,
> Sid
>
>>> +			folio_unlock(dst);
>>> +			if (folio_is_zone_device(dst))
>>> +				folio_put(dst);
>>>   			else
>>> -				putback_lru_page(newpage);
>>> +				folio_putback_lru(dst);
>>>   		}
>>>   	}
>>>   }
>> 

Re: [PATCH 1/2] mm/migrate_device: further convert migrate_device_unmap() to folios

[Matthew Wilcox]

On Thu, Feb 15, 2024 at 09:38:42AM +1100, Alistair Popple wrote:
> > +++ b/mm/migrate_device.c
> > @@ -377,33 +377,33 @@ static unsigned long migrate_device_unmap(unsigned long *src_pfns,
> >  			continue;
> >  		}
> >  
> > +		folio = page_folio(page);
> 
> Instead of open coding the migrate pfn to folio conversion I think we
> should define a migrate_pfn_to_folio() and get rid of the intermediate
> local variable. This would also allow a minor clean up to the final for
> loop in migrate_device_unmap().

I think we should stop passing pfns into migrate_device_unmap().
Passing an array of folios would make more sense to every function
involved, afaict.  Maybe I overlooked something ...

Also, have you had any thoughts on whether device memory is a type of
folio like anon/file memory, or is it its own type?

Re: [PATCH 1/2] mm/migrate_device: further convert migrate_device_unmap() to folios

[Alistair Popple]

Matthew Wilcox <willy@infradead.org> writes:

> On Thu, Feb 15, 2024 at 09:38:42AM +1100, Alistair Popple wrote:
>> > +++ b/mm/migrate_device.c
>> > @@ -377,33 +377,33 @@ static unsigned long migrate_device_unmap(unsigned long *src_pfns,
>> >  			continue;
>> >  		}
>> >  
>> > +		folio = page_folio(page);
>> 
>> Instead of open coding the migrate pfn to folio conversion I think we
>> should define a migrate_pfn_to_folio() and get rid of the intermediate
>> local variable. This would also allow a minor clean up to the final for
>> loop in migrate_device_unmap().
>
> I think we should stop passing pfns into migrate_device_unmap().
> Passing an array of folios would make more sense to every function
> involved, afaict.  Maybe I overlooked something ...

Note these are migration pfns. The main reason we do this is we need to
track and possibly modify some per-pfn state around between all these
functions during the migration process.

> Also, have you had any thoughts on whether device memory is a type of
> folio like anon/file memory, or is it its own type?

I don't quite follow what the precise distinction there is but I think
of them as normal pages/folios like anon/file memory folios because we
rely on the same kernel paths and rules to manage them (ie. they get
refcounted the same as normal pages, CoWed, etc.). Currently we only
allow these to be mapped into private/anon VMAs but I have an
experiemental series to allow them to be mapped into shared or
filebacked VMAs which basically involves putting them into the
page-cache.

Most drivers also have a 1:1 mapping of struct page to a physical page
of device memory and due to all the folio work it's fairly easy to
extend this to support higher order folios. I will try and post the
first half of my changes that convert all the page based handling to
folios. I got caught up trying figuring out a sane API for
splitting/merging during migration but maybe I should just post the
folio conversion as a simpler first step.