Re: [PATCH v2 1/5] mm: compaction: get reference before non LRU movable folio isolation

[Matthew Wilcox <willy@infradead.org>]

On Mon, Sep 02, 2024 at 06:44:09PM +0800, Kefeng Wang wrote:
> On 2024/8/31 22:04, Matthew Wilcox wrote:
> > On Thu, Aug 29, 2024 at 10:54:52PM +0800, Kefeng Wang wrote:
> > > Non-LRU movable folio isolation will fail if it can't grab a reference
> > > in isolate_movable_page(), so folio_get_nontail_page() could be called
> > > ahead to unify the handling of non-LRU movable/LRU folio isolation a bit,
> > > this is also prepare to convert isolate_movable_page() to take a folio.
> > > Since the reference count of the non-LRU movable folio is increased,
> > > a folio_put() is needed whether the folio is isolated or not.
> > 
> > There's a reason I stopped where I did when converting this function
> > to use folios.  Usually I would explain, but I think it would do you
> > good to think about why for a bit.
> 
> Hm, I don't find the reason,
> 
> The major change is that we move folio_get_nontail_page ahead, so we
> may try add a reference for each page, it always fails to isolate
> with/without this changes, so I suppose that there is no issue here,

You haven't considered the effect on others.  Taking the refcount on a
page will necessarily dirty the cacheline.  This is compaction code, so
someone else may have this page allocated.  The check is done without a
refcount in order to minimise the effect if this page cannot be migrated.

Try doing this on a NUMA system to really see the effects.

More broadly, the problem is that you're sending patches faster than I
can review them, and Andrew is picking them up.  I don't know what to
do about that.