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));
* 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;