Re: [PATCH v2] mm/migrate: put dest folio on deferred split list if source was there.

From: "Yin, Fengwei" <fengwei.yin@intel.com>
To: Matthew Wilcox <willy@infradead.org>, Zi Yan <ziy@nvidia.com>
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: Wed, 13 Mar 2024 10:07:06 +0800	[thread overview]
Message-ID: <b5017375-d43b-44f9-b931-3046ebead9cf@intel.com> (raw)
In-Reply-To: <ZfCi7kvLoBvUnARz@casper.infradead.org>

On 3/13/2024 2:46 AM, Matthew Wilcox wrote:
> On Tue, Mar 12, 2024 at 02:32:43PM -0400, Zi Yan wrote:
>> On 12 Mar 2024, at 12:38, Matthew Wilcox wrote:
>>> 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
> 
> I merely deduced this requirement; I didn't come up with it ...
My understanding is that this requirement is because of just local
list in deferred_split_scan().

Using fbatch instead of local list here as your created for that
issue debugging can eliminate this subtlety?

Regards
Yin, Fengwei

> 
>> 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?
> 
> To be fair, the folio can be split by anybody as
> split_huge_page_to_list_to_order() is careful to only manipulate the
> deferred list while the refcount is frozen at 0.  I'm still trying to
> figure out where to document this behaviour of the deferred list that
> someone (for example, your good self) would actually see it.
> 
> 
> 