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

[Yin, Fengwei]

On 3/13/2024 10:07 AM, Yin, Fengwei wrote:
> 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?
May not good idea because it's possible the folios in fbatch can
be removed from deferred_list by migration.

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