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>, <willy@infradead.org>,
        <jglisse@redhat.com>, <dri-devel@lists.freedesktop.org>,
        <jgg@nvidia.com>, <hch@lst.de>
Subject: Re: [PATCH v1 1/9] mm: add zone device coherent type memory support
Date:   Mon, 22 Nov 2021 13:40:40 +1100
Message-ID: <4157439.BacSOyMZPU@nvdebian>
In-Reply-To: <637b19c0-5ec4-b96b-f6f6-c17313f03762@amd.com>
References: <20211115193026.27568-1-alex.sierra@amd.com> <1997502.P62F5Z1OZk@nvdebian> <637b19c0-5ec4-b96b-f6f6-c17313f03762@amd.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Nov 2021 02:40:46.5987
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 833acfce-1089-43ca-2fec-08d9ad617ce9
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: CO1NAM11FT040.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4482
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

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

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