Improve migration by backing off earlier

[PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 6 months ago

As already being done in __migrate_folio(), wherein we backoff if the
folio refcount is wrong, make this check during the unmapping phase, upon
the failure of which, the original state of the PTEs will be restored and
the folio lock will be dropped via migrate_folio_undo_src(), any racing
thread will make progress and migration will be retried.

Signed-off-by: Dev Jain <dev.jain@arm.com>
---
 mm/migrate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/mm/migrate.c b/mm/migrate.c
index e7296c0fb5d5..477acf996951 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
 	}
 
 	if (!folio_mapped(src)) {
+		/*
+		 * Someone may have changed the refcount and maybe sleeping
+		 * on the folio lock. In case of refcount mismatch, bail out,
+		 * let the system make progress and retry.
+		 */
+		struct address_space *mapping = folio_mapping(src);
+
+		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
+			goto out;
 		__migrate_folio_record(dst, old_page_state, anon_vma);
 		return MIGRATEPAGE_UNMAP;
 	}
-- 
2.30.2

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Huang, Ying 1 year, 5 months ago

Hi, Dev,

Dev Jain <dev.jain@arm.com> writes:

> As already being done in __migrate_folio(), wherein we backoff if the
> folio refcount is wrong, make this check during the unmapping phase, upon
> the failure of which, the original state of the PTEs will be restored and
> the folio lock will be dropped via migrate_folio_undo_src(), any racing
> thread will make progress and migration will be retried.
>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
>  mm/migrate.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index e7296c0fb5d5..477acf996951 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>  	}
>  
>  	if (!folio_mapped(src)) {
> +		/*
> +		 * Someone may have changed the refcount and maybe sleeping
> +		 * on the folio lock. In case of refcount mismatch, bail out,
> +		 * let the system make progress and retry.
> +		 */
> +		struct address_space *mapping = folio_mapping(src);
> +
> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
> +			goto out;
>  		__migrate_folio_record(dst, old_page_state, anon_vma);
>  		return MIGRATEPAGE_UNMAP;
>  	}

Do you have some test results for this?  For example, after applying the
patch, the migration success rate increased XX%, etc.

My understanding for this issue is that the migration success rate can
increase if we undo all changes before retrying.  This is the current
behavior for sync migration, but not for async migration.  If so, we can
use migrate_pages_sync() for async migration too to increase success
rate?  Of course, we need to change the function name and comments.

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/12/24 11:04, Huang, Ying wrote:
> Hi, Dev,
>
> Dev Jain <dev.jain@arm.com> writes:
>
>> As already being done in __migrate_folio(), wherein we backoff if the
>> folio refcount is wrong, make this check during the unmapping phase, upon
>> the failure of which, the original state of the PTEs will be restored and
>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>> thread will make progress and migration will be retried.
>>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> ---
>>   mm/migrate.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index e7296c0fb5d5..477acf996951 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>   	}
>>   
>>   	if (!folio_mapped(src)) {
>> +		/*
>> +		 * Someone may have changed the refcount and maybe sleeping
>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>> +		 * let the system make progress and retry.
>> +		 */
>> +		struct address_space *mapping = folio_mapping(src);
>> +
>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>> +			goto out;
>>   		__migrate_folio_record(dst, old_page_state, anon_vma);
>>   		return MIGRATEPAGE_UNMAP;
>>   	}
> Do you have some test results for this?  For example, after applying the
> patch, the migration success rate increased XX%, etc.

Noting that the migration selftest is operating on a single page,
before the patch, the test fails on shared-anon mappings on an
average of 10 iterations of move_pages(), and after applying the
patch it fails on average of 100 iterations, which makes sense
because the unmapping() will get retried 3 + 7 = 10 times.

>
> My understanding for this issue is that the migration success rate can
> increase if we undo all changes before retrying.  This is the current
> behavior for sync migration, but not for async migration.  If so, we can
> use migrate_pages_sync() for async migration too to increase success
> rate?  Of course, we need to change the function name and comments.
>
> --
> Best Regards,
> Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Huang, Ying 1 year, 5 months ago

Dev Jain <dev.jain@arm.com> writes:

> On 8/12/24 11:04, Huang, Ying wrote:
>> Hi, Dev,
>>
>> Dev Jain <dev.jain@arm.com> writes:
>>
>>> As already being done in __migrate_folio(), wherein we backoff if the
>>> folio refcount is wrong, make this check during the unmapping phase, upon
>>> the failure of which, the original state of the PTEs will be restored and
>>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>>> thread will make progress and migration will be retried.
>>>
>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>> ---
>>>   mm/migrate.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index e7296c0fb5d5..477acf996951 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>   	}
>>>     	if (!folio_mapped(src)) {
>>> +		/*
>>> +		 * Someone may have changed the refcount and maybe sleeping
>>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>>> +		 * let the system make progress and retry.
>>> +		 */
>>> +		struct address_space *mapping = folio_mapping(src);
>>> +
>>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>>> +			goto out;
>>>   		__migrate_folio_record(dst, old_page_state, anon_vma);
>>>   		return MIGRATEPAGE_UNMAP;
>>>   	}
>> Do you have some test results for this?  For example, after applying the
>> patch, the migration success rate increased XX%, etc.
>
> Noting that the migration selftest is operating on a single page,
> before the patch, the test fails on shared-anon mappings on an
> average of 10 iterations of move_pages(), and after applying the
> patch it fails on average of 100 iterations, which makes sense
> because the unmapping() will get retried 3 + 7 = 10 times.

Thanks!  What is the test results for

https://lore.kernel.org/all/20240801081657.1386743-1-dev.jain@arm.com/

?

>>
>> My understanding for this issue is that the migration success rate can
>> increase if we undo all changes before retrying.  This is the current
>> behavior for sync migration, but not for async migration.  If so, we can
>> use migrate_pages_sync() for async migration too to increase success
>> rate?  Of course, we need to change the function name and comments.

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/12/24 11:50, Huang, Ying wrote:
> Dev Jain <dev.jain@arm.com> writes:
>
>> On 8/12/24 11:04, Huang, Ying wrote:
>>> Hi, Dev,
>>>
>>> Dev Jain <dev.jain@arm.com> writes:
>>>
>>>> As already being done in __migrate_folio(), wherein we backoff if the
>>>> folio refcount is wrong, make this check during the unmapping phase, upon
>>>> the failure of which, the original state of the PTEs will be restored and
>>>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>>>> thread will make progress and migration will be retried.
>>>>
>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>> ---
>>>>    mm/migrate.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index e7296c0fb5d5..477acf996951 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>>    	}
>>>>      	if (!folio_mapped(src)) {
>>>> +		/*
>>>> +		 * Someone may have changed the refcount and maybe sleeping
>>>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>>>> +		 * let the system make progress and retry.
>>>> +		 */
>>>> +		struct address_space *mapping = folio_mapping(src);
>>>> +
>>>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>>>> +			goto out;
>>>>    		__migrate_folio_record(dst, old_page_state, anon_vma);
>>>>    		return MIGRATEPAGE_UNMAP;
>>>>    	}
>>> Do you have some test results for this?  For example, after applying the
>>> patch, the migration success rate increased XX%, etc.
>> Noting that the migration selftest is operating on a single page,
>> before the patch, the test fails on shared-anon mappings on an
>> average of 10 iterations of move_pages(), and after applying the
>> patch it fails on average of 100 iterations, which makes sense
>> because the unmapping() will get retried 3 + 7 = 10 times.
> Thanks!  What is the test results for
>
> https://lore.kernel.org/all/20240801081657.1386743-1-dev.jain@arm.com/
>
> ?

That solves the problem completely, makes the test pass. Although
that still may not be a good solution since it solves the problem
for this particular case (reader thread faulting and raising refcount).
As David notes, a concurrent read()/write() should also create this
refcount race problem.


>
>>> My understanding for this issue is that the migration success rate can
>>> increase if we undo all changes before retrying.  This is the current
>>> behavior for sync migration, but not for async migration.  If so, we can
>>> use migrate_pages_sync() for async migration too to increase success
>>> rate?  Of course, we need to change the function name and comments.
> --
> Best Regards,
> Huang, Ying
>

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/12/24 11:04, Huang, Ying wrote:
> Hi, Dev,
>
> Dev Jain <dev.jain@arm.com> writes:
>
>> As already being done in __migrate_folio(), wherein we backoff if the
>> folio refcount is wrong, make this check during the unmapping phase, upon
>> the failure of which, the original state of the PTEs will be restored and
>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>> thread will make progress and migration will be retried.
>>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> ---
>>   mm/migrate.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index e7296c0fb5d5..477acf996951 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>   	}
>>   
>>   	if (!folio_mapped(src)) {
>> +		/*
>> +		 * Someone may have changed the refcount and maybe sleeping
>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>> +		 * let the system make progress and retry.
>> +		 */
>> +		struct address_space *mapping = folio_mapping(src);
>> +
>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>> +			goto out;
>>   		__migrate_folio_record(dst, old_page_state, anon_vma);
>>   		return MIGRATEPAGE_UNMAP;
>>   	}
> Do you have some test results for this?  For example, after applying the
> patch, the migration success rate increased XX%, etc.

I'll get back to you on this.

>
> My understanding for this issue is that the migration success rate can
> increase if we undo all changes before retrying.  This is the current
> behavior for sync migration, but not for async migration.  If so, we can
> use migrate_pages_sync() for async migration too to increase success
> rate?  Of course, we need to change the function name and comments.


As per my understanding, this is not the current behaviour for sync
migration. After successful unmapping, we fail in migrate_folio_move()
with -EAGAIN, we do not call undo src+dst (rendering the loop around
migrate_folio_move() futile), we do not push the failed folio onto the
ret_folios list, therefore, in _sync(), _batch() is never tried again.

>
> --
> Best Regards,
> Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Huang, Ying 1 year, 5 months ago

Dev Jain <dev.jain@arm.com> writes:

> On 8/12/24 11:04, Huang, Ying wrote:
>> Hi, Dev,
>>
>> Dev Jain <dev.jain@arm.com> writes:
>>
>>> As already being done in __migrate_folio(), wherein we backoff if the
>>> folio refcount is wrong, make this check during the unmapping phase, upon
>>> the failure of which, the original state of the PTEs will be restored and
>>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>>> thread will make progress and migration will be retried.
>>>
>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>> ---
>>>   mm/migrate.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index e7296c0fb5d5..477acf996951 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>   	}
>>>     	if (!folio_mapped(src)) {
>>> +		/*
>>> +		 * Someone may have changed the refcount and maybe sleeping
>>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>>> +		 * let the system make progress and retry.
>>> +		 */
>>> +		struct address_space *mapping = folio_mapping(src);
>>> +
>>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>>> +			goto out;
>>>   		__migrate_folio_record(dst, old_page_state, anon_vma);
>>>   		return MIGRATEPAGE_UNMAP;
>>>   	}
>> Do you have some test results for this?  For example, after applying the
>> patch, the migration success rate increased XX%, etc.
>
> I'll get back to you on this.
>
>>
>> My understanding for this issue is that the migration success rate can
>> increase if we undo all changes before retrying.  This is the current
>> behavior for sync migration, but not for async migration.  If so, we can
>> use migrate_pages_sync() for async migration too to increase success
>> rate?  Of course, we need to change the function name and comments.
>
>
> As per my understanding, this is not the current behaviour for sync
> migration. After successful unmapping, we fail in migrate_folio_move()
> with -EAGAIN, we do not call undo src+dst (rendering the loop around
> migrate_folio_move() futile), we do not push the failed folio onto the
> ret_folios list, therefore, in _sync(), _batch() is never tried again.

