Date:   Mon, 22 Nov 2021 12:16:55 -0500
From:   Felix Kuehling <felix.kuehling@....com>
To:     Alistair Popple <apopple@...dia.com>, akpm@...ux-foundation.org,
        linux-mm@...ck.org, rcampbell@...dia.com,
        linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org,
        Alex Sierra <alex.sierra@....com>
Cc:     amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        jglisse@...hat.com, willy@...radead.org, jgg@...dia.com, hch@....de
Subject: Re: [PATCH v1 1/9] mm: add zone device coherent type memory support

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@nvidia.com/ 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;
>>>>
>
>

Powered by blists - more mailing lists