| Message ID | 20240809103129.365029-2-dev.jain@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve migration by backing off earlier | expand |
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; > }
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.
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.
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?
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
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... >
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.
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
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. > >
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
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
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
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
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 >
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
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
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.
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
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 >
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 >> >
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. >
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
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
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
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; }
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(+)