Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <a8da9ff8-b612-c5e1-54cd-a975f9075dae@nvidia.com>
Date:   Sun, 17 Oct 2021 23:47:26 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH] mm/rmap.c: Avoid double faults migrating device private
 pages
Content-Language: en-US
To:     Alistair Popple <apopple@nvidia.com>, linux-mm@kvack.org,
        akpm@linux-foundation.org
Cc:     linux-kernel@vger.kernel.org, rcampbell@nvidia.com,
        jglisse@redhat.com
References: <20211018045247.3128058-1-apopple@nvidia.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <20211018045247.3128058-1-apopple@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Wlh2eFVhNVZ6VkVDUlpYd1l6R0JBWG1GLzhOT2NtRGh4dHVYd3J5MTB6bkxE?=
 =?utf-8?B?Q05jbjdkSHVXYmZwMmFBUzlUMGRMVFlsLzVxUHRiNVVOTGJwS1ZPVDkxWWJD?=
 =?utf-8?B?MkxnSHRneTRwanNIc3pXc3BlTVRPU25rTENPSlYyY25kVlo3WDZ1eWtiOGtm?=
 =?utf-8?B?MWE3OHVkbDhrVlNETEZmYTl3MWNqaDJyMk56T3QwNW9QcTBCQWE4M3JHc2hj?=
 =?utf-8?B?YzYzd0VXUCtwUUcvT3hJZkRBSW1QOHlxb2ovYzk4Ni93S1ovdkZ2aGhJakFw?=
 =?utf-8?B?MVk0T1poY0lQdWtCdHgyMjhyQmliTnptZ25TemZZTmoydDBCZWM0dkR1d1VR?=
 =?utf-8?B?NldpdmpOSVlaZ0dmZEVaZ0xJOGx2U1U4VGgxOEZwZGJFbnBGRTlIR1k4cWtq?=
 =?utf-8?B?cDQ0REdOcGV0a2dJcmFRM0U4YnlvWUhGNTZUNlRVazM5akRvMzRpblNjemo5?=
 =?utf-8?B?dzgvOHJzNnRjUmR2YUdMYXpib2xtbXhBZ2ZsZ2RjMGRJeTFoYjJ0T0taa2lu?=
 =?utf-8?B?a1NGWW9VZHNQNGdJaFJTbTFueWVGZE1qWlVJTHhzZmF3eW9adjJmRitwMXZz?=
 =?utf-8?B?TGlPdkNuc2VUcG5ZMzQ3bXRZNUp0U0FJZlFncUIyZlVnWXhDMk5hWnRGSm9a?=
 =?utf-8?B?eTByMnhrZDd1L1JXL2ViNjlqUmpQVXhZT1VpaGVqeUFTcHBvSjh2TGNaclRj?=
 =?utf-8?B?M1NGRmxvQjZXdEhwSlNjdWJoUXBBVXAxWE1ldEd1ZE10UUllMFA2SFkzMjdy?=
 =?utf-8?B?N1hTcHhEVnRqYVZRTEJMNG9rdEI2ak9kWFBsWmdLOWQwQXZZdyt1bC9MSGZp?=
 =?utf-8?B?ZnZMa3JLdGpaZ2VyUit3WFR0TEsrL3o3VVpYTGNrV2Z4bjdYWVRRZmhaRjJ5?=
 =?utf-8?B?TGVoM0M1SWEwMHRlSkd5b2VNeWZQTG1yTngwaGQ4b08vUDVRcDA0ekJEOEdu?=
 =?utf-8?B?VjRqbFNuM2VSTDh5WjFzU1VldFNhNVdYajVrbkxoRWpBcWdyLzhaWVV3UjVu?=
 =?utf-8?B?UFI2S3ZoZGhyNnYzZExBaWZ4djdLZTJvYmZzTXJ6aTU5UlE1L1RLSnQzZmhm?=
 =?utf-8?B?clFFVjJPbUNGS21ZY1RVcWxVSTZJcTlrVDJwMG1BQnJHNDIvTFUvVDQzQmt1?=
 =?utf-8?B?UHV5WjNxL0h5Yk5tQUU5VXc3QklGeVZZdm5nMmp3OUZmV2hTenBSWnJ1QUNw?=
 =?utf-8?B?aG40d09IODZXVmkyR0VtKzRxRThUeUJmZzdBOHNUQzBvVWRGWmVxa2w4ZDMw?=
 =?utf-8?B?L2RrTlhUU2toNzhaKzVmaThTU0owbm4rcWZSU3JDbGIrSWZFL1NMbGc1dlJ0?=
 =?utf-8?B?RHdrak52M0J6a3hjajVnV2VoY2w4WGh5WDdNa1c0b2JHWlAwM2hmaXgycDFv?=
 =?utf-8?B?cFdoaStkQ0c3cURRYkFRaVkrZUtsMG5LYi9UeUlSem4wZG9TaWwrVzh5SkF1?=
 =?utf-8?B?Q2VkNUlZc2VReE4zZFZZcE8wckJ2RjR5eGZ4NEVmeElFeHNMcG5BZkJmbWk4?=
 =?utf-8?B?Mm05YmcvZE9EU1UreWgyemdpOFhKSHVBcStNRnoxR0xpc1RTcnpEaFRZTzQ3?=
 =?utf-8?B?azhsVlNKQ0lVU3A0RGhDNXU4UkJvUldReEhFaTN3LzBFMHdmbEJwMEhYZW5T?=
 =?utf-8?B?UmNHZFN3WmdDblI1WmRLdWduYnZYUWYwbU9GQ1pzK01rREZHZGF3enQwVFFq?=
 =?utf-8?B?YWVta0dKOFNtalNpQWQrSlNPWncvcmxLRzhnYnpOZTFqZlRuS1lVaVE4UXdQ?=
 =?utf-8?Q?s9FmgTpwGWZ31bZIDljK6b/qvoUox2rPsSrlmjh?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4ed4fd07-f4a6-4890-e487-08d9920327bc
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Oct 2021 06:47:29.9789
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /5HmALVW0O/++/wKwzk0KWfpNCWdIUQVtokpmuMGDpbeg7ODwq+Dg9+khREV8VBdwxT5/tNR9ELUbA2s6JQcPw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB2919
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/17/21 21:52, Alistair Popple wrote:
> During migration special page table entries are installed for each page
> being migrated. These entries store the pfn and associated permissions
> of ptes mapping the page being migarted.

