While qualifiying the 6.4 release, the following warning was detected in messages:
vmstat_refresh: nr_file_hugepages -15664
The warning is caused by the incorrect updating of the NR_FILE_THPS counter in the function split_huge_page_to_list. The if case is checking for folio_test_swapbacked, but the else case is missing the check for folio_test_pmd_mappable. The other functions that manipulate the counter like __filemap_add_folio and filemap_unaccount_folio have the corresponding check.
I have a test case, which reproduces the problem. It can be found here: https://github.com/sroeschus/testcase/blob/main/vmstat_refresh/madv.c
The test case reproduces on an XFS filesystem. Running the same test case on a BTRFS filesystem does not reproduce the problem.
AFAIK version 6.1 until 6.6 are affected by this problem.
Signed-off-by: Stefan Roesch shr@devkernel.io Co-debugged-by: Johannes Weiner hannes@cmpxchg.org Acked-by: Johannes Weiner hannes@cmpxchg.org --- mm/huge_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 064fbd90822b4..9dbd5ef5a3902 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -2740,7 +2740,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) if (folio_test_swapbacked(folio)) { __lruvec_stat_mod_folio(folio, NR_SHMEM_THPS, -nr); - } else { + } else if (folio_test_pmd_mappable(folio)) { __lruvec_stat_mod_folio(folio, NR_FILE_THPS, -nr); filemap_nr_thps_dec(mapping);
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH v2] mm: Fix for negative counter: nr_file_hugepages Link: https://lore.kernel.org/stable/20231107181805.4188397-1-shr%40devkernel.io
On Tue, Nov 07, 2023 at 10:18:05AM -0800, Stefan Roesch wrote:
+++ b/mm/huge_memory.c @@ -2740,7 +2740,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) if (folio_test_swapbacked(folio)) { __lruvec_stat_mod_folio(folio, NR_SHMEM_THPS, -nr);
} else {
} else if (folio_test_pmd_mappable(folio)) { __lruvec_stat_mod_folio(folio, NR_FILE_THPS, -nr); filemap_nr_thps_dec(mapping);
As I said, we also need the folio_test_pmd_mappable() for swapbacked. Not because there's currently a problem, but because we don't leave landmines for other people to trip over in future!
On Tue, Nov 07, 2023 at 07:35:37PM +0000, Matthew Wilcox wrote:
On Tue, Nov 07, 2023 at 10:18:05AM -0800, Stefan Roesch wrote:
+++ b/mm/huge_memory.c @@ -2740,7 +2740,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) if (folio_test_swapbacked(folio)) { __lruvec_stat_mod_folio(folio, NR_SHMEM_THPS, -nr);
} else {
} else if (folio_test_pmd_mappable(folio)) { __lruvec_stat_mod_folio(folio, NR_FILE_THPS, -nr); filemap_nr_thps_dec(mapping);As I said, we also need the folio_test_pmd_mappable() for swapbacked. Not because there's currently a problem, but because we don't leave landmines for other people to trip over in future!
Do we need to fix filemap_unaccount_folio() as well?
On Tue, Nov 07, 2023 at 03:06:16PM -0500, Johannes Weiner wrote:
On Tue, Nov 07, 2023 at 07:35:37PM +0000, Matthew Wilcox wrote:
On Tue, Nov 07, 2023 at 10:18:05AM -0800, Stefan Roesch wrote:
+++ b/mm/huge_memory.c @@ -2740,7 +2740,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) if (folio_test_swapbacked(folio)) { __lruvec_stat_mod_folio(folio, NR_SHMEM_THPS, -nr);
} else {
} else if (folio_test_pmd_mappable(folio)) { __lruvec_stat_mod_folio(folio, NR_FILE_THPS, -nr); filemap_nr_thps_dec(mapping);As I said, we also need the folio_test_pmd_mappable() for swapbacked. Not because there's currently a problem, but because we don't leave landmines for other people to trip over in future!
Do we need to fix filemap_unaccount_folio() as well?
Looks to me like it is already correct?
__lruvec_stat_mod_folio(folio, NR_FILE_PAGES, -nr); if (folio_test_swapbacked(folio)) { __lruvec_stat_mod_folio(folio, NR_SHMEM, -nr); if (folio_test_pmd_mappable(folio)) __lruvec_stat_mod_folio(folio, NR_SHMEM_THPS, -nr); } else if (folio_test_pmd_mappable(folio)) { __lruvec_stat_mod_folio(folio, NR_FILE_THPS, -nr); filemap_nr_thps_dec(mapping); }
On Tue, Nov 07, 2023 at 08:07:59PM +0000, Matthew Wilcox wrote:
On Tue, Nov 07, 2023 at 03:06:16PM -0500, Johannes Weiner wrote:
On Tue, Nov 07, 2023 at 07:35:37PM +0000, Matthew Wilcox wrote:
On Tue, Nov 07, 2023 at 10:18:05AM -0800, Stefan Roesch wrote:
+++ b/mm/huge_memory.c @@ -2740,7 +2740,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) if (folio_test_swapbacked(folio)) { __lruvec_stat_mod_folio(folio, NR_SHMEM_THPS, -nr);
} else {
} else if (folio_test_pmd_mappable(folio)) { __lruvec_stat_mod_folio(folio, NR_FILE_THPS, -nr); filemap_nr_thps_dec(mapping);As I said, we also need the folio_test_pmd_mappable() for swapbacked. Not because there's currently a problem, but because we don't leave landmines for other people to trip over in future!
Do we need to fix filemap_unaccount_folio() as well?
Looks to me like it is already correct?
__lruvec_stat_mod_folio(folio, NR_FILE_PAGES, -nr); if (folio_test_swapbacked(folio)) { __lruvec_stat_mod_folio(folio, NR_SHMEM, -nr); if (folio_test_pmd_mappable(folio)) __lruvec_stat_mod_folio(folio, NR_SHMEM_THPS, -nr); } else if (folio_test_pmd_mappable(folio)) { __lruvec_stat_mod_folio(folio, NR_FILE_THPS, -nr); filemap_nr_thps_dec(mapping); }
Argh, I overlooked it because it's nested further in due to that NR_SHMEM update. Sorry about the noise.
Matthew Wilcox willy@infradead.org writes:
On Tue, Nov 07, 2023 at 10:18:05AM -0800, Stefan Roesch wrote:
+++ b/mm/huge_memory.c @@ -2740,7 +2740,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list) if (folio_test_swapbacked(folio)) { __lruvec_stat_mod_folio(folio, NR_SHMEM_THPS, -nr);
} else {
} else if (folio_test_pmd_mappable(folio)) { __lruvec_stat_mod_folio(folio, NR_FILE_THPS, -nr); filemap_nr_thps_dec(mapping);As I said, we also need the folio_test_pmd_mappable() for swapbacked. Not because there's currently a problem, but because we don't leave landmines for other people to trip over in future!
I'll add it in the next version.
linux-stable-mirror@lists.linaro.org
-
Johannes Weiner -
kernel test robot -
Matthew Wilcox -
Stefan Roesch