Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 203.18.50.13 as permitted sender) receiver=protection.outlook.com;
 client-ip=203.18.50.13; helo=mail.nvidia.com;
From:   Alistair Popple <apopple@nvidia.com>
To:     Felix Kuehling <felix.kuehling@amd.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>
Subject: Re: [PATCH v2 03/11] mm/gup: migrate PIN_LONGTERM dev coherent pages to system
Date:   Thu, 9 Dec 2021 21:53:12 +1100
Message-ID: <2497746.4npHOaMrmn@nvdebian>
In-Reply-To: <3f8a48d6-7ee7-fb30-5942-29054c34aac5@amd.com>
References: <20211206185251.20646-1-alex.sierra@amd.com> <2b996383-ebe6-e9d8-d794-58ecfd4a16e8@amd.com> <3f8a48d6-7ee7-fb30-5942-29054c34aac5@amd.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2021 10:53:20.8109
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 67640458-1c38-42ff-80f8-08d9bb021da8
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[203.18.50.13];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT063.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1228
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

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?

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