Skip to contentSkip to conversation

mm: Fix for negative counter: nr_file_hugepages

PATCH Superseded Stefan Roesch 4 participants 7 messages Nov 7, 2023 – Nov 8, 2023

Subsystems (3)
Stable branch Memory management THP

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Stefan Roesch

author
#0 lore raw

PATCH v2 1/1 mm: Fix for negative counter: nr_file_hugepages

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 <[email protected]>
Co-debugged-by Johannes Weiner <[email protected]>
Acked-by Johannes Weiner <[email protected]>
---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/huge_memory.c +1 -1
index 064fbd90822b4..9dbd5ef5a3902 100644
@@ -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

Signature · 1 line
2.39.3
build bot
#1 lore raw
FYI: kernel test robot notices the stable kernel rule is not satisfied.

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#option-1

Rule: add the tag "Cc: [email protected]" 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 open here
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
#2 lore raw
In reply to Stefan Roesch

mm/huge_memory.c int split_huge_page_to_list(struct page *page, struct list_head *list) · line 2746

Quoted 10 lines · Stefan Roesch

+++ b/mm/huge_memory.c

mm/huge_memory.c +1 -1
@@ -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!

#3 lore raw
Quoted 15 lines · Matthew Wilcox

mm/huge_memory.c int split_huge_page_to_list(struct page *page, struct list_head *list) · line 2746

Quoted 10 lines · Stefan Roesch

+++ b/mm/huge_memory.c

mm/huge_memory.c +1 -1
@@ -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?

#4 lore raw
Quoted 17 lines · Johannes Weiner
Quoted 15 lines · Matthew Wilcox

mm/huge_memory.c int split_huge_page_to_list(struct page *page, struct list_head *list) · line 2746

Quoted 10 lines · Stefan Roesch

+++ b/mm/huge_memory.c

mm/huge_memory.c +1 -1
@@ -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!

Quoted 1 line

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);
        }
#5 lore raw
Quoted 22 lines · Matthew Wilcox
Quoted 17 lines · Johannes Weiner
Quoted 15 lines · Matthew Wilcox

mm/huge_memory.c int split_huge_page_to_list(struct page *page, struct list_head *list) · line 2746

Quoted 10 lines · Stefan Roesch

+++ b/mm/huge_memory.c

mm/huge_memory.c +1 -1
@@ -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!

Quoted 1 line

Do we need to fix filemap_unaccount_folio() as well?

Looks to me like it is already correct?

Quoted 9 lines
        __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.

author
#6 lore raw
In reply to Matthew Wilcox
Quoted 15 lines · Matthew Wilcox

mm/huge_memory.c int split_huge_page_to_list(struct page *page, struct list_head *list) · line 2746

Quoted 10 lines · Stefan Roesch

+++ b/mm/huge_memory.c

mm/huge_memory.c +1 -1
@@ -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.