In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
called first, if failed, the folio will be restored to the original
state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called again.
So, we unlock once.  If it's necessary, we can unlock more times via
another level of loop.

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/12/24 11:45, Huang, Ying wrote:
> Dev Jain <dev.jain@arm.com> writes:
>
>> On 8/12/24 11:04, Huang, Ying wrote:
>>> Hi, Dev,
>>>
>>> Dev Jain <dev.jain@arm.com> writes:
>>>
>>>> As already being done in __migrate_folio(), wherein we backoff if the
>>>> folio refcount is wrong, make this check during the unmapping phase, upon
>>>> the failure of which, the original state of the PTEs will be restored and
>>>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>>>> thread will make progress and migration will be retried.
>>>>
>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>> ---
>>>>    mm/migrate.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index e7296c0fb5d5..477acf996951 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>>    	}
>>>>      	if (!folio_mapped(src)) {
>>>> +		/*
>>>> +		 * Someone may have changed the refcount and maybe sleeping
>>>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>>>> +		 * let the system make progress and retry.
>>>> +		 */
>>>> +		struct address_space *mapping = folio_mapping(src);
>>>> +
>>>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>>>> +			goto out;
>>>>    		__migrate_folio_record(dst, old_page_state, anon_vma);
>>>>    		return MIGRATEPAGE_UNMAP;
>>>>    	}
>>> Do you have some test results for this?  For example, after applying the
>>> patch, the migration success rate increased XX%, etc.
>> I'll get back to you on this.
>>
>>> My understanding for this issue is that the migration success rate can
>>> increase if we undo all changes before retrying.  This is the current
>>> behavior for sync migration, but not for async migration.  If so, we can
>>> use migrate_pages_sync() for async migration too to increase success
>>> rate?  Of course, we need to change the function name and comments.
>>
>> As per my understanding, this is not the current behaviour for sync
>> migration. After successful unmapping, we fail in migrate_folio_move()
>> with -EAGAIN, we do not call undo src+dst (rendering the loop around
>> migrate_folio_move() futile), we do not push the failed folio onto the
>> ret_folios list, therefore, in _sync(), _batch() is never tried again.
> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
> called first, if failed, the folio will be restored to the original
> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called again.
> So, we unlock once.  If it's necessary, we can unlock more times via
> another level of loop.

Yes, that's my point. We need to undo src+dst and retry. We will have
to decide where we want this retrying to be; do we want to change the
return value, end up in the while loop wrapped around _sync(), and retry
there by adding another level of loop, or do we want to make use of the
existing retry loops, one of which is wrapped around _unmap(); the latter
is my approach. The utility I see for the former approach is that, in case
of a large number of page migrations (which should usually be the case),
we are giving more time for the folio to get retried. The latter does not
give much time and discards the folio if it did not succeed under 7 times.

>
> --
> Best Regards,
> Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Huang, Ying 1 year, 5 months ago

Dev Jain <dev.jain@arm.com> writes:

> On 8/12/24 11:45, Huang, Ying wrote:
>> Dev Jain <dev.jain@arm.com> writes:
>>
>>> On 8/12/24 11:04, Huang, Ying wrote:
>>>> Hi, Dev,
>>>>
>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>
>>>>> As already being done in __migrate_folio(), wherein we backoff if the
>>>>> folio refcount is wrong, make this check during the unmapping phase, upon
>>>>> the failure of which, the original state of the PTEs will be restored and
>>>>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>>>>> thread will make progress and migration will be retried.
>>>>>
>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>> ---
>>>>>    mm/migrate.c | 9 +++++++++
>>>>>    1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>> --- a/mm/migrate.c
>>>>> +++ b/mm/migrate.c
>>>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>    	}
>>>>>      	if (!folio_mapped(src)) {
>>>>> +		/*
>>>>> +		 * Someone may have changed the refcount and maybe sleeping
>>>>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>>>>> +		 * let the system make progress and retry.
>>>>> +		 */
>>>>> +		struct address_space *mapping = folio_mapping(src);
>>>>> +
>>>>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>>>>> +			goto out;
>>>>>    		__migrate_folio_record(dst, old_page_state, anon_vma);
>>>>>    		return MIGRATEPAGE_UNMAP;
>>>>>    	}
>>>> Do you have some test results for this?  For example, after applying the
>>>> patch, the migration success rate increased XX%, etc.
>>> I'll get back to you on this.
>>>
>>>> My understanding for this issue is that the migration success rate can
>>>> increase if we undo all changes before retrying.  This is the current
>>>> behavior for sync migration, but not for async migration.  If so, we can
>>>> use migrate_pages_sync() for async migration too to increase success
>>>> rate?  Of course, we need to change the function name and comments.
>>>
>>> As per my understanding, this is not the current behaviour for sync
>>> migration. After successful unmapping, we fail in migrate_folio_move()
>>> with -EAGAIN, we do not call undo src+dst (rendering the loop around
>>> migrate_folio_move() futile), we do not push the failed folio onto the
>>> ret_folios list, therefore, in _sync(), _batch() is never tried again.
>> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
>> called first, if failed, the folio will be restored to the original
>> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called again.
>> So, we unlock once.  If it's necessary, we can unlock more times via
>> another level of loop.
>
> Yes, that's my point. We need to undo src+dst and retry.

For sync migration, we undo src+dst and retry now, but only once.  You
have shown that more retrying increases success rate.

> We will have
> to decide where we want this retrying to be; do we want to change the
> return value, end up in the while loop wrapped around _sync(), and retry
> there by adding another level of loop, or do we want to make use of the
> existing retry loops, one of which is wrapped around _unmap(); the latter
> is my approach. The utility I see for the former approach is that, in case
> of a large number of page migrations (which should usually be the case),
> we are giving more time for the folio to get retried. The latter does not
> give much time and discards the folio if it did not succeed under 7 times.

Because it's a race, I guess that most folios will be migrated
successfully in the first pass.

My concerns of your method are that it deal with just one case
specially.  While retrying after undoing all appears more general.

If it's really important to retry after undoing all, we can either
convert two retying loops of migrate_pages_batch() into one loop, or
remove retry loop in migrate_pages_batch() and retry in its caller
instead.

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/12/24 13:01, Huang, Ying wrote:
> Dev Jain <dev.jain@arm.com> writes:
>
>> On 8/12/24 11:45, Huang, Ying wrote:
>>> Dev Jain <dev.jain@arm.com> writes:
>>>
>>>> On 8/12/24 11:04, Huang, Ying wrote:
>>>>> Hi, Dev,
>>>>>
>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>
>>>>>> As already being done in __migrate_folio(), wherein we backoff if the
>>>>>> folio refcount is wrong, make this check during the unmapping phase, upon
>>>>>> the failure of which, the original state of the PTEs will be restored and
>>>>>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>>>>>> thread will make progress and migration will be retried.
>>>>>>
>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>> ---
>>>>>>     mm/migrate.c | 9 +++++++++
>>>>>>     1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>> --- a/mm/migrate.c
>>>>>> +++ b/mm/migrate.c
>>>>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>>     	}
>>>>>>       	if (!folio_mapped(src)) {
>>>>>> +		/*
>>>>>> +		 * Someone may have changed the refcount and maybe sleeping
>>>>>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>>>>>> +		 * let the system make progress and retry.
>>>>>> +		 */
>>>>>> +		struct address_space *mapping = folio_mapping(src);
>>>>>> +
>>>>>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>>>>>> +			goto out;
>>>>>>     		__migrate_folio_record(dst, old_page_state, anon_vma);
>>>>>>     		return MIGRATEPAGE_UNMAP;
>>>>>>     	}
>>>>> Do you have some test results for this?  For example, after applying the
>>>>> patch, the migration success rate increased XX%, etc.
>>>> I'll get back to you on this.
>>>>
>>>>> My understanding for this issue is that the migration success rate can
>>>>> increase if we undo all changes before retrying.  This is the current
>>>>> behavior for sync migration, but not for async migration.  If so, we can
>>>>> use migrate_pages_sync() for async migration too to increase success
>>>>> rate?  Of course, we need to change the function name and comments.
>>>> As per my understanding, this is not the current behaviour for sync
>>>> migration. After successful unmapping, we fail in migrate_folio_move()
>>>> with -EAGAIN, we do not call undo src+dst (rendering the loop around
>>>> migrate_folio_move() futile), we do not push the failed folio onto the
>>>> ret_folios list, therefore, in _sync(), _batch() is never tried again.
>>> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
>>> called first, if failed, the folio will be restored to the original
>>> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called again.
>>> So, we unlock once.  If it's necessary, we can unlock more times via
>>> another level of loop.
>> Yes, that's my point. We need to undo src+dst and retry.
> For sync migration, we undo src+dst and retry now, but only once.  You
> have shown that more retrying increases success rate.
>
>> We will have
>> to decide where we want this retrying to be; do we want to change the
>> return value, end up in the while loop wrapped around _sync(), and retry
>> there by adding another level of loop, or do we want to make use of the
>> existing retry loops, one of which is wrapped around _unmap(); the latter
>> is my approach. The utility I see for the former approach is that, in case
>> of a large number of page migrations (which should usually be the case),
>> we are giving more time for the folio to get retried. The latter does not
>> give much time and discards the folio if it did not succeed under 7 times.
> Because it's a race, I guess that most folios will be migrated
> successfully in the first pass.
>
> My concerns of your method are that it deal with just one case
> specially.  While retrying after undoing all appears more general.


Makes sense. Also, please ignore my "change the return value"
thing, I got confused between unmap_folios, ret_folios, etc.
Now I think I understood what the lists are doing :)

>
> If it's really important to retry after undoing all, we can either
> convert two retying loops of migrate_pages_batch() into one loop, or
> remove retry loop in migrate_pages_batch() and retry in its caller
> instead.

And if I implemented this correctly, the following makes the test
pass always:
https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXe



