Subject: Re: [PATCH v2 03/11] mm/gup: migrate PIN_LONGTERM dev coherent pages
 to system
To:     Alistair Popple <apopple@nvidia.com>, akpm@linux-foundation.org,
        linux-mm@kvack.org, rcampbell@nvidia.com,
        linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
        "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>
Cc:     amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        jglisse@redhat.com, willy@infradead.org, jgg@nvidia.com, hch@lst.de
References: <20211206185251.20646-1-alex.sierra@amd.com>
 <b9163ccc-829e-9939-8177-a66ab41187e7@amd.com>
 <72fe6b48-4aa5-b766-3f33-8c3445fdcc99@amd.com> <2613033.KcdVtnzQgr@nvdebian>
From:   Felix Kuehling <felix.kuehling@amd.com>
Organization: AMD Inc.
Message-ID: <4ceda6e1-76c4-d92a-e4a3-a78d08620058@amd.com>
Date:   Fri, 10 Dec 2021 11:39:39 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <2613033.KcdVtnzQgr@nvdebian>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eSt3aWt0MDBIaVo0ZGUrQTVxejNDU3hHS05KRWNKaHJqRkwzYnROYXh4WmJZ?=
 =?utf-8?B?VWVLVDlWUUhnNFJqNlBKbXdXNjUrdSt1cUpkN2pQNVNxaTJOS3o0eW1FckN1?=
 =?utf-8?B?cksyNnd4NUxHZnBEQmdrZlFhbHljU0RwYS9Bcjk1VVIzMm5Oc20vUTBGbzMz?=
 =?utf-8?B?T0tJcWtkQTZJREo3Q0cvMGpROGlNQXNnd1ZuSTdkYkxlZjZqRkhPM0FhaVdV?=
 =?utf-8?B?TUk3TDhZYk9Ha2dvMDhOcUVwenFvSmpBWGNsUERNRlJ3R2lMOTQ4bi9jZnJU?=
 =?utf-8?B?WFNaeHVYd0I1ekZSTkpSZmhibktqMXJYakJvL2ZveWRFYkE1WXRSVGlKZmRa?=
 =?utf-8?B?N3Eyd2RyampCa1hObmp5RW9qeUtsMEZHYXcrVUtHMHE2MXFDZXQwRFdiMVFK?=
 =?utf-8?B?SnVWais0TnlpV3A2Zi8zRXB6OFMxc1VFZ0xIeGdMeTJuOTMyU1YyYS9GQnFB?=
 =?utf-8?B?T0dLZWVvSmJMLysrei9jUjFiK3k1NUw2WWJwMjNZdlIrY1pPaytBVkN1a2VS?=
 =?utf-8?B?QVVTMmxFZ2phQjc4cGNoTjhCSXViSkh0cU5LK0hWMlFyZ2tWMmZEcHpFZnc5?=
 =?utf-8?B?ais1S3A2bDE5THRLdjhIWVcza1EvMnROYWh4U3BOcTArenNLVVdDelpuNTlF?=
 =?utf-8?B?MVpDTDBjd2Jjd3hwZDJlbEl2Wjk0b09XR2lNdnpFMm5namovd2ZTNkk1UlJr?=
 =?utf-8?B?LzNFM1BNTU5hbkJIbjVvcnZHa3ZSSXoxZVRUeFNXSno0OHFHV3JwYnpoV1pZ?=
 =?utf-8?B?cXZMZnhDM1BudGFFZ2lGTEdZV3U3aUE0SWREZ3VnUERzdWJTTHl2ZzJOUklR?=
 =?utf-8?B?SVc4dHFiU3BCbGtpVTRjQ0lGd05ROFlFVHR2OUtmRHJYdnFHazFoSFRWMU5z?=
 =?utf-8?B?TW1aYXZiZWJ4bFJiTC8vZzlLYzhGUmwzeW1aMTdDK2RMWXdsWkpROS9iaGFa?=
 =?utf-8?B?aHN0NXVSR0pCdnBFcE13UWNyeEFudnNsZlhrZ1dYNE5mRk1pYkh6dnFsZ1BK?=
 =?utf-8?B?V3B3d0R3UFV6eXB6RG82S0srRDEyUGVCRG5WUXJ6RDFBQWNBT29oV0J4Z1Zw?=
 =?utf-8?B?N2hnb0NBME9wUU1nZFVRdE40d2NqRExtQ2xaOGw3YUJtTTg2aGdlc0V4WU1R?=
 =?utf-8?B?YW5VRlJGTXFyWmtHdUtxQnpsbW1VbzdFUEZsWmx4ZHR1QVI4UG5YK1MxNkd4?=
 =?utf-8?B?OWtjdWthb1BXN3hIdnRTVVN5azl1M0JCTzlQa0xjdWpUMjZqYWhuQ0p0UGVt?=
 =?utf-8?B?OWhwcW9wQ2F3YUpDNG9UemRsQ0dUTFliT2dkVUZuUTFFajJFNmc4T2Q5SnEy?=
 =?utf-8?B?S0syRE5QdnpGLzNsK29DQkdOY3kzeE5DZDRXZklneElxb24ydzQ4UXVMU2Mw?=
 =?utf-8?B?M0hxNEZtZU8wSnJRdWhYaDEvbVlYY1ZuaUx0L1BKWFczNElYU205Mm9qMHAv?=
 =?utf-8?B?WUVqOVllNkpFbnVRU0dEUGx2Q0RWaHp6Q0o3RU9ERDFmSnFOSlB1cjdJc0Fq?=
 =?utf-8?B?L3U4eDFzRkQ1b2pzdE94YSt6R0s5NGJmS3MyVHV6bzJONlo5TDZVdlJNN3lB?=
 =?utf-8?B?cTg0cmFoZUxtdDNCWHpzcFN0RlErZWtEQ0Q2M1lVVDI0VzdCU2RCQXorZkd2?=
 =?utf-8?B?bWJFNTZ5MC94OUVJY2hnblg5b0J1TFZIK0xraWp1cXdIMTNUUDJYeXFlRzlS?=
 =?utf-8?B?cG1xcnFxeDRNM3RoYkw5Z1lQcXpkdE5oQ0FkWjdKR3RJVnBINzFOVVRlQVNL?=
 =?utf-8?B?MVp4MWJ4TUhvUlcvR3Y0YnZ5Zmk1cC81M2xTZVF0M2FwL09FNHl5Z3NrVTg0?=
 =?utf-8?B?RHgxTFlqdWpBV1FrQ1k3WnJGcU9keG11enJPZ3NsV0hKY2E1bG5YZ0JYaGV6?=
 =?utf-8?B?aGV6Y3Q5Z1hCcGl0WnI3cEMrMU5GMUw5NXc0V0ZZU29ESG9qOUEyZ0g0dWQ0?=
 =?utf-8?B?ampOS0VMN2lUU3R2bTBlS0F4c0ZRaDdJcUViZm00Q2NiRElGeEZpRkFaSmsw?=
 =?utf-8?B?a3Z0bUU5MmdxT0VRdExyOE9JLzlmWGtOMzh4V0Q3RkZsZkV5cERyM1YxY25O?=
 =?utf-8?B?NXplRlliWW5oU2k4dlpWdE1rcjVTbGJHSUY0WW9VWkpnb2phZzJnSGtxRVBT?=
 =?utf-8?B?d3cxVnRWQ2hTT2pXMGpUbFFFZVhaQktSMkVsajYrYUdhanV6anN3VXR0K1R6?=
 =?utf-8?Q?5qul1p++2YUfFvW5TxaIs5g=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9c30a377-793c-4381-65fc-08d9bbfbaa3a
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5115.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Dec 2021 16:39:42.0110
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 110y5jQmWAJ84ApBYP47xMpIyscmjBxdmJNfwubyuXocXRI8xR0iwhAN4TB7NnyoSc1xOECGaBNdcfIsfNntuQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5196
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