s/migarted/migrated/

> 
> Device-private pages use special swap pte entries to distinguish
> read-only vs. writeable pages which the migration code checks when
> creating migration entries. Normally this follows a fast path in
> migrate_vma_collect_pmd() which correctly copies the permissions of
> device-private pages over to migration entries when migrating pages back
> to the CPU.
> 
> However the slow-path falls back to using try_to_migrate() which
> unconditionally creates read-only migration entries for device-private
> pages. This leads to unnecessary double faults on the CPU as the new
> pages are always mapped read-only even when they could be mapped
> writeable. Fix this by correctly copying device-private permissions in
> try_to_migrate_one().
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> Reported-by: Ralph Campbell <rcampbell@nvidia.com>
> ---
>   mm/rmap.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)


Looks very clearly correct to me.

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA

> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index b9eb5c12f3fe..271de8118cdd 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1804,6 +1804,7 @@ static bool try_to_migrate_one(struct page *page, struct vm_area_struct *vma,
>   		update_hiwater_rss(mm);
>   
>   		if (is_zone_device_page(page)) {
> +			unsigned long pfn = page_to_pfn(page);
>   			swp_entry_t entry;
>   			pte_t swp_pte;
>   
> @@ -1812,8 +1813,11 @@ static bool try_to_migrate_one(struct page *page, struct vm_area_struct *vma,
>   			 * pte. do_swap_page() will wait until the migration
>   			 * pte is removed and then restart fault handling.
>   			 */
> -			entry = make_readable_migration_entry(
> -							page_to_pfn(page));
> +			entry = pte_to_swp_entry(pteval);
> +			if (is_writable_device_private_entry(entry))
> +				entry = make_writable_migration_entry(pfn);
> +			else
> +				entry = make_readable_migration_entry(pfn);
>   			swp_pte = swp_entry_to_pte(entry);
>   
>   			/*
>