>
> --
> Best Regards,
> Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/12/24 17:38, Dev Jain wrote:
>
> On 8/12/24 13:01, Huang, Ying wrote:
>> Dev Jain <dev.jain@arm.com> writes:
>>
>>> On 8/12/24 11:45, Huang, Ying wrote:
>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>
>>>>> On 8/12/24 11:04, Huang, Ying wrote:
>>>>>> Hi, Dev,
>>>>>>
>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>
>>>>>>> As already being done in __migrate_folio(), wherein we backoff 
>>>>>>> if the
>>>>>>> folio refcount is wrong, make this check during the unmapping 
>>>>>>> phase, upon
>>>>>>> the failure of which, the original state of the PTEs will be 
>>>>>>> restored and
>>>>>>> the folio lock will be dropped via migrate_folio_undo_src(), any 
>>>>>>> racing
>>>>>>> thread will make progress and migration will be retried.
>>>>>>>
>>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>>> ---
>>>>>>>     mm/migrate.c | 9 +++++++++
>>>>>>>     1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>>> --- a/mm/migrate.c
>>>>>>> +++ b/mm/migrate.c
>>>>>>> @@ -1250,6 +1250,15 @@ static int 
>>>>>>> migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>>>         }
>>>>>>>           if (!folio_mapped(src)) {
>>>>>>> +        /*
>>>>>>> +         * Someone may have changed the refcount and maybe 
>>>>>>> sleeping
>>>>>>> +         * on the folio lock. In case of refcount mismatch, 
>>>>>>> bail out,
>>>>>>> +         * let the system make progress and retry.
>>>>>>> +         */
>>>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>>>> +
>>>>>>> +        if (folio_ref_count(src) != 
>>>>>>> folio_expected_refs(mapping, src))
>>>>>>> +            goto out;
>>>>>>>             __migrate_folio_record(dst, old_page_state, anon_vma);
>>>>>>>             return MIGRATEPAGE_UNMAP;
>>>>>>>         }
>>>>>> Do you have some test results for this?  For example, after 
>>>>>> applying the
>>>>>> patch, the migration success rate increased XX%, etc.
>>>>> I'll get back to you on this.
>>>>>
>>>>>> My understanding for this issue is that the migration success 
>>>>>> rate can
>>>>>> increase if we undo all changes before retrying.  This is the 
>>>>>> current
>>>>>> behavior for sync migration, but not for async migration.  If so, 
>>>>>> we can
>>>>>> use migrate_pages_sync() for async migration too to increase success
>>>>>> rate?  Of course, we need to change the function name and comments.
>>>>> As per my understanding, this is not the current behaviour for sync
>>>>> migration. After successful unmapping, we fail in 
>>>>> migrate_folio_move()
>>>>> with -EAGAIN, we do not call undo src+dst (rendering the loop around
>>>>> migrate_folio_move() futile), we do not push the failed folio onto 
>>>>> the
>>>>> ret_folios list, therefore, in _sync(), _batch() is never tried 
>>>>> again.
>>>> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
>>>> called first, if failed, the folio will be restored to the original
>>>> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called again.
>>>> So, we unlock once.  If it's necessary, we can unlock more times via
>>>> another level of loop.
>>> Yes, that's my point. We need to undo src+dst and retry.
>> For sync migration, we undo src+dst and retry now, but only once.  You
>> have shown that more retrying increases success rate.
>>
>>> We will have
>>> to decide where we want this retrying to be; do we want to change the
>>> return value, end up in the while loop wrapped around _sync(), and 
>>> retry
>>> there by adding another level of loop, or do we want to make use of the
>>> existing retry loops, one of which is wrapped around _unmap(); the 
>>> latter
>>> is my approach. The utility I see for the former approach is that, 
>>> in case
>>> of a large number of page migrations (which should usually be the 
>>> case),
>>> we are giving more time for the folio to get retried. The latter 
>>> does not
>>> give much time and discards the folio if it did not succeed under 7 
>>> times.
>> Because it's a race, I guess that most folios will be migrated
>> successfully in the first pass.
>>
>> My concerns of your method are that it deal with just one case
>> specially.  While retrying after undoing all appears more general.
>
>
> Makes sense. Also, please ignore my "change the return value"
> thing, I got confused between unmap_folios, ret_folios, etc.
> Now I think I understood what the lists are doing :)
>
>>
>> If it's really important to retry after undoing all, we can either
>> convert two retying loops of migrate_pages_batch() into one loop, or
>> remove retry loop in migrate_pages_batch() and retry in its caller
>> instead.
>
> And if I implemented this correctly, the following makes the test
> pass always:
> https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXe


Okay, I did mess up with the implementation, leading to a false
positive. Let me try again :)

>
>
>
>>
>> -- 
>> Best Regards,
>> Huang, Ying
>

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/13/24 10:30, Dev Jain wrote:
>
> On 8/12/24 17:38, Dev Jain wrote:
>>
>> On 8/12/24 13:01, Huang, Ying wrote:
>>> Dev Jain <dev.jain@arm.com> writes:
>>>
>>>> On 8/12/24 11:45, Huang, Ying wrote:
>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>
>>>>>> On 8/12/24 11:04, Huang, Ying wrote:
>>>>>>> Hi, Dev,
>>>>>>>
>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>
>>>>>>>> As already being done in __migrate_folio(), wherein we backoff 
>>>>>>>> if the
>>>>>>>> folio refcount is wrong, make this check during the unmapping 
>>>>>>>> phase, upon
>>>>>>>> the failure of which, the original state of the PTEs will be 
>>>>>>>> restored and
>>>>>>>> the folio lock will be dropped via migrate_folio_undo_src(), 
>>>>>>>> any racing
>>>>>>>> thread will make progress and migration will be retried.
>>>>>>>>
>>>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>>>> ---
>>>>>>>>     mm/migrate.c | 9 +++++++++
>>>>>>>>     1 file changed, 9 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>>>> --- a/mm/migrate.c
>>>>>>>> +++ b/mm/migrate.c
>>>>>>>> @@ -1250,6 +1250,15 @@ static int 
>>>>>>>> migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>>>>         }
>>>>>>>>           if (!folio_mapped(src)) {
>>>>>>>> +        /*
>>>>>>>> +         * Someone may have changed the refcount and maybe 
>>>>>>>> sleeping
>>>>>>>> +         * on the folio lock. In case of refcount mismatch, 
>>>>>>>> bail out,
>>>>>>>> +         * let the system make progress and retry.
>>>>>>>> +         */
>>>>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>>>>> +
>>>>>>>> +        if (folio_ref_count(src) != 
>>>>>>>> folio_expected_refs(mapping, src))
>>>>>>>> +            goto out;
>>>>>>>>             __migrate_folio_record(dst, old_page_state, anon_vma);
>>>>>>>>             return MIGRATEPAGE_UNMAP;
>>>>>>>>         }
>>>>>>> Do you have some test results for this?  For example, after 
>>>>>>> applying the
>>>>>>> patch, the migration success rate increased XX%, etc.
>>>>>> I'll get back to you on this.
>>>>>>
>>>>>>> My understanding for this issue is that the migration success 
>>>>>>> rate can
>>>>>>> increase if we undo all changes before retrying.  This is the 
>>>>>>> current
>>>>>>> behavior for sync migration, but not for async migration.  If 
>>>>>>> so, we can
>>>>>>> use migrate_pages_sync() for async migration too to increase 
>>>>>>> success
>>>>>>> rate?  Of course, we need to change the function name and comments.
>>>>>> As per my understanding, this is not the current behaviour for sync
>>>>>> migration. After successful unmapping, we fail in 
>>>>>> migrate_folio_move()
>>>>>> with -EAGAIN, we do not call undo src+dst (rendering the loop around
>>>>>> migrate_folio_move() futile), we do not push the failed folio 
>>>>>> onto the
>>>>>> ret_folios list, therefore, in _sync(), _batch() is never tried 
>>>>>> again.
>>>>> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
>>>>> called first, if failed, the folio will be restored to the original
>>>>> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called again.
>>>>> So, we unlock once.  If it's necessary, we can unlock more times via
>>>>> another level of loop.
>>>> Yes, that's my point. We need to undo src+dst and retry.
>>> For sync migration, we undo src+dst and retry now, but only once.  You
>>> have shown that more retrying increases success rate.
>>>
>>>> We will have
>>>> to decide where we want this retrying to be; do we want to change the
>>>> return value, end up in the while loop wrapped around _sync(), and 
>>>> retry
>>>> there by adding another level of loop, or do we want to make use of 
>>>> the
>>>> existing retry loops, one of which is wrapped around _unmap(); the 
>>>> latter
>>>> is my approach. The utility I see for the former approach is that, 
>>>> in case
>>>> of a large number of page migrations (which should usually be the 
>>>> case),
>>>> we are giving more time for the folio to get retried. The latter 
>>>> does not
>>>> give much time and discards the folio if it did not succeed under 7 
>>>> times.
>>> Because it's a race, I guess that most folios will be migrated
>>> successfully in the first pass.
>>>
>>> My concerns of your method are that it deal with just one case
>>> specially.  While retrying after undoing all appears more general.
>>
>>
>> Makes sense. Also, please ignore my "change the return value"
>> thing, I got confused between unmap_folios, ret_folios, etc.
>> Now I think I understood what the lists are doing :)
>>
>>>
>>> If it's really important to retry after undoing all, we can either
>>> convert two retying loops of migrate_pages_batch() into one loop, or
>>> remove retry loop in migrate_pages_batch() and retry in its caller
>>> instead.
>>
>> And if I implemented this correctly, the following makes the test
>> pass always:
>> https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXe
>
>
> Okay, I did mess up with the implementation, leading to a false
> positive. Let me try again :)


Hopefully this should do the job:
https://www.codedump.xyz/diff/ZrsIV8JSOPYx5V_u

But the result is worse than the patch proposed; I rarely hit
a 3 digit number of successes of move_pages(). But, on a
base kernel without any changes, when I apply David's
suggestion to change the test, if I choose 7 as the number
of retries (= NR_MAX_MIGRATE_SYNC_RETRY) in the test, I
can touch even 4 digits. I am puzzled.
We can also try merging the for loops of unmap and move...

