On Tuesday, 23 November 2021 4:16:55 AM AEDT Felix Kuehling wrote:
[...]
> > Right, so long as my fix goes in I don't think there is anything wrong with
> > pinning device public pages. Agree that we should avoid FOLL_LONGTERM pins
> > for
> > device memory though. I think the way to do that is update
> > is_pinnable_page()
> > so we treat device pages the same as other unpinnable pages ie. long-term
> > pins
> > will migrate the page.
>
> I'm trying to understand check_and_migrate_movable_pages in gup.c. It
> doesn't look like the right way to migrate device pages. We may have to
> do something different there as well. So instead of changing
> is_pinnable_page, it maybe better to explicitly check for is_device_page
> or is_device_coherent_page in check_and_migrate_movable_pages to migrate
> it correctly, or just fail outright.
Yes, I think you're right. I was thinking check_and_migrate_movable_pages()
would work for coherent device pages. Now I see it won't because it assumes
they are lru pages and it tries to isolate them which will never succeed
because device pages aren't on a lru.
I think migrating them is the right thing to do for FOLL_LONGTERM though.
- Alistair
> Thanks,
> Felix
>
> >
> >>> In the case of device-private pages this is enforced by the fact they
> >>> never
> >>> have present pte's, so any attempt to GUP them results in a fault. But if
> >>> I'm
> >>> understanding this series correctly that won't be the case for coherent
> >>> device
> >>> pages right?
> >> Right.
> >>
> >> Regards,
> >> Felix
> >>
> >>
> >>>> - return is_device_private_page(page);
> >>>> + return is_device_page(page);
> >>>> }
> >>>>
> >>>> /* For file back page */
> >>>> @@ -2791,7 +2791,7 @@ EXPORT_SYMBOL(migrate_vma_setup);
> >>>> * handle_pte_fault()
> >>>> * do_anonymous_page()
> >>>> * to map in an anonymous zero page but the struct page will be a
> >>>> ZONE_DEVICE
> >>>> - * private page.
> >>>> + * private or coherent page.
> >>>> */
> >>>> static void migrate_vma_insert_page(struct migrate_vma *migrate,
> >>>> unsigned long addr,
> >>>> @@ -2867,10 +2867,15 @@ static void migrate_vma_insert_page(struct
> >>>> migrate_vma *migrate,
> >>>> swp_entry =
> >>>> make_readable_device_private_entry(
> >>>>
> >>>> page_to_pfn(page));
> >>>> entry = swp_entry_to_pte(swp_entry);
> >>>> + } else if (is_device_page(page)) {
> >>> How about adding an explicit `is_device_coherent_page()` helper? It would
> >>> make
> >>> the test more explicit that this is expected to handle just coherent
> >>> pages and
> >>> I bet there will be future changes that need to differentiate between
> >>> private
> >>> and coherent pages anyway.
> >>>
> >>>> + entry = pte_mkold(mk_pte(page,
> >>>> +
> >>>> READ_ONCE(vma->vm_page_prot)));
> >>>> + if (vma->vm_flags & VM_WRITE)
> >>>> + entry = pte_mkwrite(pte_mkdirty(entry));
> >>>> } else {
> >>>> /*
> >>>> - * For now we only support migrating to
> >>>> un-addressable
> >>>> - * device memory.
> >>>> + * We support migrating to private and coherent
> >>>> types
> >>>> + * for device zone memory.
> >>>> */
> >>>> pr_warn_once("Unsupported ZONE_DEVICE page
> >>>> type.\n");
> >>>> goto abort;
> >>>> @@ -2976,10 +2981,10 @@ void migrate_vma_pages(struct migrate_vma
> >>>> *migrate)
> >>>> mapping = page_mapping(page);
> >>>>
> >>>> if (is_zone_device_page(newpage)) {
> >>>> - if (is_device_private_page(newpage)) {
> >>>> + if (is_device_page(newpage)) {
> >>>> /*
> >>>> - * For now only support private
> >>>> anonymous when
> >>>> - * migrating to un-addressable device
> >>>> memory.
> >>>> + * For now only support private and
> >>>> coherent
> >>>> + * anonymous when migrating to device
> >>>> memory.
> >>>> */
> >>>> if (mapping) {
> >>>> migrate->src[i] &=
> >>>> ~MIGRATE_PFN_MIGRATE;
> >>>>
> >
> >
>
