Received-SPF: pass (google.com: domain of linux-kernel+bounces-100475-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
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 11:51:13 -0400
Message-ID: <DFA2B8B7-C690-422A-BE95-82F7E112BB95@nvidia.com>
In-Reply-To: <ZfBkjf4z-jpaNk6c@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>
Content-Type: multipart/signed;
 boundary="=_MailMate_0D4F97BF-8CEC-4487-B1E3-647E3C31FD51_=";
 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?b3s19Z82eIAtUB1MzzVyxfsKb5Fcp/EN9fDorhm1I3zv/GfMBkJ8N/pstcMj?=
 =?us-ascii?Q?H/FDDK8B223RKdIwphAj8CJOyKjOEQdF3L/kJgRmbJoQ7mF3EQFCaxt27xHZ?=
 =?us-ascii?Q?n6UAXfyzDiqKbxoGvlWJ4hjRIkz+i4JptjEhCnEDc42N7GCzw08HeK5OtQ84?=
 =?us-ascii?Q?wjWCd30ASU6Mf6oaAXT0R6FbzKzWMTnRwabCBmSRA8peKMf0lcZnEdFFWeOB?=
 =?us-ascii?Q?/B/QVIbSFvYPPgRz9C/S4Y2M5+E5z8LZubjU+Dq6UPUtWdY/rXlnlLHzYd/D?=
 =?us-ascii?Q?Fa5GgjBzU+kSNuShimtT6JU2rf9XOETdgY/UYTEHDLsS8luXTIeI7tWCaXeC?=
 =?us-ascii?Q?a5Yjp2mWwcTSQzx3ZmVDvVhmDZSK0RZdvBQGaiQAVF60P1zV8MRmMLvucJKC?=
 =?us-ascii?Q?CraQg3MapLutF6Kk4EvQHwRsBMnLo+enWjxJURqGzun+UEga3a4OUeiKXVL1?=
 =?us-ascii?Q?wCr0OF0UDAQRXzpbb8+gncENTEDXmScuZgR4yL1x90ZU12tMNhjmoGohJgdK?=
 =?us-ascii?Q?wi4CXWYkFgcirnlMSFg8ve5R5h3Z//kKV+YQ0jpYywDVP5abMbPnlGj33rHH?=
 =?us-ascii?Q?EJ0+iSGwVlaZtf+To54WtGg2M0Z7OLyN4yhNEfyi3rX2j5IRthXIFcBz6Nmb?=
 =?us-ascii?Q?PJDVUUtpUc1o9+Q8TJ8m8yqH9z3nTgn4OrKXE18paNM0/JcX3Ae7v0dj8r/X?=
 =?us-ascii?Q?pQzlgkxdcQwB2ujH0xSky6Nh05boLQDNjmYhau89jWBOITc9PC0N65/NmJq0?=
 =?us-ascii?Q?sUPnOXJo4Ih5JQwrZlkqb8+LoRZAJS5IKgNfCu6DnEUdq9tiaBTjkuyBGx+x?=
 =?us-ascii?Q?BlGumHJO5GXXp9cFExSMMod8PqZPLcvnn27E0UvsXGbhPqzNiO6aeknLffed?=
 =?us-ascii?Q?BQCbwFYmhHM+NSpPjeoLoQt4xM5mUbWbH3GDqbT+jwUo7VSD7G1cPKOp05cb?=
 =?us-ascii?Q?QKweBfhuYxZ+XEeEiVePGgsR54SSxO4brDbNG5y5ZnAQNJImjDw+fztyXhCa?=
 =?us-ascii?Q?LSxNUuS246TgtNFJwXNREWzllsCx5RUUxBuU7g0PoGVxe/gmzm8E0KZJYM+C?=
 =?us-ascii?Q?k/rGFkvaSbuTeMGxWKZ1eIz0l4RB2iSyDL+0NeMKnlgqQ8+HzWRYBaqjtENt?=
 =?us-ascii?Q?A5JtwoT7hYN82YnE8uYP9V+SMID1UEs3Jy0F+KYjzuAjizH/3sDHqMkPVVpd?=
 =?us-ascii?Q?Zf80OOt8pnQTxhY2VkVFyD29OIGTIK/qnb1J6+umcH6kfA6hkovaCH/XGh8v?=
 =?us-ascii?Q?t/Tx5o7H9TU41CGE5zLmIyt3si+0TDB1Y1MPaU2CWslj+IHpgE/N/Tr6kggU?=
 =?us-ascii?Q?4GJSA8LIBaCsn+6zk2HLG18gxCHeEZtrRR/9IpYTk+gKPdoCP6gXwWsIv7Ls?=
 =?us-ascii?Q?bg2GVCi1qcFK8ehv3/T5vM2cy7zJH3nruU3s4zGN7anpVtMQMPWt8TMnESVv?=
 =?us-ascii?Q?b5cL7dkTG5Q2AxN58J/p9l8FeUl/sQPo/Y19OSTUIN5wYA+aeWqrfn2lG8ZI?=
 =?us-ascii?Q?5wqY+tnt4bPHZVPT3LpH08ADyIvzHtNnQa9Zzr9WWJYDK2fVZf0I/LKf4FQV?=
 =?us-ascii?Q?JS99/c4vL0PC+kP7Q5I=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 55f7bd9d-aca0-45f5-cd27-08dc42ac400f
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5744.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Mar 2024 15:51:15.7961
 (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: 9g7EJLy4lhsre255zMHtfraC8tzLjDTIuEUPDo7Q11WFp4cFZvmuHyX7c6x0keUB
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB7616

--=_MailMate_0D4F97BF-8CEC-4487-B1E3-647E3C31FD51_=
Content-Type: text/plain

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. 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

--=_MailMate_0D4F97BF-8CEC-4487-B1E3-647E3C31FD51_=
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename=signature.asc
Content-Type: application/pgp-signature; name=signature.asc

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

iQJDBAEBCgAtFiEE6rR4j8RuQ2XmaZol4n+egRQHKFQFAmXwefEPHHppeUBudmlk
aWEuY29tAAoJEOJ/noEUByhUoWkP/2o98M7I7xOCTdsIyfpY28uff9HIZGhCHQ0j
g4r8l0QReZE8GorqIjBUwnJ4InqjcCXjih0c523qWiiXcb30ha8A11TRLQxCAaih
dNlXqE1HMirY5chwkE/pv5tt0uIT3ztZub+L28HfUqciiPxoGnLeK/J57kgSN3tY
U4auGApTyNCPDUyRaIwn4ckUhOjoe2BvFLN3bxsDBFd6+0ulbp7BnP59KPjOF+J9
H5o/V81utknXoG3yH1TA3AvP2cnPcgLqbYME6mjPxwQuXLOLLT/rlsY/yS0RZVbF
viN5J50V2JHna+fD6N2AR6W256adFKP4rN+3jt7BvwfXYsA6TzKqh/UZf/cUnXTB
y0cImkRhCpScoqaJAPAmz+GFEZ9IK2vNA2laLrl9r9XNJLSLeW2nFQXbXpBN9K+f
YfDSEXSOb2F4JbPwOXuGlzOXi7LP0vq/pxn7lyr9C0zjs3ywaD2XZ9qdSVkEi0n0
3+I6Ox5LgzGD20xGMv3/6EBbIwV9eZ0S1ZOcQixdg9jGquG/CZ9UB16sYOLQM5ir
Eqb0ILGxrCm6k9IUgsbeeL6YunLuvBzJDSvWlMfbyF3jo9gg5n8l6/1fYpTjIml6
YAiED1QPugd5oiLE3k4dgKzL6fkUtNQoa6/ZAKTteITmmCrKpFDJzi9uttKZa0he
jfEGMrRc
=ExKy
-----END PGP SIGNATURE-----

--=_MailMate_0D4F97BF-8CEC-4487-B1E3-647E3C31FD51_=--