>
>>
>>
>>
>>>
>>> -- 
>>> Best Regards,
>>> Huang, Ying
>>
>

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/13/24 12:52, Dev Jain wrote:
>
> On 8/13/24 10:30, Dev Jain wrote:
>>
>> On 8/12/24 17:38, Dev Jain wrote:
>>>
>>> On 8/12/24 13:01, Huang, Ying wrote:
>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>
>>>>> On 8/12/24 11:45, Huang, Ying wrote:
>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>
>>>>>>> On 8/12/24 11:04, Huang, Ying wrote:
>>>>>>>> Hi, Dev,
>>>>>>>>
>>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>>
>>>>>>>>> As already being done in __migrate_folio(), wherein we backoff 
>>>>>>>>> if the
>>>>>>>>> folio refcount is wrong, make this check during the unmapping 
>>>>>>>>> phase, upon
>>>>>>>>> the failure of which, the original state of the PTEs will be 
>>>>>>>>> restored and
>>>>>>>>> the folio lock will be dropped via migrate_folio_undo_src(), 
>>>>>>>>> any racing
>>>>>>>>> thread will make progress and migration will be retried.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>>>>> ---
>>>>>>>>>     mm/migrate.c | 9 +++++++++
>>>>>>>>>     1 file changed, 9 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>>>>> --- a/mm/migrate.c
>>>>>>>>> +++ b/mm/migrate.c
>>>>>>>>> @@ -1250,6 +1250,15 @@ static int 
>>>>>>>>> migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>>>>>         }
>>>>>>>>>           if (!folio_mapped(src)) {
>>>>>>>>> +        /*
>>>>>>>>> +         * Someone may have changed the refcount and maybe 
>>>>>>>>> sleeping
>>>>>>>>> +         * on the folio lock. In case of refcount mismatch, 
>>>>>>>>> bail out,
>>>>>>>>> +         * let the system make progress and retry.
>>>>>>>>> +         */
>>>>>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>>>>>> +
>>>>>>>>> +        if (folio_ref_count(src) != 
>>>>>>>>> folio_expected_refs(mapping, src))
>>>>>>>>> +            goto out;
>>>>>>>>>             __migrate_folio_record(dst, old_page_state, 
>>>>>>>>> anon_vma);
>>>>>>>>>             return MIGRATEPAGE_UNMAP;
>>>>>>>>>         }
>>>>>>>> Do you have some test results for this?  For example, after 
>>>>>>>> applying the
>>>>>>>> patch, the migration success rate increased XX%, etc.
>>>>>>> I'll get back to you on this.
>>>>>>>
>>>>>>>> My understanding for this issue is that the migration success 
>>>>>>>> rate can
>>>>>>>> increase if we undo all changes before retrying. This is the 
>>>>>>>> current
>>>>>>>> behavior for sync migration, but not for async migration.  If 
>>>>>>>> so, we can
>>>>>>>> use migrate_pages_sync() for async migration too to increase 
>>>>>>>> success
>>>>>>>> rate?  Of course, we need to change the function name and 
>>>>>>>> comments.
>>>>>>> As per my understanding, this is not the current behaviour for sync
>>>>>>> migration. After successful unmapping, we fail in 
>>>>>>> migrate_folio_move()
>>>>>>> with -EAGAIN, we do not call undo src+dst (rendering the loop 
>>>>>>> around
>>>>>>> migrate_folio_move() futile), we do not push the failed folio 
>>>>>>> onto the
>>>>>>> ret_folios list, therefore, in _sync(), _batch() is never tried 
>>>>>>> again.
>>>>>> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
>>>>>> called first, if failed, the folio will be restored to the original
>>>>>> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called 
>>>>>> again.
>>>>>> So, we unlock once.  If it's necessary, we can unlock more times via
>>>>>> another level of loop.
>>>>> Yes, that's my point. We need to undo src+dst and retry.
>>>> For sync migration, we undo src+dst and retry now, but only once.  You
>>>> have shown that more retrying increases success rate.
>>>>
>>>>> We will have
>>>>> to decide where we want this retrying to be; do we want to change the
>>>>> return value, end up in the while loop wrapped around _sync(), and 
>>>>> retry
>>>>> there by adding another level of loop, or do we want to make use 
>>>>> of the
>>>>> existing retry loops, one of which is wrapped around _unmap(); the 
>>>>> latter
>>>>> is my approach. The utility I see for the former approach is that, 
>>>>> in case
>>>>> of a large number of page migrations (which should usually be the 
>>>>> case),
>>>>> we are giving more time for the folio to get retried. The latter 
>>>>> does not
>>>>> give much time and discards the folio if it did not succeed under 
>>>>> 7 times.
>>>> Because it's a race, I guess that most folios will be migrated
>>>> successfully in the first pass.
>>>>
>>>> My concerns of your method are that it deal with just one case
>>>> specially.  While retrying after undoing all appears more general.
>>>
>>>
>>> Makes sense. Also, please ignore my "change the return value"
>>> thing, I got confused between unmap_folios, ret_folios, etc.
>>> Now I think I understood what the lists are doing :)
>>>
>>>>
>>>> If it's really important to retry after undoing all, we can either
>>>> convert two retying loops of migrate_pages_batch() into one loop, or
>>>> remove retry loop in migrate_pages_batch() and retry in its caller
>>>> instead.
>>>
>>> And if I implemented this correctly, the following makes the test
>>> pass always:
>>> https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXe
>>
>>
>> Okay, I did mess up with the implementation, leading to a false
>> positive. Let me try again :)
>
>
> Hopefully this should do the job:
> https://www.codedump.xyz/diff/ZrsIV8JSOPYx5V_u
>
> But the result is worse than the patch proposed; I rarely hit
> a 3 digit number of successes of move_pages(). But, on a
> base kernel without any changes, when I apply David's
> suggestion to change the test, if I choose 7 as the number
> of retries (= NR_MAX_MIGRATE_SYNC_RETRY) in the test, I
> can touch even 4 digits. I am puzzled.
> We can also try merging the for loops of unmap and move...


If people are okay with this change, I guess I can send it as
a v2? I concur with your assessment that my initial approach
is solving a specific case; the above approach does give me
a slight improvement on arm64 and should be an improvement
in general, since it makes sense to defer retrying the failed folio
as much as we can.

>

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Huang, Ying 1 year, 5 months ago

Dev Jain <dev.jain@arm.com> writes:

> On 8/13/24 12:52, Dev Jain wrote:
>>
>> On 8/13/24 10:30, Dev Jain wrote:
>>>
>>> On 8/12/24 17:38, Dev Jain wrote:
>>>>
>>>> On 8/12/24 13:01, Huang, Ying wrote:
>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>
>>>>>> On 8/12/24 11:45, Huang, Ying wrote:
>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>
>>>>>>>> On 8/12/24 11:04, Huang, Ying wrote:
>>>>>>>>> Hi, Dev,
>>>>>>>>>
>>>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>>>
>>>>>>>>>> As already being done in __migrate_folio(), wherein we
>>>>>>>>>> backoff if the
>>>>>>>>>> folio refcount is wrong, make this check during the
>>>>>>>>>> unmapping phase, upon
>>>>>>>>>> the failure of which, the original state of the PTEs will be
>>>>>>>>>> restored and
>>>>>>>>>> the folio lock will be dropped via migrate_folio_undo_src(),
>>>>>>>>>> any racing
>>>>>>>>>> thread will make progress and migration will be retried.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>>>>>> ---
>>>>>>>>>>     mm/migrate.c | 9 +++++++++
>>>>>>>>>>     1 file changed, 9 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>>>>>> --- a/mm/migrate.c
>>>>>>>>>> +++ b/mm/migrate.c
>>>>>>>>>> @@ -1250,6 +1250,15 @@ static int
>>>>>>>>>> migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>>>>>>         }
>>>>>>>>>>           if (!folio_mapped(src)) {
>>>>>>>>>> +        /*
>>>>>>>>>> +         * Someone may have changed the refcount and maybe
>>>>>>>>>> sleeping
>>>>>>>>>> +         * on the folio lock. In case of refcount mismatch,
>>>>>>>>>> bail out,
>>>>>>>>>> +         * let the system make progress and retry.
>>>>>>>>>> +         */
>>>>>>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>>>>>>> +
>>>>>>>>>> +        if (folio_ref_count(src) !=
>>>>>>>>>> folio_expected_refs(mapping, src))
>>>>>>>>>> +            goto out;
>>>>>>>>>>             __migrate_folio_record(dst, old_page_state,
>>>>>>>>>> anon_vma);
>>>>>>>>>>             return MIGRATEPAGE_UNMAP;
>>>>>>>>>>         }
>>>>>>>>> Do you have some test results for this?  For example, after
>>>>>>>>> applying the
>>>>>>>>> patch, the migration success rate increased XX%, etc.
>>>>>>>> I'll get back to you on this.
>>>>>>>>
>>>>>>>>> My understanding for this issue is that the migration success
>>>>>>>>> rate can
>>>>>>>>> increase if we undo all changes before retrying. This is the
>>>>>>>>> current
>>>>>>>>> behavior for sync migration, but not for async migration.  If
>>>>>>>>> so, we can
>>>>>>>>> use migrate_pages_sync() for async migration too to increase
>>>>>>>>> success
>>>>>>>>> rate?  Of course, we need to change the function name and
>>>>>>>>> comments.
>>>>>>>> As per my understanding, this is not the current behaviour for sync
>>>>>>>> migration. After successful unmapping, we fail in
>>>>>>>> migrate_folio_move()
>>>>>>>> with -EAGAIN, we do not call undo src+dst (rendering the loop
>>>>>>>> around
>>>>>>>> migrate_folio_move() futile), we do not push the failed folio
>>>>>>>> onto the
>>>>>>>> ret_folios list, therefore, in _sync(), _batch() is never
>>>>>>>> tried again.
>>>>>>> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
>>>>>>> called first, if failed, the folio will be restored to the original
>>>>>>> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called
>>>>>>> again.
>>>>>>> So, we unlock once.  If it's necessary, we can unlock more times via
>>>>>>> another level of loop.
>>>>>> Yes, that's my point. We need to undo src+dst and retry.
>>>>> For sync migration, we undo src+dst and retry now, but only once.  You
>>>>> have shown that more retrying increases success rate.
>>>>>
>>>>>> We will have
>>>>>> to decide where we want this retrying to be; do we want to change the
>>>>>> return value, end up in the while loop wrapped around _sync(),
>>>>>> and retry
>>>>>> there by adding another level of loop, or do we want to make use
>>>>>> of the
>>>>>> existing retry loops, one of which is wrapped around _unmap();
>>>>>> the latter
>>>>>> is my approach. The utility I see for the former approach is
>>>>>> that, in case
>>>>>> of a large number of page migrations (which should usually be
>>>>>> the case),
>>>>>> we are giving more time for the folio to get retried. The latter
>>>>>> does not
>>>>>> give much time and discards the folio if it did not succeed
>>>>>> under 7 times.
>>>>> Because it's a race, I guess that most folios will be migrated
>>>>> successfully in the first pass.
>>>>>
>>>>> My concerns of your method are that it deal with just one case
>>>>> specially.  While retrying after undoing all appears more general.
>>>>
>>>>
>>>> Makes sense. Also, please ignore my "change the return value"
>>>> thing, I got confused between unmap_folios, ret_folios, etc.
>>>> Now I think I understood what the lists are doing :)
>>>>
>>>>>
>>>>> If it's really important to retry after undoing all, we can either
>>>>> convert two retying loops of migrate_pages_batch() into one loop, or
>>>>> remove retry loop in migrate_pages_batch() and retry in its caller
>>>>> instead.
>>>>
>>>> And if I implemented this correctly, the following makes the test
>>>> pass always:
>>>> https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXe
>>>
>>>
>>> Okay, I did mess up with the implementation, leading to a false
>>> positive. Let me try again :)
>>
>>
>> Hopefully this should do the job:
>> https://www.codedump.xyz/diff/ZrsIV8JSOPYx5V_u
>>
>> But the result is worse than the patch proposed; I rarely hit
>> a 3 digit number of successes of move_pages(). But, on a
>> base kernel without any changes, when I apply David's
>> suggestion to change the test, if I choose 7 as the number
>> of retries (= NR_MAX_MIGRATE_SYNC_RETRY) in the test, I
>> can touch even 4 digits. I am puzzled.
>> We can also try merging the for loops of unmap and move...
>
>
> If people are okay with this change, I guess I can send it as
> a v2? I concur with your assessment that my initial approach
> is solving a specific case; the above approach does give me
> a slight improvement on arm64 and should be an improvement
> in general, since it makes sense to defer retrying the failed folio
> as much as we can.

