Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <627a71f8-e879-69a5-ceb3-fc8d29d2f7f1@suse.cz>
Date:   Mon, 9 May 2022 18:05:13 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [v3 PATCH 0/8] Make khugepaged collapse readonly FS THP more
 consistent
Content-Language: en-US
To:     Yang Shi <shy828301@gmail.com>, kirill.shutemov@linux.intel.com,
        linmiaohe@huawei.com, songliubraving@fb.com, riel@surriel.com,
        willy@infradead.org, ziy@nvidia.com, tytso@mit.edu,
        akpm@linux-foundation.org
Cc:     linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220404200250.321455-1-shy828301@gmail.com>
From:   Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <20220404200250.321455-1-shy828301@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/4/22 22:02, Yang Shi wrote:
>  include/linux/huge_mm.h        | 14 ++++++++++++
>  include/linux/khugepaged.h     | 59 ++++++++++++---------------------------------------
>  include/linux/sched/coredump.h |  3 ++-
>  kernel/fork.c                  |  4 +---
>  mm/huge_memory.c               | 15 ++++---------
>  mm/khugepaged.c                | 76 +++++++++++++++++++++++++++++++++++++-----------------------------
>  mm/mmap.c                      | 14 ++++++++----
>  mm/shmem.c                     | 12 -----------
>  8 files changed, 88 insertions(+), 109 deletions(-)
 
Resending my general feedback from mm-commits thread to include the
public ML's:

There's modestly less lines in the end, some duplicate code removed,
special casing in shmem.c removed, that's all good as it is. Also patch 8/8
become quite boring in v3, no need to change individual filesystems and also
no hook in fault path, just the common mmap path. So I would just handle
patch 6 differently as I just replied to it, and acked the rest.

That said it's still unfortunately rather a mess of functions that have
similar names. transhuge_vma_enabled(vma). hugepage_vma_check(vma),
transparent_hugepage_active(vma), transhuge_vma_suitable(vma, addr)?
So maybe still some space for further cleanups. But the series is fine as it
is so we don't have to wait for it now.

We could also consider that the tracking of which mm is to be scanned is
modelled after ksm which has its own madvise flag, but also no "always"
mode. What if for THP we only tracked actual THP madvised mm's, and in
"always" mode just scanned all vm's, would that allow ripping out some code
perhaps, while not adding too many unnecessary scans? If some processes are
being scanned without any effect, maybe track success separately, and scan
them less frequently etc. That could be ultimately more efficinet than
painfully tracking just *eligibility* for scanning in "always" mode?

Even more radical thing to consider (maybe that's a LSF/MM level topic, too
bad :) is that we scan pagetables in ksm, khugepaged, numa balancing, soon
in MGLRU, and I probably forgot something else. Maybe time to think about
unifying those scanners?