Improve migration by backing off earlier
Subsystems (4)
cover Improve migration by backing off earlier
It was recently observed at [1] that during the folio unmapping stage of migration, when the PTEs are cleared, a racing thread faulting on that folio may increase the refcount of the folio, sleep on the folio lock (the migration path has the lock), and migration ultimately fails when asserting the actual refcount against the expected.
Migration is a best effort service; the unmapping and the moving phase are wrapped around loops for retrying. The refcount of the folio is currently being asserted during the move stage; if it fails, we retry. But, if a racing thread changes the refcount, and ends up sleeping on the folio lock (which is mostly the case), there is no way the refcount would be decremented; as a result, this renders the retrying useless. In the first patch, we make the refcount check also during the unmap stage; if it fails, we restore the original state of the PTE, drop the folio lock, let the system make progress, and retry unmapping again. This improves the probability of migration winning the race.
Given that migration is a best-effort service, it is wrong to fail the test for just a single failure; hence, fail the test after 100 consecutive failures (where 100 is still a subjective choice).
[1] https://lore.kernel.org/all/[email protected]/
Dev Jain (2):
mm: Retry migration earlier upon refcount mismatch selftests/mm: Do not fail test for a single migration failure
mm/migrate.c | 9 +++++++++ tools/testing/selftests/mm/migration.c | 17 +++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)
Signature · 1 line
2.30.2
PATCH 1/2 mm: Retry migration earlier upon refcount mismatch
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.
--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)
mm/migrate.c +9 -0
@@ -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 ; }
Signature · 1 line
2.30.2
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 18 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +0
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, }
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1261
Quoted 10 lines
mm/migrate.c +9
-0if (!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 :)
Quoted 3 lines
mm/migrate.c +0
-0__migrate_folio_record (dst, old_page_state, anon_vma);return MIGRATEPAGE_UNMAP ; }
Signature · 3 lines
Cheers, David / dhildenb
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 4 lines · David Hildenbrand
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 viamigrate_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.
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
In reply to David HildenbrandQuoted 32 lines · David Hildenbrand
Quoted 18 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>---
mm/migrate.c| 9 +++++++++1 file changed, 9 insertions(+)mm/migrate.c +0
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, }mm/migrate.c static int migrate_folio_unmap(new_folio_t · line 1261
Quoted 10 lines
mm/migrate.c +9
-0if (!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 :)
Quoted 1 line
I wonder if we should be smarter in the
migrate_pages_batch()loop when we start the actual migrations viamigrate_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 */
}
Quoted 2 lines
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
Quoted 45 lines · Dev Jain
Quoted 35 lines · David Hildenbrand
Quoted 20 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +0
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, }mm/migrate.c static int migrate_folio_unmap(new_folio_t · line 1261
Quoted 10 lines
mm/migrate.c +9
-0if (!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 :)
Quoted 6 lines
I wonder if we should be smarter in the
migrate_pages_batch()loop when we start the actual migrations viamigrate_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.
Quoted 4 lines
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 ...
Quoted 10 lines
Quoted 6 lines
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?
Signature · 3 lines
Cheers, David / dhildenb
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 62 lines · David Hildenbrand
Quoted 46 lines · Dev Jain
Quoted 36 lines · David Hildenbrand
Quoted 21 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>---
mm/migrate.c| 9 +++++++++1 file changed, 9 insertions(+)mm/migrate.c +0
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, } if (!folio_mapped(src)) {mm/migrate.c static int migrate_folio_unmap(new_folio_t · line 1262
Quoted 10 lines
mm/migrate.c +9
-0+ /* + * 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 :)
Quoted 6 lines
I wonder if we should be smarter in the
migrate_pages_batch()loop when we start the actual migrations viamigrate_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.Quoted 4 lines
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 ...
Quoted 8 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
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...
Quoted 1 line
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 74 lines · Dev Jain
Quoted 64 lines · David Hildenbrand
Quoted 47 lines · Dev Jain
Quoted 37 lines · David Hildenbrand
Quoted 22 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing
thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +0
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, } if (!folio_mapped(src)) {mm/migrate.c static int migrate_folio_unmap(new_folio_t · line 1262
Quoted 10 lines
mm/migrate.c +9
-0+ /* + * 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 :)
Quoted 6 lines
I wonder if we should be smarter in the
migrate_pages_batch()loop when we start the actual migrations viamigrate_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.Quoted 5 lines
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 ...
Quoted 8 lines
Quoted 6 lines
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.Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
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.
Signature · 3 lines
Cheers, David / dhildenb
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 80 lines · David Hildenbrand
Quoted 79 lines · Dev Jain
Quoted 65 lines · David Hildenbrand
Quoted 48 lines · Dev Jain
Quoted 38 lines · David Hildenbrand
Quoted 23 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing
thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>---
mm/migrate.c| 9 +++++++++1 file changed, 9 insertions(+)mm/migrate.c +1
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, } if (!folio_mapped(src)) { + /*mm/migrate.c static int migrate_folio_unmap(new_folio_t · line 1263
Quoted 10 lines
mm/migrate.c +8
-0+* 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 :)
Quoted 6 lines
I wonder if we should be smarter in the
migrate_pages_batch()loop when we start the actual migrations viamigrate_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.Quoted 5 lines
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 ...
Quoted 8 lines
Quoted 6 lines
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.Quoted 7 lines
Quoted 2 lines
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(),
Quoted 8 lines
Quoted 2 lines
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 ofmigrate_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 moveret_foliosto 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?
Quoted 7 lines
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
Quoted 103 lines · Dev Jain
Quoted 93 lines · David Hildenbrand
Quoted 78 lines · Dev Jain
Quoted 67 lines · David Hildenbrand
Quoted 50 lines · Dev Jain
Quoted 40 lines · David Hildenbrand
Quoted 25 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing
thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +1
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, } if (!folio_mapped(src)) { + /*mm/migrate.c static int migrate_folio_unmap(new_folio_t · line 1263
Quoted 10 lines
mm/migrate.c +8
-0+* 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 :)
Quoted 6 lines
I wonder if we should be smarter in the
migrate_pages_batch()loop when we start the actual migrations viamigrate_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.Quoted 5 lines
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 ...
Quoted 8 lines
Quoted 6 lines
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.Quoted 8 lines
Quoted 3 lines
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.
Quoted 8 lines
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 ofmigrate_pages_batch()?Quoted 7 lines
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 moveret_foliosto the tail of "folios" and retry migration.
Quoted 7 lines
So IIUC, you are saying to change the return value in
__folio_migrate_mapping(), so that whenmove_to_new_folio()failsin
migrate_folio_move(), we end up in the retrying loop of_sync()which calls_batch()in synchronous mode. Here, wewill 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.
Signature · 3 lines
Cheers, David / dhildenb
Zi Yan
reviewerRe: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
In reply to David HildenbrandQuoted 31 lines · David Hildenbrand
Quoted 17 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +0
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, }mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1260
Quoted 10 lines
mm/migrate.c +9
-0if (!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 :)
Quoted 5 lines
I wonder if we should be smarter in the
migrate_pages_batch()loop when we start the actual migrations viamigrate_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
In reply to Dev JainHi, Dev,
Quoted 21 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +2
-0@@ -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
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 10 lines
mm/migrate.c +7
-0+* 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
Quoted 22 lines · Ying
Hi, Dev,
Quoted 18 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +0
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, }
Quoted 15 lines
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 13 lines
mm/migrate.c +9
-0if (!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.
Quoted 6 lines
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.
Quoted 4 lines
--
Best Regards,
Huang, Ying
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 48 lines · Dev Jain
Quoted 28 lines · Ying
Hi, Dev,
Quoted 24 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +6
-0@@ -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);Quoted 8 lines
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 6 lines
mm/migrate.c +3
-0+ +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.
Quoted 6 lines
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.
Quoted 5 lines
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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, 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
Quoted 52 lines · Ying
Quoted 45 lines · Dev Jain
Quoted 28 lines · Ying
Hi, Dev,
Quoted 24 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +6
-0@@ -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);Quoted 8 lines
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 6 lines
mm/migrate.c +3
-0+ +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.
Quoted 5 lines
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.Quoted 5 lines
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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.
Quoted 5 lines
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). Thenmigrate_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.
Quoted 4 lines
--
Best Regards,
Huang, Ying
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 53 lines · Dev Jain
Quoted 52 lines · Ying
Quoted 50 lines · Dev Jain
Quoted 28 lines · Ying
Hi, Dev,
Quoted 24 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +6
-0@@ -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);Quoted 8 lines
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 6 lines
mm/migrate.c +3
-0+ +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.
Quoted 5 lines
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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.
Quoted 7 lines
Quoted 5 lines
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). Thenmigrate_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.
Quoted 9 lines
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
Quoted 72 lines · Ying
Quoted 52 lines · Dev Jain
Quoted 51 lines · Ying
Quoted 49 lines · Dev Jain
Quoted 28 lines · Ying
Hi, Dev,
Quoted 24 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +6
-0@@ -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);Quoted 8 lines
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 6 lines
mm/migrate.c +3
-0+ +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.
Quoted 5 lines
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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.Quoted 6 lines
Quoted 5 lines
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). Thenmigrate_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.
Quoted 9 lines
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.
Quoted 5 lines
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 :)
Quoted 5 lines
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 inmigrate_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
Quoted 4 lines
--
Best Regards,
Huang, Ying
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 90 lines · Dev Jain
Quoted 76 lines · Ying
Quoted 56 lines · Dev Jain
Quoted 55 lines · Ying
Quoted 53 lines · Dev Jain
Quoted 32 lines · Ying
Hi, Dev,
Quoted 28 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>---
mm/migrate.c| 9 +++++++++1 file changed, 9 insertions(+)mm/migrate.c +2
-0@@ -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. + */
+ structaddress_space*mapping =folio_mapping(src); +Quoted 8 lines
Quoted 6 lines
+ if (
folio_ref_count(src)!=folio_expected_refs(mapping, src))
+ goto out;
__migrate_folio_record(dst, old_page_state, anon_vma); returnMIGRATEPAGE_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.
Quoted 5 lines
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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.Quoted 6 lines
Quoted 5 lines
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). Thenmigrate_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.
Quoted 9 lines
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.Quoted 5 lines
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 :)Quoted 1 line
Quoted 8 lines
Quoted 4 lines
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 inmigrate_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 :)
Quoted 8 lines
Quoted 7 lines
Signature · 3 lines
-- Best Regards, Huang, Ying
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 97 lines · Dev Jain
Quoted 95 lines · Dev Jain
Quoted 76 lines · Ying
Quoted 56 lines · Dev Jain
Quoted 55 lines · Ying
Quoted 53 lines · Dev Jain
Quoted 32 lines · Ying
Hi, Dev,
Quoted 28 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>---
mm/migrate.c| 9 +++++++++1 file changed, 9 insertions(+)mm/migrate.c +2
-0@@ -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. + */
+ structaddress_space*mapping =folio_mapping(src); +Quoted 8 lines
Quoted 6 lines
+ if (
folio_ref_count(src)!=folio_expected_refs(mapping, src))
+ goto out;
__migrate_folio_record(dst, old_page_state, anon_vma); returnMIGRATEPAGE_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.
Quoted 5 lines
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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.Quoted 6 lines
Quoted 5 lines
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). Thenmigrate_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.
Quoted 9 lines
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.Quoted 5 lines
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 :)Quoted 5 lines
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 inmigrate_pages_batch()and retry in its caller instead.
Quoted 7 lines
Quoted 3 lines
And if I implemented this correctly, the following makes the test pass always:
https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXeOkay, 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...
Quoted 10 lines
Quoted 9 lines
Quoted 8 lines
Signature · 3 lines
-- Best Regards, Huang, Ying
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 111 lines · Dev Jain
Quoted 97 lines · Dev Jain
Quoted 95 lines · Dev Jain
Quoted 76 lines · Ying
Quoted 56 lines · Dev Jain
Quoted 55 lines · Ying
Quoted 53 lines · Dev Jain
Quoted 32 lines · Ying
Hi, Dev,
Quoted 28 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>---
mm/migrate.c| 9 +++++++++1 file changed, 9 insertions(+)mm/migrate.c +2
-0@@ -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. + */
+ structaddress_space*mapping =folio_mapping(src); +Quoted 8 lines
Quoted 6 lines
+ if (
folio_ref_count(src)!=folio_expected_refs(mapping, src))
+ goto out;
__migrate_folio_record(dst, old_page_state, anon_vma); returnMIGRATEPAGE_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.
Quoted 5 lines
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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.Quoted 6 lines
Quoted 5 lines
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). Thenmigrate_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.
Quoted 9 lines
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.Quoted 5 lines
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 :)Quoted 5 lines
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 inmigrate_pages_batch()and retry in its caller instead.Quoted 7 lines
Quoted 3 lines
And if I implemented this correctly, the following makes the test pass always:
https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXeOkay, 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
Quoted 7 lines
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.
Quoted 1 line
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 144 lines · Dev Jain
Quoted 134 lines · Dev Jain
Quoted 120 lines · Dev Jain
Quoted 118 lines · Dev Jain
Quoted 99 lines · Ying
Quoted 71 lines · Dev Jain
Quoted 70 lines · Ying
Quoted 68 lines · Dev Jain
Quoted 38 lines · Ying
Hi, Dev,
Quoted 34 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing
thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>---
mm/migrate.c| 9 +++++++++1 file changed, 9 insertions(+)mm/migrate.c +2
-0@@ -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. + */
+ structaddress_space*mapping =folio_mapping(src); +
+ if (folio_ref_count(src)!=folio_expected_refs(mapping, src))Quoted 8 lines
Quoted 5 lines
+ goto out;
__migrate_folio_record(dst, old_page_state, anon_vma);
returnMIGRATEPAGE_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.
Quoted 10 lines
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
usemigrate_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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.Quoted 7 lines
Quoted 6 lines
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). Thenmigrate_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.
Quoted 8 lines
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 latterQuoted 8 lines
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.Quoted 5 lines
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 :)Quoted 5 lines
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 inmigrate_pages_batch()and retry in its caller instead.Quoted 7 lines
Quoted 3 lines
And if I implemented this correctly, the following makes the test pass always:
https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXeOkay, 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_uQuoted 7 lines
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...
Quoted 6 lines
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
Quoted 143 lines · Ying
Quoted 135 lines · Dev Jain
Quoted 126 lines · Dev Jain
Quoted 115 lines · Dev Jain
Quoted 114 lines · Dev Jain
Quoted 99 lines · Ying
Quoted 71 lines · Dev Jain
Quoted 70 lines · Ying
Quoted 68 lines · Dev Jain
Quoted 38 lines · Ying
Hi, Dev,
Quoted 34 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing
thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +2
-0@@ -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. + */
+ structaddress_space*mapping =folio_mapping(src); +
+ if (folio_ref_count(src)!=folio_expected_refs(mapping, src))Quoted 8 lines
Quoted 5 lines
+ goto out;
__migrate_folio_record(dst,old_page_state,anon_vma);
returnMIGRATEPAGE_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.
Quoted 10 lines
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
usemigrate_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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.Quoted 7 lines
Quoted 6 lines
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). Thenmigrate_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.
Quoted 8 lines
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 latterQuoted 8 lines
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.Quoted 5 lines
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 :)Quoted 4 lines
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 inmigrate_pages_batch()and retry in its caller instead.Quoted 6 lines
Quoted 3 lines
And if I implemented this correctly, the following makes the test pass always:
https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXeOkay, 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_uQuoted 7 lines
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...Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 9 lines
- 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"?
Quoted 4 lines
--
Best Regards,
Huang, Ying
Re: [PATCH 1/2] mm: Retry migration earlier upon refcount mismatch
Quoted 144 lines · Dev Jain
Quoted 143 lines · Ying
Quoted 141 lines · Dev Jain
Quoted 126 lines · Dev Jain
Quoted 115 lines · Dev Jain
Quoted 114 lines · Dev Jain
Quoted 99 lines · Ying
Quoted 71 lines · Dev Jain
Quoted 70 lines · Ying
Quoted 68 lines · Dev Jain
Quoted 38 lines · Ying
Hi, Dev,
Quoted 34 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing
thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +2
-0@@ -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. + */
+ structaddress_space*mapping =folio_mapping(src); +
+ if (folio_ref_count(src)!=folio_expected_refs(mapping, src))Quoted 8 lines
Quoted 5 lines
+ goto out;
__migrate_folio_record(dst,old_page_state,anon_vma);
returnMIGRATEPAGE_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.
Quoted 10 lines
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
usemigrate_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 aroundmigrate_folio_move()futile), we do not push the failed folio onto theret_folioslist, therefore, in_sync(),_batch()is never tried again.Quoted 7 lines
Quoted 6 lines
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). Thenmigrate_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.
Quoted 8 lines
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 latterQuoted 8 lines
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.Quoted 5 lines
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 :)Quoted 4 lines
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 inmigrate_pages_batch()and retry in its caller instead.Quoted 6 lines
Quoted 3 lines
And if I implemented this correctly, the following makes the test pass always:
https://www.codedump.xyz/diff/Zrn7EdxzNXmXyNXeOkay, 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_uQuoted 7 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 9 lines
- 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?
Quoted 6 lines
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)" insidemigrate_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.
Quoted 1 line
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
In reply to Huang, YingQuoted 22 lines · Ying
Hi, Dev,
Quoted 18 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +0
-0@@ -1250,6 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio, }
Quoted 15 lines
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 13 lines
mm/migrate.c +9
-0if (!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.
Quoted 10 lines
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
Quoted 38 lines · Dev Jain
Quoted 28 lines · Ying
Hi, Dev,
Quoted 24 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +6
-0@@ -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);Quoted 8 lines
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 6 lines
mm/migrate.c +3
-0+ +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.
Quoted 5 lines
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 theunmapping()will get retried 3 + 7 = 10 times.
Thanks! What is the test results for
https://lore.kernel.org/all/[email protected]/
?
Quoted 6 lines
Quoted 6 lines
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
Quoted 44 lines · Ying
Quoted 37 lines · Dev Jain
Quoted 28 lines · Ying
Hi, Dev,
Quoted 24 lines · Dev Jain
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 viamigrate_folio_undo_src(), any racing thread will make progress and migration will be retried.Signed-off-by Dev Jain <[email protected]>--- mm/migrate.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/migrate.c +6
-0@@ -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);Quoted 8 lines
mm/migrate.c static int migrate_folio_unmap(new_folio_t get_new_folio, · line 1264
Quoted 6 lines
mm/migrate.c +3
-0+ +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.
Quoted 5 lines
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 theunmapping()will get retried 3 + 7 = 10 times.
Quoted 5 lines
Thanks! What is the test results for
https://lore.kernel.org/all/[email protected]/
?
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.
Quoted 10 lines
Quoted 6 lines
Quoted 6 lines
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
PATCH 2/2 selftests/mm: Do not fail test for a single migration failure
In reply to Dev JainDo not fail the test for just a single instance of migration failure, since migration is a best-effort service.
--- tools/testing/selftests/mm/migration.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
tools/testing/selftests/mm/migration.c +11 -6
@@ -15,10 +15,10 @@#include <signal.h> #include <time.h> -#define TWOMEG (2<<20) -#define RUNTIME (20) - -#define ALIGN (x, a) (((x) + (a - 1)) & (~((a) - 1))) +#define TWOMEG (2<<20) +#define RUNTIME (20) +#define MAX_RETRIES 100 +#define ALIGN (x, a) (((x) + (a - 1)) & (~((a) - 1)))FIXTURE (migration) {
@@ -65,6 +65,7 @@ int migrate(uint64_t *ptr, int n1, int n2)int ret, tmp;int status =0 ;struct timespec ts1, ts2; +int failures =0 ;if (clock_gettime (CLOCK_MONOTONIC , &ts1))return -1 ;
@@ -79,13 +80,17 @@ int migrate(uint64_t *ptr, int n1, int n2) ret =move_pages (0 ,1 , (void **) &ptr, &n2, &status,MPOL_MF_MOVE_ALL );if (ret) { -if (ret >0 ) +if (ret >0 ) { +/* Migration is best effort; try again */ +if (++failures <MAX_RETRIES ) +continue ;printf ("Didn't migrate %d pages\n" , ret); + }else perror ("Couldn't migrate pages" );return -2 ; } - + failures =0 ; tmp = n2; n2 = n1; n1 = tmp;
Signature · 1 line
2.30.2
Shuah Khan
maintainerRe: [PATCH 2/2] selftests/mm: Do not fail test for a single migration failure
Quoted 2 lines · Dev Jain
Do not fail the test for just a single instance of migration failure, since migration is a best-effort service.
The cover letter says:
"Given that migration is a best-effort service, it is wrong to fail the test for just a single failure; hence, fail the test after 100 consecutive failures (where 100 is still a subjective choice)."
You do want to mention the above here.
The reason being, I would like to know what this does to the run-time of this test if migration fails and retried 100 times.
Quoted 46 lines
Signed-off-by Dev Jain <[email protected]>Suggested-by David Hildenbrand <[email protected]>Reviewed-by Ryan Roberts <[email protected]>Tested-by Ryan Roberts <[email protected]>--- tools/testing/selftests/mm/migration.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)tools/testing/selftests/mm/migration.c +9
-5@@ -15,10 +15,10 @@#include <signal.h> #include <time.h> -#define TWOMEG (2<<20) -#define RUNTIME (20) - -#define ALIGN (x, a) (((x) + (a - 1)) & (~((a) - 1))) +#define TWOMEG (2<<20) +#define RUNTIME (20) +#define MAX_RETRIES 100 +#define ALIGN (x, a) (((x) + (a - 1)) & (~((a) - 1)))FIXTURE (migration) {@@ -65,6 +65,7 @@ int migrate(uint64_t *ptr, int n1, int n2)int ret, tmp;int status =0 ;struct timespec ts1, ts2; +int failures =0 ;if (clock_gettime (CLOCK_MONOTONIC , &ts1))return -1 ;@@ -79,13 +80,17 @@ int migrate(uint64_t *ptr, int n1, int n2) ret =move_pages (0 ,1 , (void **) &ptr, &n2, &status,MPOL_MF_MOVE_ALL );if (ret) { -if (ret >0 ) +if (ret >0 ) { +/* Migration is best effort; try again */ +if (++failures <MAX_RETRIES ) +continue ;printf ("Didn't migrate %d pages\n" , ret);
tools/testing/selftests/mm/migration.c int migrate(uint64_t *ptr, int n1, int n2) · line 96
Quoted 10 lines
tools/testing/selftests/mm/migration.c +2
-1+ }else perror ("Couldn't migrate pages" );return -2 ; } - + failures =0 ; tmp = n2; n2 = n1; n1 = tmp;
thanks,
-- Shuah
Re: [PATCH 2/2] selftests/mm: Do not fail test for a single migration failure
Quoted 8 lines · Shuah Khan
"Given that migration is a best-effort service, it is wrong to fail the test for just a single failure; hence, fail the test after 100 consecutive failures (where 100 is still a subjective choice)."
You do want to mention the above here.
The reason being, I would like to know what this does to the run-time of this test if migration fails and retried 100 times.
If we backoff earlier without engaging too much with the page then we can in turn affort to retry more times.
Re: [PATCH 2/2] selftests/mm: Do not fail test for a single migration failure
In reply to Shuah KhanQuoted 11 lines · Shuah Khan
Quoted 2 lines · Dev Jain
Do not fail the test for just a single instance of migration failure, since migration is a best-effort service.
The cover letter says:
"Given that migration is a best-effort service, it is wrong to fail the test for just a single failure; hence, fail the test after 100 consecutive failures (where 100 is still a subjective choice)."
You do want to mention the above here.
Sure, shall update in v2.
Quoted 3 lines
The reason being, I would like to know what this does to the run-time of this test if migration fails and retried 100 times.
Sure; just for the note, it won't affect the execution time of the test since that is controlled by a timeout mechanism.
Quoted 57 lines
Quoted 46 lines
Signed-off-by Dev Jain <[email protected]>Suggested-by David Hildenbrand <[email protected]>Reviewed-by Ryan Roberts <[email protected]>Tested-by Ryan Roberts <[email protected]>---
tools/testing/selftests/mm/migration.c| 17 +++++++++++------1 file changed, 11 insertions(+), 6 deletions(-)tools/testing/selftests/mm/migration.c +10
-4@@ -15,10 +15,10 @@ #include <signal.h> #include <time.h> -#define TWOMEG (2<<20) -#define RUNTIME (20) - -#define ALIGN (x, a) (((x) + (a - 1)) & (~((a) - 1))) +#define TWOMEG (2<<20) +#define RUNTIME (20) +#define MAX_RETRIES 100 +#define ALIGN (x, a) (((x) + (a - 1)) & (~((a) - 1))) FIXTURE(migration) {@@ -65,6 +65,7 @@ int migrate(uint64_t *ptr, int n1, int n2) int ret, tmp; int status = 0; struct timespec ts1, ts2; + int failures = 0; if (clock_gettime(CLOCK_MONOTONIC, &ts1)) return -1;@@ -79,13 +80,17 @@ int migrate(uint64_t *ptr, int n1, int n2) ret = move_pages(0, 1, (void **) &ptr, &n2, &status, MPOL_MF_MOVE_ALL); if (ret) { - if (ret > 0) + if (ret > 0) { + /* Migration is best effort; try again */ + if (++failures < MAX_RETRIES) + continue; printf("Didn't migrate %d pages\n", ret); + }else
Quoted 8 lines
perror("Couldn't migrate pages"); return -2;
}
-
+ failures = 0;
tmp = n2;
n2 = n1;
n1 = tmp;thanks,
-- Shuah