On 05/09/2024 09:46, Hugh Dickins wrote:
> On Fri, 30 Aug 2024, Usama Arif wrote:
>
>> From: Yu Zhao <yuzhao@xxxxxxxxxx>
>>
>> If a tail page has only two references left, one inherited from the
>> isolation of its head and the other from lru_add_page_tail() which we
>> are about to drop, it means this tail page was concurrently zapped.
>> Then we can safely free it and save page reclaim or migration the
>> trouble of trying it.
>>
>> Signed-off-by: Yu Zhao <yuzhao@xxxxxxxxxx>
>> Tested-by: Shuang Zhai <zhais@xxxxxxxxxx>
>> Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
>
> I'm sorry, but I think this patch (just this 1/6) needs to be dropped:
> it is only an optimization, and unless a persuasive performance case
> can be made to extend it, it ought to go (perhaps revisited later).
>
I am ok for patch 1 only to be dropped. Patches 2-6 are not dependent on it.
Its an optimization and underused shrinker doesn't depend on it.
Its possible that folio->new_folio below might fix it? But if it doesn't,
I can retry later on to make this work and resend it only if it alone shows
a significant performance improvement.
Thanks a lot for debugging this! and sorry it caused an issue.
> The problem I kept hitting was that all my work, requiring compaction and
> reclaim, got (killably) stuck in or repeatedly calling reclaim_throttle():
> because nr_isolated_anon had grown high - and remained high even when the
> load had all been killed.
>
> Bisection led to the 2/6 (remap to shared zeropage), but I'd say this 1/6
> is the one to blame. I was intending to send this patch to "fix" it:
>
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3295,6 +3295,8 @@ static void __split_huge_page(struct pag
> folio_clear_active(new_folio);
> folio_clear_unevictable(new_folio);
> list_del(&new_folio->lru);
> + node_stat_sub_folio(folio, NR_ISOLATED_ANON +
> + folio_is_file_lru(folio));
Maybe this should have been below? (Notice the folio->new_folio)
+ node_stat_sub_folio(new_folio, NR_ISOLATED_ANON +
+ folio_is_file_lru(new_folio));
> if (!folio_batch_add(&free_folios, new_folio)) {
> mem_cgroup_uncharge_folios(&free_folios);
> free_unref_folios(&free_folios);
>
> And that ran nicely, until I terminated the run and did
> grep nr_isolated /proc/sys/vm/stat_refresh /proc/vmstat
> at the end: stat_refresh kindly left a pr_warn in dmesg to say
> nr_isolated_anon -334013737
>
> My patch is not good enough. IIUC, some split_huge_pagers (reclaim?)
> know how many pages they isolated and decremented the stats by, and
> increment by that same number at the end; whereas other split_huge_pagers
> (migration?) decrement one by one as they go through the list afterwards.
>
> I've run out of time (I'm about to take a break): I gave up researching
> who needs what, and was already feeling this optimization does too much
> second guessing of what's needed (and its array of VM_WARN_ON_ONCE_FOLIOs
> rather admits to that).
>
> And I don't think it's as simple as moving the node_stat_sub_folio()
> into 2/6 where the zero pte is substituted: that would probably handle
> the vast majority of cases, but aren't there others which pass the
> folio_ref_freeze(new_folio, 2) test - the title's zapped tail pages,
> or racily truncated now that the folio has been unlocked, for example?
>
> Hugh
