Received-SPF: pass (google.com: domain of linux-kernel+bounces-100662-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Zi Yan <ziy@nvidia.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
 Yang Shi <shy828301@gmail.com>, Huang Ying <ying.huang@intel.com>,
 "\"Kirill A . Shutemov\"" <kirill.shutemov@linux.intel.com>,
 Ryan Roberts <ryan.roberts@arm.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mm/migrate: put dest folio on deferred split list if
 source was there.
Date: Tue, 12 Mar 2024 14:32:43 -0400
Message-ID: <76BBDB5D-9D40-44EB-A996-767404B85BE0@nvidia.com>
In-Reply-To: <ZfCFCGkrLVpySa6X@casper.infradead.org>
References: <20240311195848.135067-1-zi.yan@sent.com>
 <Ze_P6xagdTbcu1Kz@casper.infradead.org>
 <74AF044A-A14A-4C66-A019-70F8F138A9AB@nvidia.com>
 <ZfBkjf4z-jpaNk6c@casper.infradead.org>
 <DFA2B8B7-C690-422A-BE95-82F7E112BB95@nvidia.com>
 <ZfCFCGkrLVpySa6X@casper.infradead.org>
Content-Type: multipart/signed;
 boundary="=_MailMate_9FA95A4E-3296-4A51-8878-D2196FA15765_=";
 micalg=pgp-sha512; protocol="application/pgp-signature"
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?O59RucQhybpobHOwOj9T4P6VVxJATPnml0U3IbpurROjwIn2rV9LsQNlzNd/?=
 =?us-ascii?Q?QeFQDyw+NQOFSsJhuVtuGZ+mwCYY2auqv1Qp3fGIhwkVwv3KPVzDJXANbtc1?=
 =?us-ascii?Q?9gsIrplPVDN39nCFkpDzrGwtmf61xRc1WkjRTZLgfmJvMJRgW2RdPLkIXtrr?=
 =?us-ascii?Q?t+m+OjAoRbZmidCxvOrFDvhgU6AiH8dpHZ3ZKXCTUBRKgotcrBMcPG5y4EJC?=
 =?us-ascii?Q?SusS1Y0ph4/8hBU3HZ2qXSTu+IsCQzCpFnfiFC+oNdRbttquITdO0HMvl0i6?=
 =?us-ascii?Q?C/YQeN1HZMkTrHbKNshw6RippSJkgCvJmpoRI42xdLPjQmDk2emTtunMkKdC?=
 =?us-ascii?Q?XR47XQdHIrbyK7Q4iRm4LNEI5Zo66QEiL0veO7ctCVRiYUer2eT9H0ZM1w38?=
 =?us-ascii?Q?kTAmUNOdNmvDYT1eqMLDV3H98iyvMtY49QqhKqY/IQrZgAVLeMR+ltnfatoL?=
 =?us-ascii?Q?liCX3Hynn/sFWt+D2Zalym47XvgjEnp+2rJ4TvwQdOgkVMLViLhuD+pUZTIp?=
 =?us-ascii?Q?k8L/9hGqNXC04+Mqsml2p7r6jc6IrC+vBjlDpMbiAXOUoh+znDHm8iPXLAlC?=
 =?us-ascii?Q?5y3t/p9XDt3r99vspUKh5EC7BTI8JxA3PZYk8gHPJzlZnHYADvcJQDIQJXeM?=
 =?us-ascii?Q?RUh4D3f0cvqcTqt5yyILZWPGlLkYx43PvZLVo8gaxlz1KEhDKsHIGjbEQ/f3?=
 =?us-ascii?Q?L0Uy8XHzauU87b2cRk4WdSQ+ylMjHTMurFFvZjlb3gsX+Ww6V8kiD53mW154?=
 =?us-ascii?Q?mhDbi3GiYQUmQR1pRE1uUv+HRYSa7GDUKZ5VfFCE/mZ2Hw3uwFUvHZxP+U5C?=
 =?us-ascii?Q?IzBnDENu+w82cCEi8cyEDfflSu9IsGaTFCSVgB5bJoMLbRPqPlnpnp+Hfr6b?=
 =?us-ascii?Q?Gm7RBpS8cgOVgkPGn5c0rM+QiISEXg/vpsMN0qyGoHuIgiZOgUM69DtOg7hl?=
 =?us-ascii?Q?wAhLnhlI5UQbGo+MyK/Is7sglGbR4tHgkFvtSFl9ek+r3F1Sb9XKjGrpQ92e?=
 =?us-ascii?Q?UeMIDFIbVCirUrlgl7vJOBP3SeT+yEl77XwrTei4NhNJOQI6J3D7HMHhv2eZ?=
 =?us-ascii?Q?9g6CSsDzS8yyd9w2J2MKNUJJpMAX7XouZtJqRspw2QjZP9bGsQuHKJp52gk5?=
 =?us-ascii?Q?j6i12J4833/oTrmVkW+I5ivmXbGxIQ5lyfjr6APC9EHpMRY7GDvZMa/9eRCN?=
 =?us-ascii?Q?2bD/6HjVHAQrdqohWGcrWoAYqKvtCMJT8Vo+xHfPWF90a0hdYX2OL5hXEyLK?=
 =?us-ascii?Q?Y1fv0vYnqVDdGGHVgh0tZrhVkZ0OxO2+eFXQOJjrgf0ugjltECawPOnCKxYP?=
 =?us-ascii?Q?TXGdzPaPiaShLKbUN/fM7x3UNBqCF/FwdodEJHK/0PbJ9xxS+bn68z0lEBi7?=
 =?us-ascii?Q?tEcQO1KQyLhRfwY6jUSsPUY8pl4tLLAPGnuv0i3AmHSrTpU9VuQFrfGTdZrL?=
 =?us-ascii?Q?V8Q80uR7I5nPgL04ucncGfBtgxQUPsgtixogXQlrW6Wp7RcdiXRDd9kMZfZf?=
 =?us-ascii?Q?qkLXNGDYX4vS+jvNeKlbfeTZSinZCu8u9ahpnmtwIyKIm0/p3D5Zjfxudzyq?=
 =?us-ascii?Q?H6kOLHgnn9SEDezYjhs=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3600a474-87e3-4d93-acf0-08dc42c2cff8
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Mar 2024 18:32:46.1501
 (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: EUxASGNWLfVwnsqhcrWYqMTm/OMSEDAwkzki+psxAOwKxxUDzHY8Q6V67K6+Yo5d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR12MB7302

--=_MailMate_9FA95A4E-3296-4A51-8878-D2196FA15765_=
Content-Type: text/plain

On 12 Mar 2024, at 12:38, Matthew Wilcox wrote:

> On Tue, Mar 12, 2024 at 11:51:13AM -0400, Zi Yan wrote:
>> On 12 Mar 2024, at 10:19, Matthew Wilcox wrote:
>>
>>> On Tue, Mar 12, 2024 at 10:13:16AM -0400, Zi Yan wrote:
>>>> On 11 Mar 2024, at 23:45, Matthew Wilcox wrote:
>>>>> Much more important: You're doing this with a positive refcount, which
>>>>> breaks the (undocumented) logic in deferred_split_scan() that a folio
>>>>> with a positive refcount will not be removed from the list.
>>>>
>>>> What is the issue here? I thought as long as the split_queue_lock is held,
>>>> it should be OK to manipulate the list.
>>>
>>> I just worked this out yesterday:
>>> https://lore.kernel.org/linux-mm/Ze9EFdFLXQEUVtKl@casper.infradead.org/
>>> (the last chunk, starting with Ryan asking me "what about the first bug
>>> you found")
>>
>> Hmm, like you said a folio with a positive refcount will not be removed
>> from ds_queue->split_queue, it will have no chance going to the separate
>> list in deferred_list_scan() and list_del_init() will not corrupt
>> that list.
>
> You've misread it.  Folios with a _zero_ refcount are not removed from
> the list in deferred_split_scan.  Folios with a positive refcount are
> removed from the per-node or per-cgroup list _at which point there is
> an undocumented assumption_ that they will not be removed from the
> local list because they have a positive refcount.

But that sounds very subtle if not broken. As an outsider of
deferred_split_scan(), only !list_empty(folio->_deferred_list) is checked.
The condition can be true if the folio is on split_queue or
local list of deferred_split_scan() with elevated refcount. In that case,
the folio cannot be removed from the list (either split_queue or local list)
even if split_queue_lock is held, since local list manipulation is not under
split_queue_lock. This makes _deferred_list a one-way train to anyone
except deferred_split_scan(), namely folios can only be added into
_deferred_list until they are freed or split by deferred_split_scan().

Is that intended? If yes, maybe we should document it. If not, using
split_queue_lock to protect local list, or more explicitly folio->_deferred_list
might be better?


>> So it should be safe. Or the issue is that before migration
>> adding a refcount, the folio is removed from ds_queue->split_queue
>> and put on the list in deferred_list_scan(), as a result, any manipulation
>> of folio->_deferred_list could corrupt the list. Basically,
>> !list_empty(&folio->_deferred_list) cannot tell if the folio is on
>> ds_queue->split_queue or another list. I am not sure about why "a positive
>> refcount" is related here.
>>
>> That makes me wonder whether ds_queue->split_queue_lock is also needed
>> for list_for_each_entry_safe() in deferred_split_scan(). Basically,
>> ds_queue->split_queue_lock protects folio->_deferred_list in addition to
>> ds_queue->split_queue.
>>
>>
>>
>>>>> Maximally important: Wer shouldn't be doing any of this!  This folio is
>>>>> on the deferred split list.  We shouldn't be migrating it as a single
>>>>> entity; we should be splitting it now that we're in a context where we
>>>>> can do the right thing and split it.  Documentation/mm/transhuge.rst
>>>>> is clear that we don't split it straight away due to locking context.
>>>>> Splitting it on migration is clearly the right thing to do.
>>>>>
>>>>> If splitting fails, we should just fail the migration; splitting fails
>>>>> due to excess references, and if the source folio has excess references,
>>>>> then migration would fail too.
>>>>
>>>> You are suggesting:
>>>> 1. checking if the folio is on deferred split list or not
>>>> 2. if yes, split the folio
>>>> 3. if split fails, fail the migration as well.
>>>>
>>>> It sounds reasonable to me. The split folios should be migrated since
>>>> the before-split folio wants to be migrated. This split is not because
>>>> no new page cannot be allocated, thus the split folios should go
>>>> into ret_folios list instead of split_folios list.
>>>
>>> Yes, I'm happy for the split folios to be migrated.  Bonus points if you
>>> want to figure out what order to split the folio to ;-)  I don't think
>>> it's critical.
>>
>>
>> --
>> Best Regards,
>> Yan, Zi


--
Best Regards,
Yan, Zi

--=_MailMate_9FA95A4E-3296-4A51-8878-D2196FA15765_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmXwn8sPHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhUu1UP/2+SSwlKRhMeL1wMvzijDwafX70idrgMfeUi
2NyV1reVb8xrBJKLrglEn8Z/Yg9/2CpxYpgBZBiiftbCuqkqnqnrRRD1MvIbW6tk
0s27rk4pHS5Rua02zO1UtUhi0M4sfKSh8O/9bEipSibfpr88wqzp5RhJnH+2tGxG
fjKTSYj+l8OCkLv8H0us2yFfWBxIRXdueAAAdrQu2/qhFl/Rsxsa6AisNrDpqjMm
WWzGV+ZLmohPxRMw9ti2ixWtLIMZ7G0T7g6JGJaFwfXcO69+Tlcak73ZxM41QHKZ
EKI9kBcQbi9GzPwyWvkt7d1XWosBNDfUXPkDP1WCUux2CPW7/ViF5D1XmzjQ/c9f
eXFNyI+E6MUo68f/ivEp5vA7DZGrPMdLTEP11l9mVHp0HEolhzqDiXsAIAn+L7YP
jlIQb/wI88Duno8dr3stQINeA1qjfP+Zi2JIbEd6IK3hNpDbk9ygoqD/9yxWmvUT
AMcEH0ySyNI4rp2MVKqozDLxW3xRvbYXS8vWTliY6+jnd7aTuWFGgWDQwwJygcKb
SHr+N+8O0u51wHY5NFd0EXDvRz3P1l70G1FVldDqmJt+lC/H0UYJYQv+XUvwzmGf
IKX7CC9HVsSzJ3PT+Fn8PzrIuOCBt4TaAPffeamNb+5TQ9SWrHMTkjibhRn1E75U
L0VIuH81
=rU8n
-----END PGP SIGNATURE-----

--=_MailMate_9FA95A4E-3296-4A51-8878-D2196FA15765_=--