On 2021-12-09 8:31 p.m., Alistair Popple wrote:
> On Friday, 10 December 2021 3:54:31 AM AEDT Sierra Guiza, Alejandro (Alex) wrote:
>> On 12/9/2021 10:29 AM, Felix Kuehling wrote:
>>> Am 2021-12-09 um 5:53 a.m. schrieb Alistair Popple:
>>>> On Thursday, 9 December 2021 5:55:26 AM AEDT Sierra Guiza, Alejandro (Alex) wrote:
>>>>> On 12/8/2021 11:30 AM, Felix Kuehling wrote:
>>>>>> Am 2021-12-08 um 11:58 a.m. schrieb Felix Kuehling:
>>>>>>> Am 2021-12-08 um 6:31 a.m. schrieb Alistair Popple:
>>>>>>>> On Tuesday, 7 December 2021 5:52:43 AM AEDT Alex Sierra wrote:
>>>>>>>>> Avoid long term pinning for Coherent device type pages. This could
>>>>>>>>> interfere with their own device memory manager.
>>>>>>>>> If caller tries to get user device coherent pages with PIN_LONGTERM flag
>>>>>>>>> set, those pages will be migrated back to system memory.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alex Sierra<alex.sierra@amd.com>
>>>>>>>>> ---
>>>>>>>>>     mm/gup.c | 32 ++++++++++++++++++++++++++++++--
>>>>>>>>>     1 file changed, 30 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/mm/gup.c b/mm/gup.c
>>>>>>>>> index 886d6148d3d0..1572eacf07f4 100644
>>>>>>>>> --- a/mm/gup.c
>>>>>>>>> +++ b/mm/gup.c
>>>>>>>>> @@ -1689,17 +1689,37 @@ struct page *get_dump_page(unsigned long addr)
>>>>>>>>>     #endif /* CONFIG_ELF_CORE */
>>>>>>>>>     
>>>>>>>>>     #ifdef CONFIG_MIGRATION
>>>>>>>>> +static int migrate_device_page(unsigned long address,
>>>>>>>>> +				struct page *page)
>>>>>>>>> +{
>>>>>>>>> +	struct vm_area_struct *vma = find_vma(current->mm, address);
>>>>>>>>> +	struct vm_fault vmf = {
>>>>>>>>> +		.vma = vma,
>>>>>>>>> +		.address = address & PAGE_MASK,
>>>>>>>>> +		.flags = FAULT_FLAG_USER,
>>>>>>>>> +		.pgoff = linear_page_index(vma, address),
>>>>>>>>> +		.gfp_mask = GFP_KERNEL,
>>>>>>>>> +		.page = page,
>>>>>>>>> +	};
>>>>>>>>> +	if (page->pgmap && page->pgmap->ops->migrate_to_ram)
>>>>>>>>> +		return page->pgmap->ops->migrate_to_ram(&vmf);
>>>>>>>> How does this synchronise against pgmap being released? As I understand things
>>>>>>>> at this point we're not holding a reference on either the page or pgmap, so
>>>>>>>> the page and therefore the pgmap may have been freed.
>>>>>>>>
>>>>>>>> I think a similar problem exists for device private fault handling as well and
>>>>>>>> it has been on my list of things to fix for a while. I think the solution is to
>>>>>>>> call try_get_page(), except it doesn't work with device pages due to the whole
>>>>>>>> refcount thing. That issue is blocking a fair bit of work now so I've started
>>>>>>>> looking into it.
>>>>>>> At least the page should have been pinned by the __get_user_pages_locked
>>>>>>> call in __gup_longterm_locked. That refcount is dropped in
>>>>>>> check_and_migrate_movable_pages when it returns 0 or an error.
>>>>>> Never mind. We unpin the pages first. Alex, would the migration work if
>>>>>> we unpinned them afterwards? Also, the normal CPU page fault code path
>>>>>> seems to make sure the page is locked (check in pfn_swap_entry_to_page)
>>>>>> before calling migrate_to_ram.
>>>> I don't think that's true. The check in pfn_swap_entry_to_page() is only for
>>>> migration entries:
>>>>
>>>> 	BUG_ON(is_migration_entry(entry) && !PageLocked(p));
>>>>
>>>> As this is coherent memory though why do we have to call into a device driver
>>>> to do the migration? Couldn't this all be done in the kernel?
>>> I think you're right. I hadn't thought of that mainly because I'm even
>>> less familiar with the non-device migration code. Alex, can you give
>>> that a try? As long as the driver still gets a page-free callback when
>>> the device page is freed, it should work.
> Yes, you should still get the page-free callback when the migration code drops
> the last page reference.
>
>> ACK.Will do
> There is currently not really any support for migrating device pages based on
> pfn. What I think is needed is something like migrate_pages(), but that API
> won't work for a couple of reasons - main one being that it relies on pages
> being LRU pages.
>
> I've been working on a series to implement an equivalent of migrate_pages() for
> device-private (and by extension device-coherent) pages. It might also be useful
> here so I will try and get it posted as an RFC next week.
If we want to make progress on this patch series in the shorter term, we 
could just fail get_user_pages with FOLL_LONGTERM for DEVICE_COHERENT 
pages. Then add the migration support when your patch series is ready.

Regards,
   Felix


>
>   - Alistair
>
>> Alex Sierra
>>
>>> Regards,
>>>     Felix
>>>
>>>
>>>>> No, you can not unpinned after migration. Due to the expected_count VS
>>>>> page_count condition at migrate_page_move_mapping, during migrate_page call.
>>>>>
>>>>> Regards,
>>>>> Alex Sierra
>>>>>
>>>>>> Regards,
>>>>>>      Felix
>>>>>>
>>>>>>
>
>