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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/9/5 3:02, Matthew Wilcox wrote:
> 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.
> 

Indeed, I asked whether the taking the refcount on a page
unconditionally is accepted or not in v1, but I should wait for more
time to see if there are more comments, thanks for your explanation,
will consider not only the current code modification, but also other
impacts of changes as much as possible.

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

OK, I will slowdown for the new version to collect more comments.