On Thu, Aug 25, 2022 at 11:49:05AM +1000, Alistair Popple wrote:
> Commit ab09243aa95a ("mm/migrate.c: remove MIGRATE_PFN_LOCKED") changed
> the way trylock_page() in migrate_vma_collect_pmd() works without
> updating the comment. Reword the comment to be less misleading and a
> better reflection of what happens.
>
> Signed-off-by: Alistair Popple <apopple@xxxxxxxxxx>
> Reported-by: Peter Xu <peterx@xxxxxxxxxx>
> Fixes: ab09243aa95a ("mm/migrate.c: remove MIGRATE_PFN_LOCKED")
> ---
> mm/migrate_device.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 5052093d0262..0736f846de0b 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -179,9 +179,11 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> get_page(page);
>
> /*
> - * Optimize for the common case where page is only mapped once
> - * in one process. If we can lock the page, then we can safely
> - * set up a special migration page table entry now.
> + * If we can't lock the page we can't migrate it. If we can it's
> + * safe to set up a migration entry now. In the common case
> + * where the page is mapped once in a single process setting up
> + * the migration entry now is an optimisation to avoid walking
> + * the rmap later with try_to_migrate().
> */
IMHO the last sentence can still be a bit confusing - here we 100% rely on
the trylock() to proceed or we'll stop migration right away. IMHO that
means this is not an optimization, since optimizations should always be
optional but not the case here.
Meanwhile it'll be great to also mention about why trylock is needed and no
further attempt to use lock_page(). The comment in prepare() previously
was great but unfortunately that code clip was removed.
In short, do you think something like this might be clearer?
/*
* We rely on the trylock() to migrate the pte. If this
* fails, we'll fail the migration of this page. IOW, the
* migration is very much best-effort, just like we'll also
* bail out if we found page pinned by other users after
* page being locked.
*
* We don't use lock_page() here or even later. It's
* because if to do so we may need to take the locks for
* multiple pages one by one, and the order to take the
* page locks is unclear. For example, if two processes
* want to lock multiple pages but in different order it
* can lead to deadlock.
*/
Thanks,
--
Peter Xu
