Possible 6.5 regression: Huge values for "commited memory"

Hello,

For the time being, just an observation: I monitor various parameters on my systems, and among them is "Committed memory", the Committed_AS value in /proc/meminfo.

Since upgrading to the 6.5.x series, I noticed the value there grows way higher values then previously in hppa, even if the machine is idle. Values seem to rise up to around 1.6 Gbyte, long-term average is rather 200-300 Mbyte. Also, I cannot see any memory hogs in top. The workload hasn't changed in months.

To sum it up, I reckon something went wrong in the memory usage accounting. Is this already on radar, or should I start bisecting? That might take a lot of time, though.

Christoph

Hi Christoph,

I doubt there is a specific memory leak in the parisc code. Usually we just touch driver code or other arch-related code, so if there is something wrong, then it must be in generic code and should be visible on other platforms too.
We changed to the SLUB allocator, but I don't think this makes any difference either.

Helge

Upgrading from what version?

Also, can you describe your setup? And what is the value of Committed_AS on the kernel before you upgrade? (Hint: paste full /proc/meminfo from both old and new kernel.)

You can certainly do bisection (see Documentation/admin-guide/bug-bisect.rst in the kernel sources for how to do that).

Thanks.

An old man doll... just what I always wanted! - Clara

I am also reproducing this issue.

Memory overcommit is disabled in my setup. Committed_AS grows to mem+swap after 1-2 hours of compilation load and system becomes nonoperational.

I've bisected this issue:

$ git bisect log
git bisect start
# status: waiting for both good and bad commits
# bad: [2dde18cd1d8fac735875f2e4987f11817cc0bc2c] Linux 6.5
git bisect bad 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
# status: waiting for good commit(s), bad commit known
# good: [f60d5fd5e950c89a38578ae6f25877de511bb031] Linux 6.4.15
git bisect good f60d5fd5e950c89a38578ae6f25877de511bb031
# good: [6995e2de6891c724bfeb2db33d7b87775f913ad1] Linux 6.4
git bisect good 6995e2de6891c724bfeb2db33d7b87775f913ad1
# good: [b775d6c5859affe00527cbe74263de05cfe6b9f9] Merge tag 'mips_6.5' of git://git.kernel.org/pub/scm/linux/kernel/git/mips/linux
git bisect good b775d6c5859affe00527cbe74263de05cfe6b9f9
# bad: [56cbceab928d7ac3702de172ff8dcc1da2a6aaeb] Merge tag 'usb-6.5-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
git bisect bad 56cbceab928d7ac3702de172ff8dcc1da2a6aaeb
# good: [b30d7a77c53ec04a6d94683d7680ec406b7f3ac8] Merge tag 'perf-tools-for-v6.5-1-2023-06-28' of git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next
git bisect good b30d7a77c53ec04a6d94683d7680ec406b7f3ac8
# bad: [dfab92f27c600fea3cadc6e2cb39f092024e1fef] Merge tag 'nfs-for-6.5-1' of git://git.linux-nfs.org/projects/trondmy/linux-nfs
git bisect bad dfab92f27c600fea3cadc6e2cb39f092024e1fef
# good: [28968f384be3c064d66954aac4c534a5e76bf973] Merge tag 'pinctrl-v6.5-1' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect good 28968f384be3c064d66954aac4c534a5e76bf973
# good: [5d95ff84e62be914b4a4dabfa814e4096b05b1b0] Merge tag 'v6.5-p1' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6
git bisect good 5d95ff84e62be914b4a4dabfa814e4096b05b1b0
# bad: [d25f002575146d67b5ebea541e6db3696c957c25] Merge tag 'cxl-for-6.5' of git://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl
git bisect bad d25f002575146d67b5ebea541e6db3696c957c25
# bad: [0a1c979c6b7dfe5b6c105d0f0f9f068b5eb07e25] Merge tag 'libnvdimm-for-6.5' of git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm
git bisect bad 0a1c979c6b7dfe5b6c105d0f0f9f068b5eb07e25
# good: [a507db1d8fdc39802415e4d2ef6d1aecd67927fa] Merge tag '6.5-rc-smb3-client-fixes-part1' of git://git.samba.org/sfrench/cifs-2.6
git bisect good a507db1d8fdc39802415e4d2ef6d1aecd67927fa
# good: [46e66dab8565f742374e9cc4ff7d35f344d774e2] dax/kmem: Pass valid argument to memory_group_register_static
git bisect good 46e66dab8565f742374e9cc4ff7d35f344d774e2
# bad: [170ab6c51a42a8a1c1a7ce09367b578db6f2f383] Merge tag 'flex-array-transformations-6.5-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux
git bisect bad 170ab6c51a42a8a1c1a7ce09367b578db6f2f383
# bad: [408579cd627a15bd703fe3eeb8485fd02726e9d3] mm: Update do_vmi_align_munmap() return semantics
git bisect bad 408579cd627a15bd703fe3eeb8485fd02726e9d3
# good: [e4bd84c069f212c01258e405f86e91f327888e41] mm: Always downgrade mmap_lock if requested
git bisect good e4bd84c069f212c01258e405f86e91f327888e41
# first bad commit: [408579cd627a15bd703fe3eeb8485fd02726e9d3] mm: Update do_vmi_align_munmap() return semantics

