by John Hubbard

[permalink] [raw]

Subject: Re: [PATCH v2 2/3] mm/gup.c: Migrate device coherent pages when pinning instead of failing

On 2/11/22 18:51, Alistair Popple wrote:
...
>>> @@ -1888,15 +1942,40 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
>>> continue;
>>> prev_head = head;
>>> /*
>>> - * If we get a movable page, since we are going to be pinning
>>> - * these entries, try to move them out if possible.
>>> + * Device coherent pages are managed by a driver and should not
>>> + * be pinned indefinitely as it prevents the driver moving the
>>> + * page. So when trying to pin with FOLL_LONGTERM instead try
>>> + * migrating page out of device memory.
>>> */
>>> if (is_dev_private_or_coherent_page(head)) {
>>> + /*
>>> + * device private pages will get faulted in during gup
>>> + * so it shouldn't be possible to see one here.
>>> + */
>>> WARN_ON_ONCE(is_device_private_page(head));
>>> - ret = -EFAULT;
>>> - goto unpin_pages;
>>> + WARN_ON_ONCE(PageCompound(head));
>>> +
>>> + /*
>>> + * migration will fail if the page is pinned, so convert
>>> + * the pin on the source page to a normal reference.
>>> + */
>>> + if (gup_flags & FOLL_PIN) {
>>> + get_page(head);
>>> + unpin_user_page(head);
>>
>> OK...but now gup_flags can no longer be used as a guide for how to
>> release these pages, right? In other words, up until this point,
>> FOLL_PIN meant "call unpin_user_page() in order to release". However,
>> now this page must be released via put_page().
>
> This is the source page (head). We are unpinning it because we can't migrate a
> pinned page, however we still need a reference on it for migrate_vma hence the
> get_page followed by unpin. In the non-FOLL_PIN case we already have a
> reference from gup.
>
>> See below...
>>
>>> + }
>>> +
>>> + pages[i] = migrate_device_page(head, gup_flags);
>
> migrate_device_page() will return a new page that has been correctly pinned
> with gup_flags by try_grab_page(). Therefore this page can still be released
> with unpin_user_page() or put_page() as appropriate for the given gup_flags.
>
> The reference we had on the source page (head) always gets dropped in
> migrate_vma_finalize().

OK. Good.

The above would be good to have in a comment, right around here, imho.
Because we have this marvelous mix of references for migration (get_page())
and other, and it's a bit hard to see that it's all correct without a
hint or two.

...
>
> Which unless I've missed something is still the correct thing to do.
>
>> This reminds me: out of the many things to monitor, the FOLL_PIN counts
>> in /proc/vmstat are especially helpful, whenever making changes to code
>> that deals with this:
>>
>> nr_foll_pin_acquired
>> nr_foll_pin_released
>>
>> ...and those should normally be equal to each other when "at rest".
>>

I hope this is/was run, just to be sure?

thanks,
--
John Hubbard
NVIDIA

2022-02-14 10:33:16

On 2/6/22 20:26, Alistair Popple wrote:
> migrate_vma_setup() checks that a valid vma is passed so that the page
> tables can be walked to find the pfns associated with a given address
> range. However in some cases the pfns are already known, such as when
> migrating device coherent pages during pin_user_pages() meaning a valid
> vma isn't required.
>
> Signed-off-by: Alistair Popple <[email protected]>
> Acked-by: Felix Kuehling <[email protected]>
> ---
>
> Changes for v2:
>
> - Added Felix's Acked-by
>
> mm/migrate.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
>

Hi Alistair,

Another late-breaking review question, below. :)

> diff --git a/mm/migrate.c b/mm/migrate.c
> index a9aed12..0d6570d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2602,24 +2602,24 @@ int migrate_vma_setup(struct migrate_vma *args)
>
> args->start &= PAGE_MASK;
> args->end &= PAGE_MASK;
> - if (!args->vma || is_vm_hugetlb_page(args->vma) ||
> - (args->vma->vm_flags & VM_SPECIAL) || vma_is_dax(args->vma))
> - return -EINVAL;
> - if (nr_pages <= 0)
> - return -EINVAL;

Was the above check left out intentionally? If so, then it needs a
commit description note. And maybe even a separate patch, because it
changes the behavior.

If you do want to change the behavior:

* The kerneldoc comment above this function supports such a change: it
requires returning 0 for the case of zero pages requested. So your
change would bring the comments into alignment with the code.

* I don't think memset deals properly with a zero length input arg, so
it's probably better to return 0, before that point.

thanks,
--
John Hubbard
NVIDIA

> - if (args->start < args->vma->vm_start ||
> - args->start >= args->vma->vm_end)
> - return -EINVAL;
> - if (args->end <= args->vma->vm_start || args->end > args->vma->vm_end)
> - return -EINVAL;
> if (!args->src || !args->dst)
> return -EINVAL;
> -
> - memset(args->src, 0, sizeof(*args->src) * nr_pages);
> - args->cpages = 0;
> - args->npages = 0;
> -
> - migrate_vma_collect(args);
> + if (args->vma) {
> + if (is_vm_hugetlb_page(args->vma) ||
> + (args->vma->vm_flags & VM_SPECIAL) || vma_is_dax(args->vma))
> + return -EINVAL;
> + if (args->start < args->vma->vm_start ||
> + args->start >= args->vma->vm_end)
> + return -EINVAL;
> + if (args->end <= args->vma->vm_start || args->end > args->vma->vm_end)
> + return -EINVAL;
> +
> + memset(args->src, 0, sizeof(*args->src) * nr_pages);
> + args->cpages = 0;
> + args->npages = 0;
> +
> + migrate_vma_collect(args);
> + }
>
> if (args->cpages)
> migrate_vma_unmap(args);
> @@ -2804,7 +2804,7 @@ void migrate_vma_pages(struct migrate_vma *migrate)
> continue;
> }
>
> - if (!page) {
> + if (!page && migrate->vma) {
> if (!(migrate->src[i] & MIGRATE_PFN_MIGRATE))
> continue;
> if (!notified) {