Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
From:   Alistair Popple <apopple@nvidia.com>
To:     <akpm@linux-foundation.org>, <linux-mm@kvack.org>,
        <rcampbell@nvidia.com>, <linux-ext4@vger.kernel.org>,
        <linux-xfs@vger.kernel.org>, Alex Sierra <alex.sierra@amd.com>,
        Felix Kuehling <felix.kuehling@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>
Subject: Re: [PATCH v1 1/9] mm: add zone device coherent type memory support
Date:   Tue, 23 Nov 2021 18:10:05 +1100
Message-ID: <3642425.Elj8dHrGez@nvdebian>
In-Reply-To: <f45fb007-ccc4-4d09-b609-09faa81d3b81@amd.com>
References: <20211115193026.27568-1-alex.sierra@amd.com> <4157439.BacSOyMZPU@nvdebian> <f45fb007-ccc4-4d09-b609-09faa81d3b81@amd.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Nov 2021 07:10:14.3885
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: d7b75513-7706-40d2-2c5a-08d9ae504bfb
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT035.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR12MB4491
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

On Tuesday, 23 November 2021 4:16:55 AM AEDT Felix Kuehling wrote:

[...]

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

Yes, I think you're right. I was thinking check_and_migrate_movable_pages()
would work for coherent device pages. Now I see it won't because it assumes
they are lru pages and it tries to isolate them which will never succeed
because device pages aren't on a lru.

I think migrating them is the right thing to do for FOLL_LONGTERM though.

 - Alistair

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