Re: [PATCH v2 2/2] mm/gup.c: Refactor check_and_migrate_movable_pages()

[Alistair Popple <apopple@nvidia.com>]

John Hubbard <jhubbard@nvidia.com> writes:

Thanks John, have fixed most of this but a couple of comments below.

> On 8/4/22 23:29, Alistair Popple wrote:

[...]

>>   +		/*
>> +		 * We can't migrate pages with unexpected references, so drop
>> +		 * the reference obtained by get_user_pages().
>
> The get_user_pages() reference is confusing, since we only handle FOLL_PIN
> here. It's hard to connect the comment to the code. Maybe a more precise
> pointer to where the reference was taken would help.

Ok. Is changing the reference to __get_user_pages_locked() better? (I
get that this is the FOLL_PIN case, but that is the function that
actually does that in __gup_longterm_locked()).

>> +		 * folio_isolate_lru() takes a reference so the page won't be
>> +		 * freed.
>
> Also confusing because it's difficult to connect the comment back to the code.
> Maybe mention where folio_isolate_lru() is called from in this case?

Will update this to:

		/*
		 * We can't migrate pages with unexpected references, so drop
		 * the reference obtained by __get_user_pages_locked().
		 * Migrating pages have been added to movable_page_list after
		 * calling folio_isolate_lru() which takes a reference so the
		 * page won't be freed if it's migrating.
		 */

I'd rather not explicitly call out the function where
folio_isolate_lru() is called as that could change. However it should be
pretty easy to find where pages are added to movable_page_list and pages
should never be on that list without being isolated anyway so this
better documents what the code assumes IMHO.

>> +		 */
>>   		unpin_user_page(pages[i]);
>> +		pages[i] = NULL;
>
> Is this correct? The loop covers all of nr_pages, so we are setting every
> pages[i] = NULL, for non-DEV_COHERENT cases. This seems wrong.

Yes, this is correct. Regardless of error or success we have to unpin
the pages, and if we've unpinned the pages we need to clear them from
the pages[] array. Will update the function description to include this.

This is also partly why I wasn't convinced the 'goto err' error handling
was better - we need to take the same actions regardless of error or not
and that perhaps makes it less clear.

[...]

>>   @@ -2049,10 +2066,10 @@ static long __gup_longterm_locked(struct mm_struct
>> *mm,
>>     		WARN_ON_ONCE(!(gup_flags & FOLL_PIN));
>>   		rc = check_and_migrate_movable_pages(rc, pages);
>> -	} while (!rc);
>> +	} while (rc == -EAGAIN);
>>   	memalloc_pin_restore(flags);
>>   -	return rc;
>> +	return rc ? rc : nr_pages;
>
> This seems to add some holes in the handling of error cases. If -EBUSY
> or -ENOMEM happens, then we return an -errno, rather than the number
> of pinned pages. But some pages may have been pinned.

No they won't - as mentioned above migrate_unpinnable_pages() will (or
at least should) always clear and unpin all pages.

 - Alistair

> Previously it just looped if there was any error, so that couldn't
> happen.
>
>
> thanks,