Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8f1f3278-ce9e-e328-6a8b-a793bc7f950f@nvidia.com>
Date:   Fri, 29 Jul 2022 14:22:03 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] mm/gup.c: Simplify and fix
 check_and_migrate_movable_pages() return codes
Content-Language: en-US
To:     Jason Gunthorpe <jgg@nvidia.com>,
        Alistair Popple <apopple@nvidia.com>
Cc:     akpm@linux-foundation.org, minchan@kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, pasha.tatashin@soleen.com
References: <20220729024645.764366-1-apopple@nvidia.com>
 <YuQ4mJqjIUncf7iF@nvidia.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <YuQ4mJqjIUncf7iF@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?ak9NMUNmaktFWDFyaTJ6cENXeHVmMWVsRE1HZXg5MVl3czBGNCtuSlpoSk1D?=
 =?utf-8?B?K3A4TzRXRHlWQlk1VU1qZkRhNVpNTm9vL2t2bkNoVHZUNEhCQW8zKzJPZENx?=
 =?utf-8?B?c1ZGejhWaXFtZmJ5a3ltSkdFS05Dc3NCeHBlK3NoRjBwQXNKZ2RqV2srYlJj?=
 =?utf-8?B?ek5ZUDVqc2lkbHQxU1hqOUlWeHRTdDYvcC8yMW1wdzFYb21waGFVRk54Q0ox?=
 =?utf-8?B?WktISU5zQU9tZzlNR3Z1bDZVaHdyNTNpaXZsUUMyV0dXVHY3ZWxBalh4Y09m?=
 =?utf-8?B?TWluR1ZRZ1pGNUFYcmk1T3RXQlZ4ZTdQRkNJQjBxdGZ2Q24zd1Z3MGh4YkRX?=
 =?utf-8?B?bTR3cFRpNFduaXBwSm12alcxY0daU0F1eUNGWmkwY1hEeVY2VnFYaXQrcnlr?=
 =?utf-8?B?d2liWXl3RlJqTnpRWWM5Q0xwL29EblJRSlV5c1M2TWxtOWwrTWMyYzhRcW1q?=
 =?utf-8?B?ekdVMEZZRlJkTzl3Szg4bHQ0a3NuN1M0VkYwU2ZsWW5JM2lyTXJ0UE8zaXkw?=
 =?utf-8?B?dm5rczZHbGE3RUs5blpab3BpV3cxSzhsQndHencwNXhKMHd5b0ZnRVF0VC8z?=
 =?utf-8?B?ODVPQzM5ZGI2VERaT0VKRHlJbVFtUGx1K2t5cUJEWW01UEdicmJVajg5YkFY?=
 =?utf-8?B?YkRMQWYzTE9Ec0o1aHZFNGlXblpySnpRbGcxMzRRRVlsMzVnRWRYbHBZWmZi?=
 =?utf-8?B?M3M5cUNNblRZdmxxTGlNbXExeGh3WjVERXl5U2FyY2NwT1g4UllUdVMwaDNw?=
 =?utf-8?B?ZG9SdnBLdW5ZajZIejA4b3F5VGYyQnpQZG1scmtQc0cwLzdiaXRjRjArODkw?=
 =?utf-8?B?dlU1dCtINUlqc3JWUkEwQlE1VjZjTDhCTHgyRjJzWGJFZzM3SHRHQ2tXK1ow?=
 =?utf-8?B?b2szSld0ejlsNFRLR2FrL1NmZi9oRzQ3ek53bWFMeUNKaXk4Y0FuSGR0cGlU?=
 =?utf-8?B?bi9nK0lEZVpDUm9HVVlwUDJMSFU1L2hGYjFIKzRra2hSUUswRWVSNlRoR1Q5?=
 =?utf-8?B?ODdTNk1ETDFCdDVPazRlbVJ2MUEzdzFUKzg5eUtqY0NtOGlXZWtzOEE0aUlt?=
 =?utf-8?B?THdpbldENGoxOVRhYVBEMnBhdDdTRE1Fd2hKS1k3U3hCNnVPc0k1YTc4NkdO?=
 =?utf-8?B?UUgzTjlkK3RJT3lndlNxWkxYVkRWWDRrVlliRjJ6YVhjODI0UW9KNFV5Rkg4?=
 =?utf-8?B?RnRjY25ocUFhK0RCdUxvTVVLYzgySnUzRHdRRVJrUEVIcUpMT09LMmxBSS9m?=
 =?utf-8?B?WTN2VHZ1MjV4SnZhVWc2NVFXSGtVTHcvU2xyVitHUTFic2VvMHZRd3NRU0wy?=
 =?utf-8?B?ZENnenByZVBLWU9Xc3JIYzdEbFFIaU5Zbk4zN2syUVdEbjJUUVNMVFQ2Y0Na?=
 =?utf-8?B?Y21MbDZzaERCODNrcUE2bTQ2cGkvMldjdXdyazZnTDcrZUFBenAreXJ4NjF3?=
 =?utf-8?B?cEJQTGd4aVdLZThSb3pxTjYwK1VSWGEvR1ExaXA4QnRXeDdoYThRWllEcjUv?=
 =?utf-8?B?aGxjYUxjUDlLNktKZWRGZWtCajJLWXpPNUNjcytWc01TN3pMRkVaZTVVMlRs?=
 =?utf-8?B?dk1CSEZsU1lHdllmTWZxVWg3QllGaytaTzMxaUh4RWlaWk9PTktoUkIvQWtT?=
 =?utf-8?B?UjkvamNYc3hDdUQvblhIOTVUcTN3dDBIQWhyN2ozSVJpRlRueTBQZkMvUzky?=
 =?utf-8?B?VGdoekg5VkY1K3hUc0wvNEluQTNLZERNMFJqaVNKMWZ3T1pMTEZXWGFQVHBu?=
 =?utf-8?B?MW52Q0lCU2FqNWRML1l0V0d3QzZrY0UrY25LNUhHdnYybWdFeTY2bkpKSzJW?=
 =?utf-8?B?cFg3bGVUWUVmL3NXd3hvRlpHSkorbWUxNVE1OVRyTGhSTFZPejhQQXBvVDNo?=
 =?utf-8?B?S0VtOUxqbW9JWlFmeUlMaGgyTnJIdXVGS2QxVk5ZQkhCcDVDMzVFaW9xcEk0?=
 =?utf-8?B?VEZoUmk5VFV6WjdBK2VIWGdHR3poZXh0a2FhRmJRdzhiMEIzN3hBQjNHeXZp?=
 =?utf-8?B?SGplR1pwRFlTVkFjU3JZLzhOejRzT09GZFEwNER6Tk43TjlsYUtlTDBMZzM0?=
 =?utf-8?B?TEw3UUh5czJRQVRhc1N5RktJMWlpOXFmUHhqcFkxTGdmM0tkRUxrVWc1aHF0?=
 =?utf-8?Q?Ssddk4K+8s5EyHaIz2H5Q6juo?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 22da4c70-559d-417f-131b-08da71a862fb
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jul 2022 21:22:05.6037
 (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: T7BQXN33S3AokZj67CWNVVbqP43NJtbhyqm0YQiZ21W8w4Vd0DBmiHsrws4Sw9S02+cJJf1n15GJoVqnyxMOGg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB3910
X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 7/29/22 12:44, Jason Gunthorpe wrote:

> I came up with this ontop:
> 
That cleans it up even more, looks nice. I'd go just a touch further,
and also (unless there is some odd reason?) stay with -EAGAIN rather
than -EBUSY, because otherwise both the function's comment header, and
the caller, should change from -EBUSY to -EAGAIN just for consistency.

And also because the way it's used: the caller is literally "trying
again".

So on top of the ontop:

diff --git a/mm/gup.c b/mm/gup.c
index 43c1fc532842..5f04033ee0ed 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1901,10 +1901,12 @@ struct page *get_dump_page(unsigned long addr)

  #ifdef CONFIG_MIGRATION
  /*
- * Check whether all pages are pinnable. If some pages are not pinnable migrate
- * them and unpin all the pages. Returns -EAGAIN if pages were unpinned or zero
- * if all pages are pinnable and in the right zone. Other errors indicate
- * migration failure.
+ * Check whether all pages are pinnable. If some pages are not pinnable, migrate
+ * them and unpin all the pages.
+ * Return values:
+ *       0:      all pages are already pinnable and in the right zone
+ *      -EAGAIN: some pages were unpinned or were zero
+ *      -ENOMEM: migration of some pages failed
   */
  static long check_and_migrate_movable_pages(unsigned long nr_pages,
  					    struct page **pages,
@@ -1914,11 +1916,11 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
  		.nid = NUMA_NO_NODE,
  		.gfp_mask = GFP_USER | __GFP_NOWARN,
  	};
-	unsigned long i;
+	unsigned long i, not_migrated;
  	struct folio *prev_folio = NULL;
  	LIST_HEAD(movable_page_list);
  	bool drain_allow = true, coherent_pages = false;
-	int ret = -EBUSY;
+	int ret = -EAGAIN;

  	for (i = 0; i < nr_pages; i++) {
  		struct folio *folio = page_folio(pages[i]);
@@ -1990,7 +1992,7 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,

  	/*
  	 * Unpin all pages. If device coherent pages were found
-	 * migrate_deivce_coherent_page() will have dropped the pin and set
+	 * migrate_device_coherent_page() will have dropped the pin and set
  	 * pages[i] == NULL.
  	 */
  	for (i = 0; i < nr_pages; i++) {


thanks,
-- 
John Hubbard
NVIDIA