We need to deal with something else before a formal v2,

- stats need to be fixed, please check result processing for the first
  loop of migrate_pages_sync().

- Do we need something similar for async migration.

- Can we add another level of explicit loop for the second loop of
  migrate_pages_sync()?  That is to improve code readability.  Or, add a
  function to dot that?

- Is it good to remove retry loop in migrate_pages_batch()?  And do
  retry in the caller?

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/19/24 12:28, Huang, Ying wrote:
> Dev Jain <dev.jain@arm.com> writes:
>
>> On 8/13/24 12:52, Dev Jain wrote:
>>> On 8/13/24 10:30, Dev Jain wrote:
>>>> On 8/12/24 17:38, Dev Jain wrote:
>>>>> On 8/12/24 13:01, Huang, Ying wrote:
>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>
>>>>>>> On 8/12/24 11:45, Huang, Ying wrote:
>>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>>
>>>>>>>>> On 8/12/24 11:04, Huang, Ying wrote:
>>>>>>>>>> Hi, Dev,
>>>>>>>>>>
>>>>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>>>>
>>>>>>>>>>> As already being done in __migrate_folio(), wherein we
>>>>>>>>>>> backoff if the
>>>>>>>>>>> folio refcount is wrong, make this check during the
>>>>>>>>>>> unmapping phase, upon
>>>>>>>>>>> the failure of which, the original state of the PTEs will be
>>>>>>>>>>> restored and
>>>>>>>>>>> the folio lock will be dropped via migrate_folio_undo_src(),
>>>>>>>>>>> any racing
>>>>>>>>>>> thread will make progress and migration will be retried.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>>>>>>> ---
>>>>>>>>>>>      mm/migrate.c | 9 +++++++++
>>>>>>>>>>>      1 file changed, 9 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>>>>>>> --- a/mm/migrate.c
>>>>>>>>>>> +++ b/mm/migrate.c
>>>>>>>>>>> @@ -1250,6 +1250,15 @@ static int
>>>>>>>>>>> migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>>>>>>>          }
>>>>>>>>>>>            if (!folio_mapped(src)) {
>>>>>>>>>>> +        /*
>>>>>>>>>>> +         * Someone may have changed the refcount and maybe
>>>>>>>>>>> sleeping
>>>>>>>>>>> +         * on the folio lock. In case of refcount mismatch,
>>>>>>>>>>> bail out,
>>>>>>>>>>> +         * let the system make progress and retry.
>>>>>>>>>>> +         */
>>>>>>>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>>>>>>>> +
>>>>>>>>>>> +        if (folio_ref_count(src) !=
>>>>>>>>>>> folio_expected_refs(mapping, src))
>>>>>>>>>>> +            goto out;
>>>>>>>>>>>              __migrate_folio_record(dst, old_page_state,
>>>>>>>>>>> anon_vma);
>>>>>>>>>>>              return MIGRATEPAGE_UNMAP;
>>>>>>>>>>>          }
>>>>>>>>>> Do you have some test results for this?  For example, after
>>>>>>>>>> applying the
>>>>>>>>>> patch, the migration success rate increased XX%, etc.
>>>>>>>>> I'll get back to you on this.
>>>>>>>>>
>>>>>>>>>> My understanding for this issue is that the migration success
>>>>>>>>>> rate can
>>>>>>>>>> increase if we undo all changes before retrying. This is the
>>>>>>>>>> current
>>>>>>>>>> behavior for sync migration, but not for async migration.  If
>>>>>>>>>> so, we can
>>>>>>>>>> use migrate_pages_sync() for async migration too to increase
>>>>>>>>>> success
>>>>>>>>>> rate?  Of course, we need to change the function name and
>>>>>>>>>> comments.
>>>>>>>>> As per my understanding, this is not the current behaviour for sync
>>>>>>>>> migration. After successful unmapping, we fail in
>>>>>>>>> migrate_folio_move()
>>>>>>>>> with -EAGAIN, we do not call undo src+dst (rendering the loop
>>>>>>>>> around
>>>>>>>>> migrate_folio_move() futile), we do not push the failed folio
>>>>>>>>> onto the
>>>>>>>>> ret_folios list, therefore, in _sync(), _batch() is never
>>>>>>>>> tried again.
>>>>>>>> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
>>>>>>>> called first, if failed, the folio will be restored to the original
>>>>>>>> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called
>>>>>>>> again.
>>>>>>>> So, we unlock once.  If it's necessary, we can unlock more times via
>>>>>>>> another level of loop.
>>>>>>> Yes, that's my point. We need to undo src+dst and retry.
>>>>>> For sync migration, we undo src+dst and retry now, but only once.  You
>>>>>> have shown that more retrying increases success rate.
>>>>>>
>>>>>>> We will have
>>>>>>> to decide where we want this retrying to be; do we want to change the
>>>>>>> return value, end up in the while loop wrapped around _sync(),
>>>>>>> and retry
>>>>>>> there by adding another level of loop, or do we want to make use
>>>>>>> of the
>>>>>>> existing retry loops, one of which is wrapped around _unmap();
>>>>>>> the latter
>>>>>>> is my approach. The utility I see for the former approach is
>>>>>>> that, in case
>>>>>>> of a large number of page migrations (which should usually be
>>>>>>> the case),
>>>>>>> we are giving more time for the folio to get retried. The latter
>>>>>>> does not
>>>>>>> give much time and discards the folio if it did not succeed
>>>>>>> under 7 times.
>>>>>> Because it's a race, I guess that most folios will be migrated
>>>>>> successfully in the first pass.
>>>>>>
>>>>>> My concerns of your method are that it deal with just one case
>>>>>> specially.  While retrying after undoing all appears more general.
>>>>>
>>>>> Makes sense. Also, please ignore my "change the return value"
>>>>> thing, I got confused between unmap_folios, ret_folios, etc.
>>>>> Now I think I understood what the lists are doing :)
>>>>>
>>>>>> If it's really important to retry after undoing all, we can either
>>>>>> convert two retying loops of migrate_pages_batch() into one loop, or
>>>>>> remove retry loop in migrate_pages_batch() and retry in its caller
>>>>>> instead.
>>>>> And if I implemented this correctly, the following makes the test
>>>>> pass always:
>>>>> https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXe
>>>>
>>>> Okay, I did mess up with the implementation, leading to a false
>>>> positive. Let me try again :)
>>>
>>> Hopefully this should do the job:
>>> https://www.codedump.xyz/diff/ZrsIV8JSOPYx5V_u
>>>
>>> But the result is worse than the patch proposed; I rarely hit
>>> a 3 digit number of successes of move_pages(). But, on a
>>> base kernel without any changes, when I apply David's
>>> suggestion to change the test, if I choose 7 as the number
>>> of retries (= NR_MAX_MIGRATE_SYNC_RETRY) in the test, I
>>> can touch even 4 digits. I am puzzled.
>>> We can also try merging the for loops of unmap and move...
>>
>> If people are okay with this change, I guess I can send it as
>> a v2? I concur with your assessment that my initial approach
>> is solving a specific case; the above approach does give me
>> a slight improvement on arm64 and should be an improvement
>> in general, since it makes sense to defer retrying the failed folio
>> as much as we can.
> We need to deal with something else before a formal v2,
>
> - stats need to be fixed, please check result processing for the first
>    loop of migrate_pages_sync().

Sorry, can you point out where do they need to be fixed exactly?
The change I did is inside the while(!list_empty(from)) block,
and there is no stat computation being done there already.

>
> - Do we need something similar for async migration.
>
> - Can we add another level of explicit loop for the second loop of
>    migrate_pages_sync()?  That is to improve code readability.  Or, add a
>    function to dot that?
>
> - Is it good to remove retry loop in migrate_pages_batch()?  And do
>    retry in the caller?

I am personally in favour of leaving the retry loop, and async
migration, as it is. Since async version is basically minimal-effort
migration, it won't make sense to "optimize" it, given the code churn
it would create, including the change we will have to then do in
"if (mode == MIGRATE_ASYNC) => migrate_pages_batch(ASYNC)" inside
migrate_pages().

Sorry, what do you mean by "another level of explicit loop"?

>
> --
> Best Regards,
> Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Huang, Ying 1 year, 5 months ago

Dev Jain <dev.jain@arm.com> writes:

