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: <dc8fd102-ba30-d980-bdbb-11f39326103d@nvidia.com>
Date:   Fri, 12 Aug 2022 00:05:18 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v4 2/2] mm/gup.c: Refactor
 check_and_migrate_movable_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,
        "Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>,
        Chaitanya Kulkarni <kch@nvidia.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Felix Kuehling <Felix.Kuehling@amd.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Logan Gunthorpe <logang@deltatee.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        David Hildenbrand <david@redhat.com>,
        Matthew Wilcox <willy@infradead.org>
References: <487960bf67c7273ff5606c76f73bb51271bc7b90.1660269441.git-series.apopple@nvidia.com>
 <6b61e9bf7c65f78524db32ba3e65a7eb6b8a76a0.1660269441.git-series.apopple@nvidia.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <6b61e9bf7c65f78524db32ba3e65a7eb6b8a76a0.1660269441.git-series.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?eDc2ZmNXUDlGQXRVeXpLSWtQR2s3MGRrTVhEcWVrbTZyalJzSEhEK0UySVZD?=
 =?utf-8?B?Z0ZZQkE1eHNzQlUrbXQybmk3SXVDS2ZlS2IxakxaUExDQVk1Wnd6eUgvSVlk?=
 =?utf-8?B?ZUNUNWhmSGlzL1VEcCtrQm5mZ2VtcDVYTkUrMHRIOUdvUmhUZFg5Q0U1a241?=
 =?utf-8?B?TFBSMVZRSnZ5YTlmZE41bmdZSXN6czRxd25hZ3RRVXpMMUpUVCtZQUpWQWhP?=
 =?utf-8?B?ZmpUVHNEN3ZQSmdoRW5rVjUvd0hvdnA0SGptbXZiQzkzUUtDTko0ejJxNTdD?=
 =?utf-8?B?ckNZeU9ackp5RzNsZ0tJTURpYUxrdW1kL1d1UU9FRFNST3lybFpzMExRZXdQ?=
 =?utf-8?B?eW82WjMyT2dsZUtzaW5vaVYyL2NzaDJBaytUdWI5SFMxcWZUaEswNlNDeStQ?=
 =?utf-8?B?TjJnN0tpcGJWTUYxNndPVllOWXQyZ2tvckI0VnV5YTdxRjZ1NDcvUFdscGpY?=
 =?utf-8?B?bG9XMVRTclpQYWdmR3I1RWIxQTgrTWUrWDBkSmtTdk85K2VzYmdYeU56QVl5?=
 =?utf-8?B?ZmhhU05ONEE1MHVaV3RsdkZicU9Lb2dWbm1GUlQ5NUl0ZDhINS9SQVM2U2py?=
 =?utf-8?B?d0JtZDJwcTlvWU5BMVl5WVBnRGhPNzZYUXJuVWkzTjI3OW5rREhzTVdMRnM2?=
 =?utf-8?B?QmlxSFdRZ0hkV2x3bU1hSGFqM3FNZTlCeHc4cEUwNm9OWmx0ejh6aDRsY0NO?=
 =?utf-8?B?azlQWHRYbGhrTWd0ekVybkROQk1Cc3VhZWVBL3htNkVHWWdQdjBwbW5XY2Rt?=
 =?utf-8?B?T1JYSUtWUWNNTUZ1eTR6b2FrTzRVUGNXajRucmV4UTlQZUlCMENjRnJJcjhQ?=
 =?utf-8?B?UExpRGpRTmVaWlByYUVtcVBUMFY5bEdRS1ludkF0WHVNbDk3OTVZdlVDYzBj?=
 =?utf-8?B?dHJGZExnVDkxZEt1REs2Y2dzQWlBUmFaSlFLWGQxbnNlZm1UVEJpeENPeFRS?=
 =?utf-8?B?REY0eWtva2J0WFJmaCt4UVh4TkJ6SHZHYk9Ub0IzYnd6d3U3UVBsYlo3a091?=
 =?utf-8?B?bTRUWnFCcFNQYW90a1RweVJRc1BsMTFqeW9ua3NWSEVEUUFuTHFLS2UrZ1pn?=
 =?utf-8?B?ZnYvaWZLeGtvempYWEdtWUdGaDRBNVduODg1cTJkRVR5WVY4TnBvTEd6SXpa?=
 =?utf-8?B?V2RhYTB0eHdFWnFNb0hoYWNjWTEzaG02ME9YWDVGYzlpNEllZXZDQVllVUpt?=
 =?utf-8?B?VFdYaEYvUWlWQzB2bjdOWDNmdUUrekYyQ3JhL3ZleUcyamNVUmNKVXJ3OHpS?=
 =?utf-8?B?cWVhV0hQb0N4Z3N0bU54Z2t4ODcyYjFqTCtHaVNYNnJRSnBJTlhQYndLTStw?=
 =?utf-8?B?a29LT2NnTk45Z0lMNm4yS1hZVHkyVFZTbFl4OHFGSkFlSUt6Q0FlelhNeTRI?=
 =?utf-8?B?Z2p0VWJ4U01CRjdwclRCSlFUaGJZQ01uOFVKUUV2U2NzejR6T3VaZzdNUExT?=
 =?utf-8?B?aE5HcnpiVm9sbmdnQ2kxMW9vdjZPZ1hTZGUyQ2NsSmRFRWhBOVBndm0xZ3dD?=
 =?utf-8?B?NVdKeGlzL2l3SFNkb2RPUDhvRC9zeWlzcnZQS0JIYW5WRnFwRk43eVJpSHUy?=
 =?utf-8?B?ZDZLamNyWUNjSWY2WDZvOWhEQ1ExTm9MNVVlNHlCWGtwZ0JkWkg5SWpkaFdQ?=
 =?utf-8?B?RlNYYVNZMVFQZGNwSkdaWE5JRGdrU29qZlFQQm1qNFp2NkRpSVUzbXVvUU5I?=
 =?utf-8?B?bnorb0NlZitQUzVmdVpWMWRuNjZnZTBtMXJMbGFhcDRTbVBudG9OeWU4L0Qv?=
 =?utf-8?B?WElCek9EMktTRE5TNW02bzJucDNkUmVCRmhvcVM5eVVXdG0wZVJEOFpOV2JN?=
 =?utf-8?B?eFVEenQrVk1oRERYdlZ5bkhTd0p6bjB4MGNVZEl4Wlo2N1BhYUZoRlBJWEJw?=
 =?utf-8?B?dnRnNDZSaVpXSnAxYzhDWEcrYVdWVDB3RFliRVd1MDJ3YXFhZG5rRjNwTUNO?=
 =?utf-8?B?dllBSjU2YWlGQXQwblYvWSt4U0JCQ2FaQ0VYWTdsL1ZtZzlsaGRWMEYzZS9z?=
 =?utf-8?B?MTRjdmRtVzg3bVBoYUdUbFhZaGh2TytzTzZjTzc4aStZOWwxeERhQTdzMXJW?=
 =?utf-8?B?dkNwTVFnMXRMVmV6eUpidHZBTHpCL3UyS2FRZFJienhudld4ZEFMeTN2ZUw2?=
 =?utf-8?Q?dmjo+WT5V9/G8Ttu13UCMOBli?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6091eca9-aa68-43bd-cb68-08da7c31053c
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Aug 2022 07:05:21.0707
 (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: 7kScs9fBlbR7UDf/85S8rzvtpPW2fqZqW8HuJJ+v2lu62pILymi+YfKgtwn++S45Ud6xa8dXeLJXtmcIk8/OGw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB6295
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,T_SCC_BODY_TEXT_LINE 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 8/11/22 19:13, Alistair Popple wrote:
> When pinning pages with FOLL_LONGTERM check_and_migrate_movable_pages()
> is called to migrate pages out of zones which should not contain any
> longterm pinned pages.
> 
> When migration succeeds all pages will have been unpinned so pinning
> needs to be retried. Migration can also fail, in which case the pages
> will also have been unpinned but the operation should not be retried. If
> all pages are in the correct zone nothing will be unpinned and no retry
> is required.
> 
> The logic in check_and_migrate_movable_pages() tracks unnecessary state
> and the return codes for each case are difficult to follow. Refactor the
> code to clean this up. No behaviour change is intended.
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>

OK, I've finally convinced myself that this is a correct transformation.
This cleanup does help clarify things, definitely.

I've got two documentation additions (and changes) to suggest, below, and a
couple of too-long lines, but the code itself looks good, so with those
tweaks or something approximating them, please feel free to add:

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

...

> +/*
> + * 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.
> + */

Instead of the above, I'd like to suggest this:

/*
  * Check whether all pages are *allowed* to be pinned. Rather confusingly, all
  * pages in the range are required to be pinned via FOLL_PIN, before calling
  * this routine.
  *
  * If any pages in the range are not allowed to be pinned, then this routine
  * will migrate those pages away, unpin all the pages in the range and return
  * -EAGAIN. The caller should re-pin the entire range with FOLL_PIN and then
  * call this routine again.
  *
  * If an error other than -EAGAIN occurs, this indicates a migration failure.
  * The caller should give up, and propagate the error back up the call stack.
  *
  * If everything is OK and all pages in the range are allowed to be pinned, then
  * this routine leaves all pages pinned and returns zero for success.
  */

> +static long check_and_migrate_movable_pages(unsigned long nr_pages,
> +					    struct page **pages)
> +{
> +	int ret;
> +	unsigned long collected;
> +	LIST_HEAD(movable_page_list);
> +
> +	collected = collect_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);

There is no reason to exceed 80 cols here.

> +	if (!collected)
> +		return 0;
> +
> +	ret = migrate_longterm_unpinnable_pages(&movable_page_list, nr_pages, pages);

Nor here.

...

> @@ -2051,10 +2079,10 @@ static long __gup_longterm_locked(struct mm_struct *mm,
>   			break;

...and in this routine, let's fortify the comment like so:

@@ -2068,7 +2078,15 @@ static long __gup_longterm_locked(struct mm_struct *mm,
  	if (!(gup_flags & FOLL_LONGTERM))
  		return __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
  					       NULL, gup_flags);
-	/* check_and_migrate_movable_pages() assumes pages have been pinned. */
+	/*
+	 * If we get to this point then FOLL_LONGTERM is set. And FOLL_LONGTERM
+	 * implies FOLL_PIN (although the reverse is not true). And that, in
+	 * turn, makes it correct to unconditionally call
+	 * check_and_migrate_movable_pages(), which assumes pages have been
+	 * pinned via FOLL_PIN.
+	 *
+	 * Enforce the above reasoning, by asserting that FOLL_PIN is set:
+	 */
  	if (WARN_ON(!(gup_flags & FOLL_PIN)))
  		return -EINVAL;
  	flags = memalloc_pin_save();


...and with that, it's actually possible for the reader to work their way
through this story, I think.


thanks,
-- 
John Hubbard
NVIDIA