Re: [PATCH] mm/migrate_device.c: Fix a misleading and out-dated comment

[Alistair Popple <apopple@nvidia.com>]

Peter Xu <peterx@redhat.com> writes:

[...]

>> >>> 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.
>> >>
>> >> Will add.
>> >>
>> >>> In short, do you think something like this might be clearer?
>> >>
>> >> I think it's important to mention the optimisation, otherwise the
>> >> temptation is to remove the installation of migration entries here and
>> >> rely on try_to_migrate() to do it later. I would actually like to be
>> >> able to do that because it simplifies the code in many ways but based on
>> >> my testing the optimisation turns out to be very worth while.
>> >>
>> >>> 		/*
>> >>> 		 * 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.
>> >>
>> >> Honestly I think this describes what the code does rather than why and
>> >> is likely to become outdated and confusing. IMHO it's quite clear from
>> >> the code that the migration will fail here if we can't lock the page.
>> >
>> > If you see that's what I was struggling to understand previously, so not
>> > clear at least to me. :) Since normally a function like page migration
>> > should (from the gut feeling) not rely on trylock only.
>>
>> IIRC, ordinary page migration will also only trylock. See
>> isolate_movable_page().
>
> Fair enough.
>
> I think it depends on what we want to do with the migration.  I think not
> even all users of isolate_movable_page() only depend on trylock, and it can
> retry with lock_page later.  E.g.:
>
> - soft_offline_in_use_page
>   - isolate_page
>     - isolate_movable_page <----- trylock here
>   - (if isolate_page fails...) migrate_pages
>     - unmap_and_move       <----- lock_page here after 2 unsuccessful rounds
>
> And I also agree migration may always fail (e.g. by spurious page refcounts
> being taken), so feel free to ignore the "gut feeling" - that's indeed kind
> of subjective.

So how about the following:

		/*
		 * We rely on trylock_page() to avoid deadlock between
		 * concurrent migrations where each is waiting on the others
		 * page lock. If we can't immediately lock the page we fail this
		 * migration as it is only best effort anyway.
		 *
		 * If we can lock the page 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().
		 */

Thanks.

 - Alistair