> On 8/19/24 12:28, Huang, Ying wrote:
>> Dev Jain <dev.jain@arm.com> writes:
>>
>>> On 8/13/24 12:52, Dev Jain wrote:
>>>> On 8/13/24 10:30, Dev Jain wrote:
>>>>> On 8/12/24 17:38, Dev Jain wrote:
>>>>>> On 8/12/24 13:01, Huang, Ying wrote:
>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>
>>>>>>>> On 8/12/24 11:45, Huang, Ying wrote:
>>>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>>>
>>>>>>>>>> On 8/12/24 11:04, Huang, Ying wrote:
>>>>>>>>>>> Hi, Dev,
>>>>>>>>>>>
>>>>>>>>>>> Dev Jain <dev.jain@arm.com> writes:
>>>>>>>>>>>
>>>>>>>>>>>> As already being done in __migrate_folio(), wherein we
>>>>>>>>>>>> backoff if the
>>>>>>>>>>>> folio refcount is wrong, make this check during the
>>>>>>>>>>>> unmapping phase, upon
>>>>>>>>>>>> the failure of which, the original state of the PTEs will be
>>>>>>>>>>>> restored and
>>>>>>>>>>>> the folio lock will be dropped via migrate_folio_undo_src(),
>>>>>>>>>>>> any racing
>>>>>>>>>>>> thread will make progress and migration will be retried.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>      mm/migrate.c | 9 +++++++++
>>>>>>>>>>>>      1 file changed, 9 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>>>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>>>>>>>> --- a/mm/migrate.c
>>>>>>>>>>>> +++ b/mm/migrate.c
>>>>>>>>>>>> @@ -1250,6 +1250,15 @@ static int
>>>>>>>>>>>> migrate_folio_unmap(new_folio_t get_new_folio,
>>>>>>>>>>>>          }
>>>>>>>>>>>>            if (!folio_mapped(src)) {
>>>>>>>>>>>> +        /*
>>>>>>>>>>>> +         * Someone may have changed the refcount and maybe
>>>>>>>>>>>> sleeping
>>>>>>>>>>>> +         * on the folio lock. In case of refcount mismatch,
>>>>>>>>>>>> bail out,
>>>>>>>>>>>> +         * let the system make progress and retry.
>>>>>>>>>>>> +         */
>>>>>>>>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>>>>>>>>> +
>>>>>>>>>>>> +        if (folio_ref_count(src) !=
>>>>>>>>>>>> folio_expected_refs(mapping, src))
>>>>>>>>>>>> +            goto out;
>>>>>>>>>>>>              __migrate_folio_record(dst, old_page_state,
>>>>>>>>>>>> anon_vma);
>>>>>>>>>>>>              return MIGRATEPAGE_UNMAP;
>>>>>>>>>>>>          }
>>>>>>>>>>> Do you have some test results for this?  For example, after
>>>>>>>>>>> applying the
>>>>>>>>>>> patch, the migration success rate increased XX%, etc.
>>>>>>>>>> I'll get back to you on this.
>>>>>>>>>>
>>>>>>>>>>> My understanding for this issue is that the migration success
>>>>>>>>>>> rate can
>>>>>>>>>>> increase if we undo all changes before retrying. This is the
>>>>>>>>>>> current
>>>>>>>>>>> behavior for sync migration, but not for async migration.  If
>>>>>>>>>>> so, we can
>>>>>>>>>>> use migrate_pages_sync() for async migration too to increase
>>>>>>>>>>> success
>>>>>>>>>>> rate?  Of course, we need to change the function name and
>>>>>>>>>>> comments.
>>>>>>>>>> As per my understanding, this is not the current behaviour for sync
>>>>>>>>>> migration. After successful unmapping, we fail in
>>>>>>>>>> migrate_folio_move()
>>>>>>>>>> with -EAGAIN, we do not call undo src+dst (rendering the loop
>>>>>>>>>> around
>>>>>>>>>> migrate_folio_move() futile), we do not push the failed folio
>>>>>>>>>> onto the
>>>>>>>>>> ret_folios list, therefore, in _sync(), _batch() is never
>>>>>>>>>> tried again.
>>>>>>>>> In migrate_pages_sync(), migrate_pages_batch(,MIGRATE_ASYNC) will be
>>>>>>>>> called first, if failed, the folio will be restored to the original
>>>>>>>>> state (unlocked).  Then migrate_pages_batch(,_SYNC*) is called
>>>>>>>>> again.
>>>>>>>>> So, we unlock once.  If it's necessary, we can unlock more times via
>>>>>>>>> another level of loop.
>>>>>>>> Yes, that's my point. We need to undo src+dst and retry.
>>>>>>> For sync migration, we undo src+dst and retry now, but only once.  You
>>>>>>> have shown that more retrying increases success rate.
>>>>>>>
>>>>>>>> We will have
>>>>>>>> to decide where we want this retrying to be; do we want to change the
>>>>>>>> return value, end up in the while loop wrapped around _sync(),
>>>>>>>> and retry
>>>>>>>> there by adding another level of loop, or do we want to make use
>>>>>>>> of the
>>>>>>>> existing retry loops, one of which is wrapped around _unmap();
>>>>>>>> the latter
>>>>>>>> is my approach. The utility I see for the former approach is
>>>>>>>> that, in case
>>>>>>>> of a large number of page migrations (which should usually be
>>>>>>>> the case),
>>>>>>>> we are giving more time for the folio to get retried. The latter
>>>>>>>> does not
>>>>>>>> give much time and discards the folio if it did not succeed
>>>>>>>> under 7 times.
>>>>>>> Because it's a race, I guess that most folios will be migrated
>>>>>>> successfully in the first pass.
>>>>>>>
>>>>>>> My concerns of your method are that it deal with just one case
>>>>>>> specially.  While retrying after undoing all appears more general.
>>>>>>
>>>>>> Makes sense. Also, please ignore my "change the return value"
>>>>>> thing, I got confused between unmap_folios, ret_folios, etc.
>>>>>> Now I think I understood what the lists are doing :)
>>>>>>
>>>>>>> If it's really important to retry after undoing all, we can either
>>>>>>> convert two retying loops of migrate_pages_batch() into one loop, or
>>>>>>> remove retry loop in migrate_pages_batch() and retry in its caller
>>>>>>> instead.
>>>>>> And if I implemented this correctly, the following makes the test
>>>>>> pass always:
>>>>>> https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXe
>>>>>
>>>>> Okay, I did mess up with the implementation, leading to a false
>>>>> positive. Let me try again :)
>>>>
>>>> Hopefully this should do the job:
>>>> https://www.codedump.xyz/diff/ZrsIV8JSOPYx5V_u
>>>>
>>>> But the result is worse than the patch proposed; I rarely hit
>>>> a 3 digit number of successes of move_pages(). But, on a
>>>> base kernel without any changes, when I apply David's
>>>> suggestion to change the test, if I choose 7 as the number
>>>> of retries (= NR_MAX_MIGRATE_SYNC_RETRY) in the test, I
>>>> can touch even 4 digits. I am puzzled.
>>>> We can also try merging the for loops of unmap and move...
>>>
>>> If people are okay with this change, I guess I can send it as
>>> a v2? I concur with your assessment that my initial approach
>>> is solving a specific case; the above approach does give me
>>> a slight improvement on arm64 and should be an improvement
>>> in general, since it makes sense to defer retrying the failed folio
>>> as much as we can.
>> We need to deal with something else before a formal v2,
>>
>> - stats need to be fixed, please check result processing for the first
>>    loop of migrate_pages_sync().
>
> Sorry, can you point out where do they need to be fixed exactly?
> The change I did is inside the while(!list_empty(from)) block,
> and there is no stat computation being done there already.

The migration failure for one folio may be counted multiple times during
retrying.  So, you may need to drop counting except the final one.

>>
>> - Do we need something similar for async migration.
>>
>> - Can we add another level of explicit loop for the second loop of
>>    migrate_pages_sync()?  That is to improve code readability.  Or, add a
>>    function to dot that?
>>
>> - Is it good to remove retry loop in migrate_pages_batch()?  And do
>>    retry in the caller?
>
> I am personally in favour of leaving the retry loop, and async
> migration, as it is. Since async version is basically minimal-effort
> migration, it won't make sense to "optimize" it, given the code churn
> it would create, including the change we will have to then do in
> "if (mode == MIGRATE_ASYNC) => migrate_pages_batch(ASYNC)" inside
> migrate_pages().

I think that it may make sense to improve async migration too if it's
possible and not too complex.  If necessary, we can refactor the code to
improve the readability.

> Sorry, what do you mean by "another level of explicit loop"?

I mean something like,

for (i = 0; i < NR_MAX_MIGRATE_SYNC_RETRY; i++) {
        while (!list_empty(from)) {
                migrate_pages_batch(, 1);
        }
}

--
Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by David Hildenbrand 1 year, 6 months ago

On 09.08.24 12:31, Dev Jain wrote:
> As already being done in __migrate_folio(), wherein we backoff if the
> folio refcount is wrong, make this check during the unmapping phase, upon
> the failure of which, the original state of the PTEs will be restored and
> the folio lock will be dropped via migrate_folio_undo_src(), any racing
> thread will make progress and migration will be retried.
> 
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
>   mm/migrate.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index e7296c0fb5d5..477acf996951 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>   	}
>   
>   	if (!folio_mapped(src)) {
> +		/*
> +		 * Someone may have changed the refcount and maybe sleeping
> +		 * on the folio lock. In case of refcount mismatch, bail out,
> +		 * let the system make progress and retry.
> +		 */
> +		struct address_space *mapping = folio_mapping(src);
> +
> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
> +			goto out;

This really seems to be the latest point where we can "easily" back off 
and unlock the source folio -- in this function :)

I wonder if we should be smarter in the migrate_pages_batch() loop when 
we start the actual migrations via migrate_folio_move(): if we detect 
that a folio has unexpected references *and* it has waiters 
(PG_waiters), back off then and retry the folio later. If it only has 
unexpected references, just keep retrying: no waiters -> nobody is 
waiting for the lock to make progress.

For example, when migrate_folio_move() fails with -EAGAIN, check if 
there are waiters (PG_waiter?) and undo+unlock to try again later.

But I'm not really a migration expert, so only my 2 cents :)

>   		__migrate_folio_record(dst, old_page_state, anon_vma);
>   		return MIGRATEPAGE_UNMAP;
>   	}

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Zi Yan 1 year, 6 months ago

On 9 Aug 2024, at 9:47, David Hildenbrand wrote:

> On 09.08.24 12:31, Dev Jain wrote:
>> As already being done in __migrate_folio(), wherein we backoff if the
>> folio refcount is wrong, make this check during the unmapping phase, upon
>> the failure of which, the original state of the PTEs will be restored and
>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>> thread will make progress and migration will be retried.
>>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> ---
>>   mm/migrate.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index e7296c0fb5d5..477acf996951 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>   	}
>>    	if (!folio_mapped(src)) {
>> +		/*
>> +		 * Someone may have changed the refcount and maybe sleeping
>> +		 * on the folio lock. In case of refcount mismatch, bail out,
>> +		 * let the system make progress and retry.
>> +		 */
>> +		struct address_space *mapping = folio_mapping(src);
>> +
>> +		if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>> +			goto out;
>
> This really seems to be the latest point where we can "easily" back off and unlock the source folio -- in this function :)
>
> I wonder if we should be smarter in the migrate_pages_batch() loop when we start the actual migrations via migrate_folio_move(): if we detect that a folio has unexpected references *and* it has waiters (PG_waiters), back off then and retry the folio later. If it only has unexpected references, just keep retrying: no waiters -> nobody is waiting for the lock to make progress.
>
> For example, when migrate_folio_move() fails with -EAGAIN, check if there are waiters (PG_waiter?) and undo+unlock to try again later.
>
> But I'm not really a migration expert, so only my 2 cents :)

Sounds reasonable to me. Also add Ying (the author of batch page migration).

--
Best Regards,
Yan, Zi

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 6 months ago