commit 408579cd627a15bd703fe3eeb8485fd02726e9d3
Author: Liam R. Howlett
Date:   Thu Jun 29 22:28:16 2023 -0400

    mm: Update do_vmi_align_munmap() return semantics

    Since do_vmi_align_munmap() will always honor the downgrade request on the success, the callers no longer have to deal with confusing return codes.  Since all callers that request downgrade actually want the lock to be dropped, change the downgrade to an unlock request.

    Note that the lock still needs to be held in read mode during the page table clean up to avoid races with a map request.

    Update do_vmi_align_munmap() to return 0 for success.  Clean up the callers and comments to always expect the unlock to be honored on the success path.  The error path will always leave the lock untouched.

    As part of the cleanup, the wrapper function do_vmi_munmap() and callers to the wrapper are also updated.

Looks like memory statistics depends on removed locks.

Thanks.

Thanks for the regression report. Michael had already bisected it [1], so telling regzbot:

#regzbot ^introduced: 408579cd627a15
#regzbot title: huge committed memory due to returning 0 on do_vmi_align_mmunmap() success

[1]: https://lore.kernel.org/linux-parisc/[email protected]/

An old man doll... just what I always wanted! - Clara

Funky. That commit isn't actually supposed to change anything, and the only locking change was because it incorrectly ended up doing the unlock a bit too early (before it did a validate_mm() - fixed in commit b5641a5d8b8b ("mm: don't do validate_mm() unnecessarily and without mmap locking").

HOWEVER.

Now that I look at it again, I note this change in move_vma().

