Bug #19101: madvise(free) was broken in 3.1?

• Status: Closed
• Assignee: ioquatix (Samuel Williams)
• Target version: -
• Backport: 2.7: DONTNEED, 3.0: DONTNEED, 3.1: DONE

[ioquatix (Samuel Williams)]

https://github.com/ruby/ruby/commit/77f3319071e600a2aafaa9863b892dfd3c1da343#r88774579

We need to investigate why this was done, how to fix it, and how to prevent regressions in the future.

[ioquatix (Samuel Williams)]

https://github.com/ruby/ruby/pull/6671

[ioquatix (Samuel Williams)]

• Status changed from Open to Closed

Merged.

[smcgivern (Sean McGivern)]

ioquatix (Samuel Williams) wrote:

https://github.com/ruby/ruby/commit/77f3319071e600a2aafaa9863b892dfd3c1da343#r88774579

We need to investigate why this was done, how to fix it, and how to prevent regressions in the future.

I don't think it's appropriate for Ruby itself to do this particular test, but could the Falcon project add benchmarks like https://bugs.ruby-lang.org/issues/15997#note-19 to CI? I guess if https://github.com/socketry/falcon-benchmark ran against different Ruby versions, you could also plot that.

Beyond that, the test with strace (using dtruss where applicable) in https://github.com/ruby/ruby/commit/77f3319071e600a2aafaa9863b892dfd3c1da343#r88774579 is fairly trivial but would involve shelling out, so I'm not sure if that's the kind of thing that typically happens in the Ruby core project either.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN to 2.7: DONTNEED, 3.0: DONTNEED, 3.1: REQUIRED

[ioquatix (Samuel Williams)]

I am copying all the discussion from GitHub here:

smcgivern yesterday

@ioquatix (Samuel Williams) I wasn't sure of the best way to let you know about this, so I am commenting on a commit from 2019

It looks like this use of MADV_FREE has effectively been reverted by 5e16c3a.

It places a higher priority on POSIX_MADV_DONTNEED, calling posix_madvise(base, size, POSIX_MADV_DONTNEED). Per https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/posix_madvise.c;h=71ef3435eaea2e0feb6954468bd1ba1f80f0a6d1;hb=HEAD:

/* We have one problem: the kernel's MADV_DONTNEED does not
correspond to POSIX's POSIX_MADV_DONTNEED.  The former simply
discards changes made to the memory without writing it back to
disk, if this would be necessary.  The POSIX behavior does not
allow this.  There is no functionality mapping the POSIX behavior
so far so we ignore that advice for now.  */
if (advice == POSIX_MADV_DONTNEED)
return 0;

I don't think that was intentional, and it may be a performance regression. To test this, I did:

$ strace -e trace=madvise ruby -e "p RUBY_VERSION; Fiber.new { 1 }.resume"
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352400, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352402, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352404, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352405, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352406, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352407, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352455, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352457, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352459, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352472, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352474, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352477, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352479, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352483, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352484, si_uid=1000, si_status=2, si_utime=0, si_stime=0} ---
"3.0.4"
madvise(0x7fd6a9149000, 655360, MADV_FREE) = 0
# switch to Ruby 3.1
$ strace -e trace=madvise ruby -e "p RUBY_VERSION; Fiber.new { 1 }.resume"
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352608, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352610, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352612, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352613, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352614, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352615, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352663, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352665, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352667, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352680, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352682, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352685, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352687, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352691, si_uid=1000, si_status=0, si_utime=0, si_stime=0} ---
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=352692, si_uid=1000, si_status=2, si_utime=0, si_stime=0} ---
"3.1.2"
+++ exited with 0 +++

ioquatix yesterday

Ah thanks, I didn't know this was going on. Do you propose we remove the posix variant?

mttkay 14 hours ago

@ioquatix (Samuel Williams) Just out of curiosity -- using madvise on memory regions that the Ruby VM had requested via malloc and freed via free (i.e. using a memory allocator that may be swapped out -- I think this is what happens in fiber_free?

ruby/cont.c
	 fiber_free(void *ptr) 

isn't this potentially undermining how that allocator manages memory? if jemalloc is used and if it would make its own choices w.r.t. how memory advice should be given via free (e.g. MADV_FREE), what would happen when the Ruby VM has a different opinion on this? Should Ruby maybe not do any of this and let the memory allocator decide how memory is returned to the kernel? What are the pros and cons of this?

smcgivern 12 hours ago

Thanks for looking at this, @ioquatix (Samuel Williams)!

For a bit of background: @mttkay and I work together, and we were investigating a case of seemingly high RSS for fluentd, which uses Async::HTTP in plugins that have HTTP servers. (Nice work on the Async series of libraries, by the way.)

It turned out that this was due to LazyFree memory - from MADV_FREE - being included in RSS, and not an actual increase in 'real' memory usage by fluentd. Incidentally, Go reverted their use of MADV_FREE due to similar concerns: golang/go#42330

So our goal here was really just to figure out why Ruby 3.1 behaved differently to 2.7 and 3.1; I personally don't have a strong opinion on which we use, but it seems odd to put the option which is a no-op on Linux first. Thanks for creating the issue, I'll add a short comment there too.

ioquatix 2 minutes ago

@mttkay madvise is not called on general memory allocated by malloc, and I agree with you it should not, so if that is happening some how please let me know.

@smcgivern (Sean McGivern) thanks for the details. Well, I don't have a strong opinion. I actually didn't want to do anything - everything you do here with madvise is a system call and an overhead for the sake of "using less memory". For the purpose of memory, you just tell the OS: discard these page data I don't care about them (worse case, they get paged to disk). For the purpose of performance, just keep them around.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.7: DONTNEED, 3.0: DONTNEED, 3.1: REQUIRED to 2.7: DONTNEED, 3.0: DONTNEED, 3.1: DONE

ruby_3_1 ad92236d245b791d14fd78bcb5a0a0789aa169c0 merged revision(s) 2bb89b7f114e4beb3012f63e12e726ae23005e6f.

[smcgivern (Sean McGivern)]

ioquatix (Samuel Williams) wrote in #note-5:

I am copying all the discussion from GitHub here:
[...]
smcgivern 12 hours ago

Thanks for looking at this, @ioquatix (Samuel Williams)!

For a bit of background: @mttkay and I work together, and we were investigating a case of seemingly high RSS for fluentd, which uses Async::HTTP in plugins that have HTTP servers. (Nice work on the Async series of libraries, by the way.)

It turned out that this was due to LazyFree memory - from MADV_FREE - being included in RSS, and not an actual increase in 'real' memory usage by fluentd. Incidentally, Go reverted their use of MADV_FREE due to similar concerns: golang/go#42330

In a link format that works here: https://github.com/golang/go/issues/42330