On 8/9/24 19:17, David Hildenbrand wrote:
> On 09.08.24 12:31, Dev Jain wrote:
>> As already being done in __migrate_folio(), wherein we backoff if the
>> folio refcount is wrong, make this check during the unmapping phase, 
>> upon
>> the failure of which, the original state of the PTEs will be restored 
>> and
>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>> thread will make progress and migration will be retried.
>>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> ---
>>   mm/migrate.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index e7296c0fb5d5..477acf996951 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t 
>> get_new_folio,
>>       }
>>         if (!folio_mapped(src)) {
>> +        /*
>> +         * Someone may have changed the refcount and maybe sleeping
>> +         * on the folio lock. In case of refcount mismatch, bail out,
>> +         * let the system make progress and retry.
>> +         */
>> +        struct address_space *mapping = folio_mapping(src);
>> +
>> +        if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>> +            goto out;
>
> This really seems to be the latest point where we can "easily" back 
> off and unlock the source folio -- in this function :)
>
> I wonder if we should be smarter in the migrate_pages_batch() loop 
> when we start the actual migrations via migrate_folio_move(): if we 
> detect that a folio has unexpected references *and* it has waiters 
> (PG_waiters), back off then and retry the folio later. If it only has 
> unexpected references, just keep retrying: no waiters -> nobody is 
> waiting for the lock to make progress.


The patch currently retries migration irrespective of the reason of 
refcount change.

If you are suggesting that, break the retrying according to two conditions:

1. If the folio has waiters, retry according to 
NR_MAX_MIGRATE_PAGES_RETRY = 10.

2. If not, retry for a large number of iterations, say 10,000, since we 
just need to keep

retrying till the racer finishes reading the folio/failing on 
folio_trylock(), and decrementing

refcount.

If so, we will have to make the check as a refcount freeze(with 
xas_lock()); if we don't do that,

anyone can increase the refcount again, reading data from a stale 
reference to the folio, making

our check futile (which begs the question: is commit 0609139 correct? 
Checking refcount mismatch

in __migrate_folio() is ineffective since after that, and before 
folio_ref_freeze() in __folio_migrate_mapping(),

the refcount may change.) As a result, the freeze will have to take 
place immediately after we unmap

the folios from everyone's address space, something like:

while (!folio_ref_freeze(src, expected_count) && ++retries < 10000) {

         if (folio has waiters)

                 break;    /* will be retried by the outer loop giving 
us 10 chances in total */

}


> This really seems to be the latest point where we can "easily" back 
> off and unlock the source folio -- in this function :)
> For example, when migrate_folio_move() fails with -EAGAIN, check if 
> there are waiters (PG_waiter?) and undo+unlock to try again later.


Currently, on -EAGAIN, migrate_folio_move() returns without undoing src 
and dst; even if we were to fall

through to _undo_src/dst, the folios will not be unmapped again since 
_unmap() and _move() are

wrapped around different loops. This is what I was hinting to when I 
wrote in the cover letter:

"...there is no way the refcount would be decremented; as a result, this 
renders the retrying

useless" since upon the failure of _move(), the lock will not be dropped 
(which is dropped

through undo_src()), rendering the _move() loop useless. Sorry, should 
have noted this there.

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by David Hildenbrand 1 year, 6 months ago

On 10.08.24 20:42, Dev Jain wrote:
> 
> On 8/9/24 19:17, David Hildenbrand wrote:
>> On 09.08.24 12:31, Dev Jain wrote:
>>> As already being done in __migrate_folio(), wherein we backoff if the
>>> folio refcount is wrong, make this check during the unmapping phase,
>>> upon
>>> the failure of which, the original state of the PTEs will be restored
>>> and
>>> the folio lock will be dropped via migrate_folio_undo_src(), any racing
>>> thread will make progress and migration will be retried.
>>>
>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>> ---
>>>    mm/migrate.c | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index e7296c0fb5d5..477acf996951 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t
>>> get_new_folio,
>>>        }
>>>          if (!folio_mapped(src)) {
>>> +        /*
>>> +         * Someone may have changed the refcount and maybe sleeping
>>> +         * on the folio lock. In case of refcount mismatch, bail out,
>>> +         * let the system make progress and retry.
>>> +         */
>>> +        struct address_space *mapping = folio_mapping(src);
>>> +
>>> +        if (folio_ref_count(src) != folio_expected_refs(mapping, src))
>>> +            goto out;
>>
>> This really seems to be the latest point where we can "easily" back
>> off and unlock the source folio -- in this function :)
>>
>> I wonder if we should be smarter in the migrate_pages_batch() loop
>> when we start the actual migrations via migrate_folio_move(): if we
>> detect that a folio has unexpected references *and* it has waiters
>> (PG_waiters), back off then and retry the folio later. If it only has
>> unexpected references, just keep retrying: no waiters -> nobody is
>> waiting for the lock to make progress.
> 
> 
> The patch currently retries migration irrespective of the reason of
> refcount change.
> 
> If you are suggesting that, break the retrying according to two conditions:

That's not what I am suggesting ...

> 
> 
>> This really seems to be the latest point where we can "easily" back
>> off and unlock the source folio -- in this function :)
>> For example, when migrate_folio_move() fails with -EAGAIN, check if
>> there are waiters (PG_waiter?) and undo+unlock to try again later.
> 
> 
> Currently, on -EAGAIN, migrate_folio_move() returns without undoing src
> and dst; even if we were to fall

...

I am wondering if we should detect here if there are waiters and undo 
src+dst.

Does that make sense?

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 6 months ago

On 8/11/24 00:22, David Hildenbrand wrote:
> On 10.08.24 20:42, Dev Jain wrote:
>>
>> On 8/9/24 19:17, David Hildenbrand wrote:
>>> On 09.08.24 12:31, Dev Jain wrote:
>>>> As already being done in __migrate_folio(), wherein we backoff if the
>>>> folio refcount is wrong, make this check during the unmapping phase,
>>>> upon
>>>> the failure of which, the original state of the PTEs will be restored
>>>> and
>>>> the folio lock will be dropped via migrate_folio_undo_src(), any 
>>>> racing
>>>> thread will make progress and migration will be retried.
>>>>
>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>> ---
>>>>    mm/migrate.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index e7296c0fb5d5..477acf996951 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t
>>>> get_new_folio,
>>>>        }
>>>>          if (!folio_mapped(src)) {
>>>> +        /*
>>>> +         * Someone may have changed the refcount and maybe sleeping
>>>> +         * on the folio lock. In case of refcount mismatch, bail out,
>>>> +         * let the system make progress and retry.
>>>> +         */
>>>> +        struct address_space *mapping = folio_mapping(src);
>>>> +
>>>> +        if (folio_ref_count(src) != folio_expected_refs(mapping, 
>>>> src))
>>>> +            goto out;
>>>
>>> This really seems to be the latest point where we can "easily" back
>>> off and unlock the source folio -- in this function :)
>>>
>>> I wonder if we should be smarter in the migrate_pages_batch() loop
>>> when we start the actual migrations via migrate_folio_move(): if we
>>> detect that a folio has unexpected references *and* it has waiters
>>> (PG_waiters), back off then and retry the folio later. If it only has
>>> unexpected references, just keep retrying: no waiters -> nobody is
>>> waiting for the lock to make progress.
>>
>>
>> The patch currently retries migration irrespective of the reason of
>> refcount change.
>>
>> If you are suggesting that, break the retrying according to two 
>> conditions:
>
> That's not what I am suggesting ...
>
>>
>>
>>> This really seems to be the latest point where we can "easily" back
>>> off and unlock the source folio -- in this function :)
>>> For example, when migrate_folio_move() fails with -EAGAIN, check if
>>> there are waiters (PG_waiter?) and undo+unlock to try again later.
>>
>>
>> Currently, on -EAGAIN, migrate_folio_move() returns without undoing src
>> and dst; even if we were to fall
>
> ...
>
> I am wondering if we should detect here if there are waiters and undo 
> src+dst.

After undoing src+dst, which restores the PTEs, how are you going to set the

PTEs to migration again? That is being done through migrate_folio_unmap(),

and the loops of _unmap() and _move() are different. Or am I missing 
something...

>

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by David Hildenbrand 1 year, 6 months ago

On 11.08.24 08:06, Dev Jain wrote:
> 
> On 8/11/24 00:22, David Hildenbrand wrote:
>> On 10.08.24 20:42, Dev Jain wrote:
>>>
>>> On 8/9/24 19:17, David Hildenbrand wrote:
>>>> On 09.08.24 12:31, Dev Jain wrote:
>>>>> As already being done in __migrate_folio(), wherein we backoff if the
>>>>> folio refcount is wrong, make this check during the unmapping phase,
>>>>> upon
>>>>> the failure of which, the original state of the PTEs will be restored
>>>>> and
>>>>> the folio lock will be dropped via migrate_folio_undo_src(), any
>>>>> racing
>>>>> thread will make progress and migration will be retried.
>>>>>
>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>> ---
>>>>>     mm/migrate.c | 9 +++++++++
>>>>>     1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>> --- a/mm/migrate.c
>>>>> +++ b/mm/migrate.c
>>>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t
>>>>> get_new_folio,
>>>>>         }
>>>>>           if (!folio_mapped(src)) {
>>>>> +        /*
>>>>> +         * Someone may have changed the refcount and maybe sleeping
>>>>> +         * on the folio lock. In case of refcount mismatch, bail out,
>>>>> +         * let the system make progress and retry.
>>>>> +         */
>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>> +
>>>>> +        if (folio_ref_count(src) != folio_expected_refs(mapping,
>>>>> src))
>>>>> +            goto out;
>>>>
>>>> This really seems to be the latest point where we can "easily" back
>>>> off and unlock the source folio -- in this function :)
>>>>
>>>> I wonder if we should be smarter in the migrate_pages_batch() loop
>>>> when we start the actual migrations via migrate_folio_move(): if we
>>>> detect that a folio has unexpected references *and* it has waiters
>>>> (PG_waiters), back off then and retry the folio later. If it only has
>>>> unexpected references, just keep retrying: no waiters -> nobody is
>>>> waiting for the lock to make progress.
>>>
>>>
>>> The patch currently retries migration irrespective of the reason of
>>> refcount change.
>>>
>>> If you are suggesting that, break the retrying according to two
>>> conditions:
>>
>> That's not what I am suggesting ...
>>
>>>
>>>
>>>> This really seems to be the latest point where we can "easily" back
>>>> off and unlock the source folio -- in this function :)
>>>> For example, when migrate_folio_move() fails with -EAGAIN, check if
>>>> there are waiters (PG_waiter?) and undo+unlock to try again later.
>>>
>>>
>>> Currently, on -EAGAIN, migrate_folio_move() returns without undoing src
>>> and dst; even if we were to fall
>>
>> ...
>>
>> I am wondering if we should detect here if there are waiters and undo
>> src+dst.
> 
> After undoing src+dst, which restores the PTEs, how are you going to set the
> 
> PTEs to migration again? That is being done through migrate_folio_unmap(),
> 
> and the loops of _unmap() and _move() are different. Or am I missing
> something...

Again, no expert on the code, but it would mean that if we detect that 
there are waiters, we would undo src+dst and add them to ret_folios, 
similar to what we do in "Cleanup remaining folios" at the end of 
migrate_pages_batch()?

So instead of retrying migration of that folio, just give it up 
immediately and retry again later.

Of course, this means that (without further modifications to that 
function), we would leave retrying these folios to the caller, such as 
in migrate_pages_sync(), where we move ret_folios to the tail of 
"folios" and retry migration.