- if (do_vmi_munmap(&vmi, mm, old_addr, old_len, uf_unmap, false) < 0) {
+ if (!do_vmi_munmap(&vmi, mm, old_addr, old_len, uf_unmap, false)) {

and I think that is wrong.

The return value that changed was the old "return 1 if successful _and_ lock downgraded".

Now it does "lock is always released on success if requested". So the special "1" return went away, but the failure case didn't change.

So that change to "move_vma()" seems to be bogus. It used to do "if failed". Now it does "if success".

Does the attached patch fix the problem?

Liam - or am I just crazy? That return value check change really looks bogus to me, but it looks *so* bogus that it makes me think I'm missing something.

Linus

 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 056478c106ee..382e81c33fc4 100644
@@ -715,7 +715,7 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 	}
 
 	vma_iter_init(&vmi, mm, old_addr);
-	if (!do_vmi_munmap(&vmi, mm, old_addr, old_len, uf_unmap, false)) {
+	if (do_vmi_munmap(&vmi, mm, old_addr, old_len, uf_unmap, false) < 0) {
 		/* OOM: unable to split vma, just get accounts right */
 		if (vm_flags & VM_ACCOUNT && !(flags & MREMAP_DONTUNMAP))
 			vm_acct_memory(old_len >> PAGE_SHIFT);

So while I didn't confirm the fix myself, I'm pretty sure that was it. Getting the return value wrong would cause an incorrect extra vm_acct_memory() call in the non-error case when VM_ACCOUNT is set (and mean the loss of one in the error case, but the error case never happens in practice).

Which then causes 'vm_committed_as' to grow when it shouldn't, and causes exactly that "Committed_AS" in /proc/meminfo to be off.

So here's the same patch, but now with a proper commit message etc.

I haven't pushed it out (because it would be lovely to get a "Tested-by" for it, and that will make the commit ID change), but I'll probably do so later today, with or without confirmation, because it does seem to be the problem.

Linus

From 65a0e100c8a6f8763a9c3bf2c0b361c8f436e42d Mon Sep 17 00:00:00 2001
From: Linus Torvalds <[email protected]>
Date: Sat, 16 Sep 2023 12:31:42 -0700
Subject: [PATCH] vm: fix move_vma() memory accounting being off

Commit 408579cd627a ("mm: Update do_vmi_align_munmap() return semantics") seems to have updated one of the callers of do_vmi_munmap()
incorrectly: it used to check for the error case (which didn't
change: negative means error).

That commit changed the check to the success case (which did change:
before that commit, 0 was success, and 1 was "success and lock downgraded". After the change, it's always 0 for success, and the lock will have been released if requested).

This didn't change any actual VM behavior _except_ for memory accounting when 'VM_ACCOUNT' was set on the vma. Which made the wrong return value test fairly subtle, since everything continues to work.

Or rather - it continues to work but the "Committed memory" accounting goes all wonky (Committed_AS value in /proc/meminfo), and depending on settings that then causes problems much much later as the VM relies on bogus statistics for its heuristics.

Revert that one line of the change back to the original logic.

---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 056478c106ee..382e81c33fc4 100644
@@ -715,7 +715,7 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 	}
 
 	vma_iter_init(&vmi, mm, old_addr);
-	if (!do_vmi_munmap(&vmi, mm, old_addr, old_len, uf_unmap, false)) {
+	if (do_vmi_munmap(&vmi, mm, old_addr, old_len, uf_unmap, false) < 0) {
 		/* OOM: unable to split vma, just get accounts right */
 		if (vm_flags & VM_ACCOUNT && !(flags & MREMAP_DONTUNMAP))
 			vm_acct_memory(old_len >> PAGE_SHIFT);

2.42.0.rc0.30.gca81aba3b0

This patch fixes problem with counter for committed memory.

Thanks!

Oh, no, thank *you*. With the bisection, this was fairly straightforward.

A silly mistake in the original patch, but hard to see without a clear "this is when the problem was introduced".

Linus

Greg & Sasha,

can you please queue-up this upstream patch for kernel v6.5-stable ?

commit 3cec50490969afd4a76ccee441f747d869ccff77

Author: Linus Torvalds <[email protected]>
Date:   Sat Sep 16 12:31:42 2023 -0700

vm: fix move_vma() memory accounting being off

Thanks,
Helge

Now queued up, thanks.

greg k-h

[
A tad late in the party, I know. The box I had encoutered this isn't quite on 24/7, and reproduction took some time.
]

Linus Torvalds wrote...

Confirmed this solves the issue here, too. Tested on 6.6-rc2.

Christoph

You are correct. I should not have changed that check.

I haven't devised a way to check this on parisc (I finally have a qemu parisc vm booting), but it should happen on any call to move_vma().

It looks like you have a tester though, so I'll return to dinner.

Thanks,
Liam

#regzbot fix: 3cec50490969af

An old man doll... just what I always wanted! - Clara