Received-SPF: pass (google.com: domain of linux-kernel+bounces-100686-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 12 Mar 2024 18:46:06 +0000
From: Matthew Wilcox <willy@infradead.org>
To: 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.
Message-ID: <ZfCi7kvLoBvUnARz@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>
 <76BBDB5D-9D40-44EB-A996-767404B85BE0@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <76BBDB5D-9D40-44EB-A996-767404B85BE0@nvidia.com>

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

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