Maybe one would want to optimize that retry logic with such "temporarily 
failed because someone else has to make progress for us to make progress 
and free up a page reference" case. These are different to the typical 
"speculative" references that we try to handle via the existing retry magic.

Please let me know if I am missing something fundamental.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Dev Jain 1 year, 5 months ago

On 8/11/24 14:38, David Hildenbrand wrote:
> On 11.08.24 08:06, Dev Jain wrote:
>>
>> On 8/11/24 00:22, David Hildenbrand wrote:
>>> On 10.08.24 20:42, Dev Jain wrote:
>>>>
>>>> On 8/9/24 19:17, David Hildenbrand wrote:
>>>>> On 09.08.24 12:31, Dev Jain wrote:
>>>>>> As already being done in __migrate_folio(), wherein we backoff if 
>>>>>> the
>>>>>> folio refcount is wrong, make this check during the unmapping phase,
>>>>>> upon
>>>>>> the failure of which, the original state of the PTEs will be 
>>>>>> restored
>>>>>> and
>>>>>> the folio lock will be dropped via migrate_folio_undo_src(), any
>>>>>> racing
>>>>>> thread will make progress and migration will be retried.
>>>>>>
>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>> ---
>>>>>>     mm/migrate.c | 9 +++++++++
>>>>>>     1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>> --- a/mm/migrate.c
>>>>>> +++ b/mm/migrate.c
>>>>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t
>>>>>> get_new_folio,
>>>>>>         }
>>>>>>           if (!folio_mapped(src)) {
>>>>>> +        /*
>>>>>> +         * Someone may have changed the refcount and maybe sleeping
>>>>>> +         * on the folio lock. In case of refcount mismatch, bail 
>>>>>> out,
>>>>>> +         * let the system make progress and retry.
>>>>>> +         */
>>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>>> +
>>>>>> +        if (folio_ref_count(src) != folio_expected_refs(mapping,
>>>>>> src))
>>>>>> +            goto out;
>>>>>
>>>>> This really seems to be the latest point where we can "easily" back
>>>>> off and unlock the source folio -- in this function :)
>>>>>
>>>>> I wonder if we should be smarter in the migrate_pages_batch() loop
>>>>> when we start the actual migrations via migrate_folio_move(): if we
>>>>> detect that a folio has unexpected references *and* it has waiters
>>>>> (PG_waiters), back off then and retry the folio later. If it only has
>>>>> unexpected references, just keep retrying: no waiters -> nobody is
>>>>> waiting for the lock to make progress.
>>>>
>>>>
>>>> The patch currently retries migration irrespective of the reason of
>>>> refcount change.
>>>>
>>>> If you are suggesting that, break the retrying according to two
>>>> conditions:
>>>
>>> That's not what I am suggesting ...
>>>
>>>>
>>>>
>>>>> This really seems to be the latest point where we can "easily" back
>>>>> off and unlock the source folio -- in this function :)
>>>>> For example, when migrate_folio_move() fails with -EAGAIN, check if
>>>>> there are waiters (PG_waiter?) and undo+unlock to try again later.
>>>>
>>>>
>>>> Currently, on -EAGAIN, migrate_folio_move() returns without undoing 
>>>> src
>>>> and dst; even if we were to fall
>>>
>>> ...
>>>
>>> I am wondering if we should detect here if there are waiters and undo
>>> src+dst.
>>
>> After undoing src+dst, which restores the PTEs, how are you going to 
>> set the
>>
>> PTEs to migration again? That is being done through 
>> migrate_folio_unmap(),
>>
>> and the loops of _unmap() and _move() are different. Or am I missing
>> something...
>
> Again, no expert on the code, but it would mean that if we detect that 
> there are waiters, we would undo src+dst and add them to ret_folios, 
> similar to what we do in "Cleanup remaining folios" at the end of 
> migrate_pages_batch()?
>
> So instead of retrying migration of that folio, just give it up 
> immediately and retry again later.
>
> Of course, this means that (without further modifications to that 
> function), we would leave retrying these folios to the caller, such as 
> in migrate_pages_sync(), where we move ret_folios to the tail of 
> "folios" and retry migration.

So IIUC, you are saying to change the return value in 
__folio_migrate_mapping(), so that when move_to_new_folio() fails

in migrate_folio_move(), we end up in the retrying loop of _sync() which 
calls _batch() in synchronous mode. Here, we

will have to make a change to decide how much we want to retry?

>
>
> Maybe one would want to optimize that retry logic with such 
> "temporarily failed because someone else has to make progress for us 
> to make progress and free up a page reference" case. These are 
> different to the typical "speculative" references that we try to 
> handle via the existing retry magic.
>
> Please let me know if I am missing something fundamental.
>
>

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by David Hildenbrand 1 year, 5 months ago

On 12.08.24 07:35, Dev Jain wrote:
> 
> On 8/11/24 14:38, David Hildenbrand wrote:
>> On 11.08.24 08:06, Dev Jain wrote:
>>>
>>> On 8/11/24 00:22, David Hildenbrand wrote:
>>>> On 10.08.24 20:42, Dev Jain wrote:
>>>>>
>>>>> On 8/9/24 19:17, David Hildenbrand wrote:
>>>>>> On 09.08.24 12:31, Dev Jain wrote:
>>>>>>> As already being done in __migrate_folio(), wherein we backoff if
>>>>>>> the
>>>>>>> folio refcount is wrong, make this check during the unmapping phase,
>>>>>>> upon
>>>>>>> the failure of which, the original state of the PTEs will be
>>>>>>> restored
>>>>>>> and
>>>>>>> the folio lock will be dropped via migrate_folio_undo_src(), any
>>>>>>> racing
>>>>>>> thread will make progress and migration will be retried.
>>>>>>>
>>>>>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>>>>>> ---
>>>>>>>      mm/migrate.c | 9 +++++++++
>>>>>>>      1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>>>> index e7296c0fb5d5..477acf996951 100644
>>>>>>> --- a/mm/migrate.c
>>>>>>> +++ b/mm/migrate.c
>>>>>>> @@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t
>>>>>>> get_new_folio,
>>>>>>>          }
>>>>>>>            if (!folio_mapped(src)) {
>>>>>>> +        /*
>>>>>>> +         * Someone may have changed the refcount and maybe sleeping
>>>>>>> +         * on the folio lock. In case of refcount mismatch, bail
>>>>>>> out,
>>>>>>> +         * let the system make progress and retry.
>>>>>>> +         */
>>>>>>> +        struct address_space *mapping = folio_mapping(src);
>>>>>>> +
>>>>>>> +        if (folio_ref_count(src) != folio_expected_refs(mapping,
>>>>>>> src))
>>>>>>> +            goto out;
>>>>>>
>>>>>> This really seems to be the latest point where we can "easily" back
>>>>>> off and unlock the source folio -- in this function :)
>>>>>>
>>>>>> I wonder if we should be smarter in the migrate_pages_batch() loop
>>>>>> when we start the actual migrations via migrate_folio_move(): if we
>>>>>> detect that a folio has unexpected references *and* it has waiters
>>>>>> (PG_waiters), back off then and retry the folio later. If it only has
>>>>>> unexpected references, just keep retrying: no waiters -> nobody is
>>>>>> waiting for the lock to make progress.
>>>>>
>>>>>
>>>>> The patch currently retries migration irrespective of the reason of
>>>>> refcount change.
>>>>>
>>>>> If you are suggesting that, break the retrying according to two
>>>>> conditions:
>>>>
>>>> That's not what I am suggesting ...
>>>>
>>>>>
>>>>>
>>>>>> This really seems to be the latest point where we can "easily" back
>>>>>> off and unlock the source folio -- in this function :)
>>>>>> For example, when migrate_folio_move() fails with -EAGAIN, check if
>>>>>> there are waiters (PG_waiter?) and undo+unlock to try again later.
>>>>>
>>>>>
>>>>> Currently, on -EAGAIN, migrate_folio_move() returns without undoing
>>>>> src
>>>>> and dst; even if we were to fall
>>>>
>>>> ...
>>>>
>>>> I am wondering if we should detect here if there are waiters and undo
>>>> src+dst.
>>>
>>> After undoing src+dst, which restores the PTEs, how are you going to
>>> set the
>>>
>>> PTEs to migration again? That is being done through
>>> migrate_folio_unmap(),
>>>
>>> and the loops of _unmap() and _move() are different. Or am I missing
>>> something...
>>
>> Again, no expert on the code, but it would mean that if we detect that
>> there are waiters, we would undo src+dst and add them to ret_folios,
>> similar to what we do in "Cleanup remaining folios" at the end of
>> migrate_pages_batch()?
>>
>> So instead of retrying migration of that folio, just give it up
>> immediately and retry again later.
>>
>> Of course, this means that (without further modifications to that
>> function), we would leave retrying these folios to the caller, such as
>> in migrate_pages_sync(), where we move ret_folios to the tail of
>> "folios" and retry migration.
> 
> So IIUC, you are saying to change the return value in
> __folio_migrate_mapping(), so that when move_to_new_folio() fails
> 
> in migrate_folio_move(), we end up in the retrying loop of _sync() which
> calls _batch() in synchronous mode. Here, we
> 
> will have to make a change to decide how much we want to retry?

So essentially, instead of checking for "unexpected references" and 
backing off once at the beginning (what you do in this patch), we would 
*not* add new checks for "unexpected references" and not fail early in 
that case.

Instead, we would continuously check if there are waiters, and if there 
are waiters, we back-off completely (->unlock) instead of retrying 
something that cannot possibly make progress.

For "unexpected references" it can make sense to just retry immediately, 
because these might just be speculative references or short-term 
references that will go away soon.

For "unexpected reference with waiters" (or just "waiters" which should 
be the same because "waiters" should imply "unexpected references"), 
it's different as you discovered.

What we do with these "somebody else is waiting to make progress" pages 
is indeed a god question -- Ying seems to have some ideas in how to 
optimize retrying further.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch

Posted by Christoph Lameter (Ampere) 1 year, 6 months ago

On Fri, 9 Aug 2024, David Hildenbrand wrote:

>
> This really seems to be the latest point where we can "easily" back off and 
> unlock the source folio -- in this function :)
>
> I wonder if we should be smarter in the migrate_pages_batch() loop when we 
> start the actual migrations via migrate_folio_move(): if we detect that a 
> folio has unexpected references *and* it has waiters (PG_waiters), back off 
> then and retry the folio later. If it only has unexpected references, just 
> keep retrying: no waiters -> nobody is waiting for the lock to make progress.

Well just backoff ASAP if there are waiters detected anytime. A waiter 
would have increased the refcount. And a waiter will likely modify the page status soon. So 
push it to the end of the pages to be migrated to give it as much time 
as we can and check again later.

[PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
[PATCH 2/2] selftests/mm: Do not fail test for a single migration failure