Am 2021-11-21 um 9:40 p.m. schrieb Alistair Popple: >>>> diff --git a/mm/migrate.c b/mm/migrate.c >>>> index 1852d787e6ab..f74422a42192 100644 >>>> --- a/mm/migrate.c >>>> +++ b/mm/migrate.c >>>> @@ -362,7 +362,7 @@ static int expected_page_refs(struct address_space *mapping, struct page *page) >>>> * Device private pages have an extra refcount as they are >>>> * ZONE_DEVICE pages. >>>> */ >>>> - expected_count += is_device_private_page(page); >>>> + expected_count += is_device_page(page); >>>> if (mapping) >>>> expected_count += thp_nr_pages(page) + page_has_private(page); >>>> >>>> @@ -2503,7 +2503,7 @@ static bool migrate_vma_check_page(struct page *page) >>>> * FIXME proper solution is to rework migration_entry_wait() so >>>> * it does not need to take a reference on page. >>>> */ >>> Note that I have posted a patch to fix this - see >>> https://lore.kernel.org/all/20211118020754.954425-1-apopple@xxxxxxxxxx/ This >>> looks ok for now assuming coherent pages can never be pinned. >>> >>> However that raises a question - what happens when something calls >>> get_user_pages() on a pfn pointing to a coherent device page? I can't see >>> anything in this series that prevents pinning of coherent device pages, so we >>> can't just assume they aren't pinned. >> I agree. I think we need to depend on your patch to go in first. >> >> I'm also wondering if we need to do something to prevent get_user_pages >> from pinning device pages. And by "pin", I think migrate_vma_check_page >> is not talking about FOLL_PIN, but any get_user_pages call. As far as I >> can tell, there should be nothing fundamentally wrong with pinning >> device pages for a short time. But I think we'll want to avoid >> FOLL_LONGTERM because that would affect our memory manager's ability to >> evict device memory. > 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. 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; >>>> > >
