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, willy@infradead.org,
        jglisse@redhat.com, dri-devel@lists.freedesktop.org,
        jgg@nvidia.com, hch@lst.de
References: <20211206185251.20646-1-alex.sierra@amd.com>
 <2b996383-ebe6-e9d8-d794-58ecfd4a16e8@amd.com>
 <3f8a48d6-7ee7-fb30-5942-29054c34aac5@amd.com> <2497746.4npHOaMrmn@nvdebian>
From:   Felix Kuehling <felix.kuehling@amd.com>
Message-ID: <b9163ccc-829e-9939-8177-a66ab41187e7@amd.com>
Date:   Thu, 9 Dec 2021 11:29:55 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <2497746.4npHOaMrmn@nvdebian>
Content-Type: text/plain; charset=utf-8
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?VEt6U2F0YW5zZjQrNmI0VXdOMHpEdlB3VEJsSFBrdG9lcmZ0Um1yQmptMTlW?=
 =?utf-8?B?aUJ1aHRCYVROVno4WVVHOUFDWjdhWlRBSGlJd3YrMUdzYWc2R0lkcVpraFJm?=
 =?utf-8?B?NDg4Mmd0WDduWmw0SlRTQUxFTHlCb0dNS0FWaEdBclFqWmlMVkp4MlNpWTJv?=
 =?utf-8?B?YjBiMm5XNjRtT0laUVphU2xHMXRGNGxyYkxFZlYxMUw5ZllBV3BqWCs0L1Vi?=
 =?utf-8?B?NEZUblFFZVZES3ZCSXZvQTlaNDltUjJQNktWdnZvWVMyS3JObC9jcFQwL3g1?=
 =?utf-8?B?blFGVnpwcWtNYU0zSEdDREd1TGVIenljOFZId2Q1OFh2WkJQM2xKSkNTalUx?=
 =?utf-8?B?UDJvZE84OXlLL3FwNkpzTTJYSXFGQnNVQTE0L25tdHRhdEs0ZGVKdUlRVE5G?=
 =?utf-8?B?MGU4NEVwSU1ET0xiSmdNMERYaVQ2Rm1xTkxkbXdaSmFUVytwMDVQdzdYdEhP?=
 =?utf-8?B?Q0xqQnd3N1hHamJhZ0pzbzlJRkRTY0NCVVk1WEpaTDFBaWZ6djlXR2ZjTWhF?=
 =?utf-8?B?Wm0xMkR0QTRMVXFIV3o2YUJUKzM5KzhETU5ySXI5ZE9SSzNSQUNVc3ZncGdj?=
 =?utf-8?B?eWhGRHhWdEMxMUdnbDdmTUozREdJSVNmYW1wcXJ3STFqL3I0Qmp3cGhTYjZ3?=
 =?utf-8?B?cS90bWl3UGNjRVlicWtZTjFYblJ3ZHYybnAza0t3clAvRjluUFA5dldxZlZD?=
 =?utf-8?B?RTE4STNlaTVKd2JXZ081eW1MQ1lFZEdlZjRJWXVsS290QURhb1c4c1h6NVp4?=
 =?utf-8?B?V0x2R3IvRVdseXlXYkNwMFJteHJ6R043bHpDdy9WM3loZkYvWVZuRDMzTTVY?=
 =?utf-8?B?MmJYRE5YR21QaVorYjBjZVBFU1JNaitIR29JcHNwenZ1VStXaGpSZWU2M0Mv?=
 =?utf-8?B?TWh5VjVOTDZraXZYZ1V5WDBlWDQ5Z0JSMjdWZE05ajh6OVY1YmcrZ29MckJT?=
 =?utf-8?B?YlpUSVNpdWJGdjFYakpScStQaWowNFhoaStwSTdhVStqUUVxRDJwS0JrZEhk?=
 =?utf-8?B?anJxYWZJS09SbUwrNEVSalJ0TEdZVnlyV05ldi9PT1BFNHhVUHZlZmRrSlcv?=
 =?utf-8?B?dzEzS3VqNUo4UG9Qd2lPb3lnNXZxMUl6R0lBOEpnMEJ6WHI0WXBZWHU3c3RZ?=
 =?utf-8?B?OFErckdNVjZEZkFIcUp5dmtuKzdIcDlhL0hYZTZRbmx1eFFMV1ArcnI0T2JN?=
 =?utf-8?B?dkFZWHZ2Ky9vWGI1OTJZVkVIRzNoVmdWK3NtMDBYSmZpVjdZQlRYVWNyRTM3?=
 =?utf-8?B?QWYvTGFYaERGVHVCd2NkNUo2NlhxUndBcGtTbE45aUVpQndUakUyM2QwdTV1?=
 =?utf-8?B?aXB2MUUzckZ3VmJLVEFHZ0s1UG1NOENSQ2hHYVhJV3hOSjVSdVlFbGhudzly?=
 =?utf-8?B?WTFDTVlwbjdReDBoWm0vbERpdFMvWkVMRFIyMmk2Wi9ocXRRWTF3VDNEd2JY?=
 =?utf-8?B?UXBQMmJKbmk1NzVQV0lLZ2xSL2U3ZXhRT0NVbXlpR2JWbmkrR253UmZiUC9H?=
 =?utf-8?B?NTE2QndGYXcvVlRheGlzdit4SjRZeVhGRnJFZFg3Q2RSZkgyaE91MG9wYktI?=
 =?utf-8?B?eHZsWGdwbUw2ckxhSWlmVmVzSnNrUnFMNVZlejFTU1Y5VmNVY1R0NlZFTng1?=
 =?utf-8?B?WWxIN205YXdrdHV0ZEdSZlU2alR0cVJjR3c1a21kOE5nanNjd2wyMXlFUWRl?=
 =?utf-8?B?enVvNUo0dE1MNGFVaDI3MVM3NUdOS2Fxa2s3YjZCcTUvV0ZjakdPOFlONXpv?=
 =?utf-8?B?QmZHaUxQeGY1bjRZTXI2aGM1VDJRd2hsYnlleVRCanZmVnRJZXI1aVk5S01U?=
 =?utf-8?B?NXhKSnVFa0JNVEt2UU9ubTRQUE5JRHI3UUVjTEVKcSs3S1haR3VYc0t5cTVi?=
 =?utf-8?B?NmNWaWErMEh4WVl6d25FaC9VN0pGZkdzSW5BdHErNXdWazJNR3AvOHY3OE5S?=
 =?utf-8?B?RkZ6NkFhb2lORVh0U0Zwb3NLMXdXMVE1ZEo5Rk5DdS9SN3NhWUhGMDdadUVB?=
 =?utf-8?B?RUZDS3B0dmcwNFpOOVNCOXFPWm41cVZJRUxxUkJFZk1iaDVjQ0sxRDJ1K1dO?=
 =?utf-8?B?MmhuakkwODdBVTUzRGk2aFp2MllJTUt0aE1aRC9iZGNheDQ3U2s4a09tQkIr?=
 =?utf-8?B?QmUxc0dyK0ZhU1E1anR6bm51Vi9zOTUwSitreDVPRWhaNWhpNmlCSFpVNnpO?=
 =?utf-8?Q?CosaqJvkK41WNeKsGR94VcQ=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9576076e-7b12-4803-0bd5-08d9bb312386
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5115.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2021 16:29:57.4858
 (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: mDIbPRNKTM5PNMH2bBTRHdTr0trcorGFcQ9GPyI0Zhz75695PAI90phsXiwzL4tgLkZyS2vDd8qGqaaZKurQ8Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5113
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

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.

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
>>>
>>>
>
>