Re: Possible 6.5 regression: Huge values for "commited memory"

Re: Possible 6.5 regression: Huge values for "commited memory"

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

On Sun, Sep 10, 2023 at 07:48:23PM +0200, Christoph Biedl wrote:
> 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.

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.)

> 
> 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.
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Possible 6.5 regression: Huge values for "commited memory"

[Michael Labiuk]

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.

Re: Possible 6.5 regression: Huge values for "commited memory"

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1515 bytes --]

On Sat, 16 Sept 2023 at 04:43, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> 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/30f16b4f-a2fa-fc42-fe6e-abad01c3f794@virtuozzo.com/

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 610 bytes --]

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

diff --git a/mm/mremap.c b/mm/mremap.c
index 056478c106ee..382e81c33fc4 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -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);

Re: Possible 6.5 regression: Huge values for "commited memory"

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Sat, 16 Sept 2023 at 12:31, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Does the attached patch fix the problem?

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

[-- Attachment #2: 0001-vm-fix-move_vma-memory-accounting-being-off.patch --]
[-- Type: text/x-patch, Size: 2297 bytes --]

From 65a0e100c8a6f8763a9c3bf2c0b361c8f436e42d Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
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.

Fixes: 408579cd627a ("mm: Update do_vmi_align_munmap() return semantics")
Reported-by: Christoph Biedl <linux-kernel.bfrz@manchmal.in-ulm.de>
Reported-and-bisected-by: Michael Labiuk <michael.labiuk@virtuozzo.com>
Cc: Bagas Sanjaya <bagasdotme@gmail.com>
Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
Link: https://lore.kernel.org/all/1694366957@msgid.manchmal.in-ulm.de/
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 056478c106ee..382e81c33fc4 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -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

Re: Possible 6.5 regression: Huge values for "commited memory"

[Michael Labiuk]

On 17.09.2023 00:17, Linus Torvalds wrote:

> On Sat, 16 Sept 2023 at 12:31, Linus Torvalds 
> <torvalds@linux-foundation.org> wrote:
>
>> Does the attached patch fix the problem?
>>
This patch fixes problem with counter for committed memory.

Thanks!

Re: Possible 6.5 regression: Huge values for "commited memory"

[Linus Torvalds]

On Sat, 16 Sept 2023 at 15:20, Michael Labiuk
<michael.labiuk@virtuozzo.com> wrote:
>
> 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

Re: Possible 6.5 regression: Huge values for "commited memory"

[Liam R. Howlett]

* Linus Torvalds <torvalds@linux-foundation.org> [230916 15:32]:
> On Sat, 16 Sept 2023 at 04:43, Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> >
> > 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/30f16b4f-a2fa-fc42-fe6e-abad01c3f794@virtuozzo.com/
> 
> 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.

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

Re: Possible 6.5 regression: Huge values for "commited memory"

[Helge Deller]

Greg & Sasha,

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

commit 3cec50490969afd4a76ccee441f747d869ccff77
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat Sep 16 12:31:42 2023 -0700

     vm: fix move_vma() memory accounting being off

Thanks,
Helge


On 9/16/23 23:17, Linus Torvalds wrote:
> On Sat, 16 Sept 2023 at 12:31, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Does the attached patch fix the problem?
>
> 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

Re: Possible 6.5 regression: Huge values for "commited memory"

[Greg Kroah-Hartman]

On Sun, Sep 17, 2023 at 07:02:50AM +0200, Helge Deller wrote:
> Greg & Sasha,
> 
> can you please queue-up this upstream patch for kernel v6.5-stable ?
> 
> commit 3cec50490969afd4a76ccee441f747d869ccff77
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Sat Sep 16 12:31:42 2023 -0700
> 
>     vm: fix move_vma() memory accounting being off

Now queued up, thanks.

greg k-h

Re: Possible 6.5 regression: Huge values for "commited memory"

[Christoph Biedl]

[
    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...

> 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.

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

    Christoph