References: <20230131063206.28820-1-Kuan-Ying.Lee@mediatek.com>
 <20230131151742.df43c3fd11bda01b613234ef@linux-foundation.org>
User-agent: mu4e 1.8.10; emacs 28.2
From:   Alistair Popple <apopple@nvidia.com>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        John Hubbard <jhubbard@nvidia.com>, chinwen.chang@mediatek.com,
        andrew.yang@mediatek.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org
Subject: Re: [PATCH] mm/gup: Add folio to list when folio_isolate_lru() succeed
Date:   Thu, 02 Feb 2023 17:09:02 +1100
In-reply-to: <20230131151742.df43c3fd11bda01b613234ef@linux-foundation.org>
Message-ID: <87a61wmtpq.fsf@nvidia.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?NVuhIST1PbW4XJIU0HJzxmcgnsS4EGOeOQlgvlwjQDoSTv74H/J68tvfLEdR?=
 =?us-ascii?Q?m3rWWYE+2a//AsDCQ8gn7qmgbr29K+EoXfaarALgq+cFA89+N4Dcqb3/qu7A?=
 =?us-ascii?Q?GtRk53Z2RJb0CTzIkBNLLCFwV3J5dejCDOl6dBymLQuV+/O6/VA+iikr1Kmt?=
 =?us-ascii?Q?FYI2cOcr+vOp2b5B0xkBaaduYfJZWXE7irVvRaetPUakc7pCcNGqhMVeS/II?=
 =?us-ascii?Q?rkB1VhwLHOieKfVWhwK5uhkrGrv+Ep0SxC1IQ5KzyXCRsq0PORdneTdH+0ma?=
 =?us-ascii?Q?zBY5ucgLXqnH4QpSOW62pZsyeRFzUdbhY2Gr3BAXxl2ehQoYhRJzlCyu+Hxh?=
 =?us-ascii?Q?Tw5EcbOTtK1JjWMGgjAcIdkUzQs0sho13RgAI3RwEJgNINbX11jNHjM0dMqE?=
 =?us-ascii?Q?BOj2RHxr8ZqnzgvlDeeh/e1mHsGXzwO9PeVgsjd/qVWwldwj3J3TN8FiGiPq?=
 =?us-ascii?Q?42sZEsQ/0ZEtx2nynWpy62GeidVDv5m31iwJG0QdTaoVDHHmLpU9DesKe0jI?=
 =?us-ascii?Q?2wp1N2+miGs8t4p31TiifcTmuQVaewRQsqdr202w4azOW9r7MZ1y3+hUaQDi?=
 =?us-ascii?Q?ETlTuLBIsCgr1puGjcEOEJa58a2joOoq3mWhsv8D4LIxQzXW2HOkLhulsI2V?=
 =?us-ascii?Q?5YZj0+ooFZro1MzsMP4up9nd6K1heCefNKU7HKarsB7aBnbCX97Z3lYpB1bD?=
 =?us-ascii?Q?lgWSpIREIbfSN+JNm07qWYMNLILatw59jYOXhEPT1y5IiXpYmaHa1/dmi9oY?=
 =?us-ascii?Q?X1Bd4n3oahRJKy/93xL8qdSYEc8NAvU9DIfLkz0nDaYT76fQa9B/PdgnXo+p?=
 =?us-ascii?Q?Qtz3vZndTHPKqSwWz9HdVOIcdWGGJ3OE4+h2EH6cXX10WMafsVKHQpFeiML4?=
 =?us-ascii?Q?1ctlX+BrtLX+X64EW3VOvjGmNc9TECN+1gzZPYxu5ZJ1vlm9Sj3P0CFvfURX?=
 =?us-ascii?Q?v8CPIbyAlHRdrPo1vscyEwBnntp4PIFH2TGUgHFv2eDBxsSJlcFSHLc4F5gJ?=
 =?us-ascii?Q?ierVfBWuOTduvPdTKsvsAv3i9go4rM0w48QGFOruO/CU0OWb9qhm+nVykHtZ?=
 =?us-ascii?Q?ghn7ES9Oo+LWwURh3PwJHdODp6EWlDrNpSqlAnCDJGpNDEj0kiEBpdIXWV+3?=
 =?us-ascii?Q?af6InV8FmQqXIHzout4zhroL13yAwQNDN73QiA83r0D6LApMiLfvSkighQw3?=
 =?us-ascii?Q?yTqsmsrS+eEXy15nuBanJgAuXkLGSlE4WKs+2CKBpR7LcoYKgs/ziVhsP3e3?=
 =?us-ascii?Q?eT1QVlGEiu9sE1OJNnt1v0f8EzKveym5HgxhXdgGoIOQmyAlMs3R0UhgbbZ6?=
 =?us-ascii?Q?yXzmkBOyFpxKCap0GvrmQQSDic+m+bmefVnWWcgy+DUWWlQhNDvqHJrVZKwY?=
 =?us-ascii?Q?Uui6hoaxN76J7lMqWPeLDWOPCeg6dvrEzuJfbpsBukeONWeR9DeRh0m1Q9pI?=
 =?us-ascii?Q?ldBPGeDX3ZNSek0SdlAdlpSSSn+/SZBvHEXfgarvFFZleq6IpmXz4PyUYY0F?=
 =?us-ascii?Q?xOvfLInCUELYSDR5XKkHoqDjpfSXII/eDFcnJReejfKnaNli5OE55xp+y6Eh?=
 =?us-ascii?Q?QUpGkKHy+MBJ1dmd3YiXysGffKOgqg3CRE50Zo0S?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 70680a7f-8772-4e99-5ebd-08db04ec1688
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2023 07:07:04.1917
 (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: NSsfzmj0Lqd77SQjmGVMknLLzvrfjILonPyT37/PWXBfS9XMZHcP733lnchvEEd72ej7nYyjsBbdbV0wdJd8RQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5107
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Andrew Morton <akpm@linux-foundation.org> writes:

> On Tue, 31 Jan 2023 14:32:06 +0800 Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com> wrote:
>
>> If we call folio_isolate_lru() successfully, we will get
>> return value 0. We need to add this folio to the
>> movable_pages_list.

Ugh, thanks for catching this:

Reviewed-by: Alistair Popple <apopple@nvidia.com>

>> Fixes: 67e139b02d99 ("mm/gup.c: refactor check_and_migrate_movable_pages()")
>> Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>
>>
>> ...
>>
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1914,7 +1914,7 @@ static unsigned long collect_longterm_unpinnable_pages(
>>  			drain_allow = false;
>>  		}
>>  
>> -		if (!folio_isolate_lru(folio))
>> +		if (folio_isolate_lru(folio))
>>  			continue;
>>  
>>  		list_add_tail(&folio->lru, movable_page_list);
>
> Thanks.  What are the user-visible effects of this bug?

In the common case none other than an extra loop through
collect_longterm_unpinnable_pages():

1. First call to collect_longterm_unpinnable_pages() will increment
   collected and isolate the page but not add it to movable_page_list.

2. migrate_longterm_unpinnable_pages() will return -EAGAIN and unpin all
   the pages but they will remain LRU isolated.

3. The next spin around __gup_longterm_locked() will re-pin the pages
   and re-call collect_longterm_unpinnable_pages(). As the page has
   already been isolated folio_isolate_lru() will return -EBUSY which
   will add the page to movable_page_list and complete processing as
   intended.

However this assumes the page table still points to the same page when
__get_user_pages_locked() is called the second time. That may not be the
case in which case we would leave the page LRU isolated forever
effectively leaving an unmovable page in a movable zone which is what we
were trying to avoid in the first place.

So I think Cc: stable is warranted.

 - Alistair