[ruby-core:90177] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[ruby-core:90177] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Eric Wong]

k0kubun@ruby-lang.org wrote:
> k0kubun	2018-11-27 00:47:20 +0900 (Tue, 27 Nov 2018)
> 
>   New Revision: 66001
> 
>   https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=66001
> 
>   Log:
>     vm_trace.c: MJIT-limited thread-safety for postponed_job

Unfortunately, this breaks async-signal-safety because signal handler
can see mjit_enabled as true.

I will replace it, now.

[ruby-core:90182] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Eric Wong]

> >   https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=66001
> > 
> >   Log:
> >     vm_trace.c: MJIT-limited thread-safety for postponed_job
> 
> Unfortunately, this breaks async-signal-safety because signal handler
> can see mjit_enabled as true.

Testing (will take a while):
https://80x24.org/spew/20181130023856.27983-1-e@80x24.org/raw

And my connection to my workstation keeps dropping

[ruby-core:90183] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Eric Wong]

> https://80x24.org/spew/20181130023856.27983-1-e@80x24.org/raw

Committed as r66100

Where there any other CI failures or thread/signal-safety
questions you would like me to look at?

I still haven't looked at fork-safety, yet (but I need to fix
my car again :<)

[ruby-core:90190] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Takashi Kokubun]

Hi Eric,

Thanks here again, your work is much appreciated.

> Where there any other CI failures or thread/signal-safety questions you would like me to look at?

I don't have those issues now, good job!
The current hardest pending thing for me around MJIT is test failures
on IO/fd, which you may be also interested in.
There are some skips for MJIT in following tests with CI failure url comments:

spec/ruby/core/io/initialize_spec.rb
spec/ruby/core/io/reopen_spec.rb
test/ruby/test_io.rb
test/ruby/test_process.rb

If you have some insight about them, please let me know.

> I still haven't looked at fork-safety, yet (but I need to fix my car again :<)

Do you mean fork-safety during postponed_job/workqueue registration?
Before fork, at least MJIT thread is stopped by my recent change. So
workqueue may not be touched.
I don't know what would work when async-signal executes postponed_job
during proceeding fork,
but that situation should be mostly the same as Ruby 2.5 as long as
MJIT worker thread is stopped.

Good luck for your car :)

k0kubun
2018年11月30日(金) 13:21 Eric Wong <normalperson@yhbt.net>:
>
> > https://80x24.org/spew/20181130023856.27983-1-e@80x24.org/raw
>
> Committed as r66100
>
> Where there any other CI failures or thread/signal-safety
> questions you would like me to look at?
>
> I still haven't looked at fork-safety, yet (but I need to fix
> my car again :<)

[ruby-core:90256] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Eric Wong]

Takashi Kokubun <takashikkbn@gmail.com> wrote:
> Hi Eric,
> 
> Thanks here again, your work is much appreciated.
> 
> > Where there any other CI failures or thread/signal-safety questions you would like me to look at?
> 
> I don't have those issues now, good job!

No problem.

> The current hardest pending thing for me around MJIT is test failures
> on IO/fd, which you may be also interested in.
> There are some skips for MJIT in following tests with CI failure url comments:

Thanks for the notes, I solved most of them.

> spec/ruby/core/io/initialize_spec.rb
> spec/ruby/core/io/reopen_spec.rb

Explained by r66159 (extremely unlikely to have real-world implications)

> test/ruby/test_io.rb

Haven't investigated much, weakref scares me :x

> test/ruby/test_process.rb

This one has real-world implications, testing this fix:
https://80x24.org/spew/20181203093534.4235-1-e@80x24.org/raw
(will commit the above within 24 hours, I hope)

I rejected this test-only fix, since I realized the real-world
implications:
https://80x24.org/spew/20181203074256.14307-1-e@80x24.org/raw

> If you have some insight about them, please let me know.
> 
> > I still haven't looked at fork-safety, yet (but I need to fix my car again :<)
> 
> Do you mean fork-safety during postponed_job/workqueue registration?
> Before fork, at least MJIT thread is stopped by my recent change. So
> workqueue may not be touched.

Taking a 30s look at rb_fork_ruby; I think mjit_pause will need to wait
for thread stop before fork.  Unfortunately, that slows down fork...

So the correct thing to do would be to reinitialize MJIT data structures
in mjit_child_after_fork (and not touch the parent at all).
That's how the current thread.c stuff works for mutexes and queues.

> I don't know what would work when async-signal executes postponed_job
> during proceeding fork,
> but that situation should be mostly the same as Ruby 2.5 as long as
> MJIT worker thread is stopped.

Right, postponed job data structure has never been fork-safe;
but not many people used it from signal handler.

> Good luck for your car :)

Thanks, it's an expensive mess :<

[ruby-core:90262] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Takashi Kokubun]

> Explained by r66159 (extremely unlikely to have real-world implications)

Gotcha. Thank you.
Though @mame indicated the existence of user program which closes all
fds prior to fork and suggested to let `rb_reserved_fd_p` know MJIT's
fd (only generated C file's fd hopefully?) in the future.

> This one has real-world implications, testing this fix:
> https://80x24.org/spew/20181203093534.4235-1-e@80x24.org/raw
> (will commit the above within 24 hours, I hope)

Hmm, still not understanding 100%. Do you mean gcc process that
inherits open fd of "tmp_script.cmd" is problematic for
Process.spawn("tmp_script.cmd")?
Anyway let's commit that and monitor test results. Thank you for working on it.

> I think mjit_pause will need to wait for thread stop before fork.  Unfortunately, that slows down fork...

This part didn't make sense to me. In rb_fork_ruby, at least MJIT
worker thread is stopped by mjit_pause(FALSE) before fork.
Which thread do you intent to stop before fork?

> Right, postponed job data structure has never been fork-safe; but not many people used it from signal handler.

I assume real-world postponed_job's user is almost only stackprof.gem
and it does not seem to use fork. So that should be fine.
2018年12月3日(月) 18:50 Eric Wong <normalperson@yhbt.net>:
>
> Takashi Kokubun <takashikkbn@gmail.com> wrote:
> > Hi Eric,
> >
> > Thanks here again, your work is much appreciated.
> >
> > > Where there any other CI failures or thread/signal-safety questions you would like me to look at?
> >
> > I don't have those issues now, good job!
>
> No problem.
>
> > The current hardest pending thing for me around MJIT is test failures
> > on IO/fd, which you may be also interested in.
> > There are some skips for MJIT in following tests with CI failure url comments:
>
> Thanks for the notes, I solved most of them.
>
> > spec/ruby/core/io/initialize_spec.rb
> > spec/ruby/core/io/reopen_spec.rb
>
> Explained by r66159 (extremely unlikely to have real-world implications)
>
> > test/ruby/test_io.rb
>
> Haven't investigated much, weakref scares me :x
>
> > test/ruby/test_process.rb
>
> This one has real-world implications, testing this fix:
> https://80x24.org/spew/20181203093534.4235-1-e@80x24.org/raw
> (will commit the above within 24 hours, I hope)
>
> I rejected this test-only fix, since I realized the real-world
> implications:
> https://80x24.org/spew/20181203074256.14307-1-e@80x24.org/raw
>
> > If you have some insight about them, please let me know.
> >
> > > I still haven't looked at fork-safety, yet (but I need to fix my car again :<)
> >
> > Do you mean fork-safety during postponed_job/workqueue registration?
> > Before fork, at least MJIT thread is stopped by my recent change. So
> > workqueue may not be touched.
>
> Taking a 30s look at rb_fork_ruby; I think mjit_pause will need to wait
> for thread stop before fork.  Unfortunately, that slows down fork...
>
> So the correct thing to do would be to reinitialize MJIT data structures
> in mjit_child_after_fork (and not touch the parent at all).
> That's how the current thread.c stuff works for mutexes and queues.
>
> > I don't know what would work when async-signal executes postponed_job
> > during proceeding fork,
> > but that situation should be mostly the same as Ruby 2.5 as long as
> > MJIT worker thread is stopped.
>
> Right, postponed job data structure has never been fork-safe;
> but not many people used it from signal handler.
>
> > Good luck for your car :)
>
> Thanks, it's an expensive mess :<

[ruby-core:90269] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Eric Wong]

Takashi Kokubun <takashikkbn@gmail.com> wrote:
> > Explained by r66159 (extremely unlikely to have real-world implications)
> 
> Gotcha. Thank you.
> Though @mame indicated the existence of user program which closes all
> fds prior to fork and suggested to let `rb_reserved_fd_p` know MJIT's
> fd (only generated C file's fd hopefully?) in the future.

Yes, that's a separate problem.

Since MJIT operates in parallel, rb_reserved_fd_p would have to
have locking to be consistent.  MJIT can also open a set of
stable FDs and reuse them indefinitely with truncate/ftruncate;
which may be the most performant option anyways.

> > This one has real-world implications, testing this fix:
> > https://80x24.org/spew/20181203093534.4235-1-e@80x24.org/raw
> > (will commit the above within 24 hours, I hope)
> 
> Hmm, still not understanding 100%. Do you mean gcc process that
> inherits open fd of "tmp_script.cmd" is problematic for
> Process.spawn("tmp_script.cmd")?
> Anyway let's commit that and monitor test results. Thank you for working on it.

Yes, the process intended for gcc (AFTER it calls vfork, but
BEFORE it calls "execve(gcc, ...)" will still see the open fd
for tmp_script.cmd.

FD_CLOEXEC can't protect us since it takes effect only when
execve happens.

> > I think mjit_pause will need to wait for thread stop before fork.  Unfortunately, that slows down fork...
> 
> This part didn't make sense to me. In rb_fork_ruby, at least MJIT
> worker thread is stopped by mjit_pause(FALSE) before fork.
> Which thread do you intent to stop before fork?

Wait, I missed that stop_worker() always waits for the thread :)

I only saw the lack of waiting on active_units...

However, waiting on stop_worker() can slow down Ruby-level fork,
especially since gcc is slow.  I don't think it's necessary to
wait at all (will work on it later)

> > Right, postponed job data structure has never been fork-safe; but not many people used it from signal handler.
> 
> I assume real-world postponed_job's user is almost only stackprof.gem
> and it does not seem to use fork. So that should be fine.

stackprof is meant to be used in existing programs, some may fork.

[ruby-core:90285] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Takashi Kokubun]

> I don't think it's necessary to wait at all (will work on it later)

You mean it's not necessary to stop MJIT worker thread, or suddenly
stopping MJIT worker thread without synchronization?
Could you describe more about what you're going to change?

By design, we don't re-construct MJIT-related objects (units,
dlopen-ed code, filenames, etc) on child Ruby thread after fork,
because we don't want to waste time to compile the same methods which
are already compiled before fork.
So we just start_worker (not mjit_init) in the child. Given that,
MJIT-related objects should not be intermediate state on fork, as MJIT
worker thread would suddenly disappear on fork in child Ruby thread if
we didn't mjit_pause it prior to fork. Suddenly stopping MJIT worker
thread without waiting may not work in the same reason.
2018年12月4日(火) 5:04 Eric Wong <normalperson@yhbt.net>:
>
> Takashi Kokubun <takashikkbn@gmail.com> wrote:
> > > Explained by r66159 (extremely unlikely to have real-world implications)
> >
> > Gotcha. Thank you.
> > Though @mame indicated the existence of user program which closes all
> > fds prior to fork and suggested to let `rb_reserved_fd_p` know MJIT's
> > fd (only generated C file's fd hopefully?) in the future.
>
> Yes, that's a separate problem.
>
> Since MJIT operates in parallel, rb_reserved_fd_p would have to
> have locking to be consistent.  MJIT can also open a set of
> stable FDs and reuse them indefinitely with truncate/ftruncate;
> which may be the most performant option anyways.
>
> > > This one has real-world implications, testing this fix:
> > > https://80x24.org/spew/20181203093534.4235-1-e@80x24.org/raw
> > > (will commit the above within 24 hours, I hope)
> >
> > Hmm, still not understanding 100%. Do you mean gcc process that
> > inherits open fd of "tmp_script.cmd" is problematic for
> > Process.spawn("tmp_script.cmd")?
> > Anyway let's commit that and monitor test results. Thank you for working on it.
>
> Yes, the process intended for gcc (AFTER it calls vfork, but
> BEFORE it calls "execve(gcc, ...)" will still see the open fd
> for tmp_script.cmd.
>
> FD_CLOEXEC can't protect us since it takes effect only when
> execve happens.
>
> > > I think mjit_pause will need to wait for thread stop before fork.  Unfortunately, that slows down fork...
> >
> > This part didn't make sense to me. In rb_fork_ruby, at least MJIT
> > worker thread is stopped by mjit_pause(FALSE) before fork.
> > Which thread do you intent to stop before fork?
>
> Wait, I missed that stop_worker() always waits for the thread :)
>
> I only saw the lack of waiting on active_units...
>
> However, waiting on stop_worker() can slow down Ruby-level fork,
> especially since gcc is slow.  I don't think it's necessary to
> wait at all (will work on it later)
>
> > > Right, postponed job data structure has never been fork-safe; but not many people used it from signal handler.
> >
> > I assume real-world postponed_job's user is almost only stackprof.gem
> > and it does not seem to use fork. So that should be fine.
>
> stackprof is meant to be used in existing programs, some may fork.

[ruby-core:90286] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Eric Wong]

Takashi Kokubun <takashikkbn@gmail.com> wrote:
> > I don't think it's necessary to wait at all (will work on it later)
> 
> You mean it's not necessary to stop MJIT worker thread, or suddenly
> stopping MJIT worker thread without synchronization?
> Could you describe more about what you're going to change?

It's not necessary to stop MJIT worker thread.  We already
support regular Threads without stopping them (GVL makes it
easier, I admit).  At least, we will need to reinitialize all
the mutex and condvars to keep them in a consistent state.

It's very tricky to deal with fork after threads are created,
and we may need to accept there will be memory leaks in each
successive generation of forked children if a Ruby process
leapfrogs without exec (uncommon)[*]

> By design, we don't re-construct MJIT-related objects (units,
> dlopen-ed code, filenames, etc) on child Ruby thread after fork,
> because we don't want to waste time to compile the same methods which
> are already compiled before fork.

OK.  The key is the linked-lists and internal structures need
to remain consistent (no half-modified stuff in child)

So maybe the Ruby thread needs to acquire MJIT critical section
on fork.  But it looks like there's some accesses to .length
and other stuff in MJIT which isn't protected by critical section.

> So we just start_worker (not mjit_init) in the child. Given that,
> MJIT-related objects should not be intermediate state on fork, as MJIT
> worker thread would suddenly disappear on fork in child Ruby thread if
> we didn't mjit_pause it prior to fork. Suddenly stopping MJIT worker
> thread without waiting may not work in the same reason.

Right, so stopping worker seems OK.  But that means the Ruby
thread is potentially blocked waiting for gcc to finish, which
can take a long time...  I suppose it's acceptable for 2.6


[*] Current pthread implementation already leaks some memory
    due to condvars being initialized while busy.  It is pretty
    unrealistic for a real-world program to do this, though:

==> fork_leapfrog.rb <==
# This causes monotonically increasing growth on Linux NPTL
# see thread.c::thread_cleanup_func comment
require 'io/wait'
Dir.chdir '/proc'
prev = 0
loop do
  pid = fork
  exit!(0) if pid

  # show the number of 4K pages used (Linux-only)
  n = File.read("#$$/statm").split(-' ')[1].to_i
  if n > prev
    # be patient with this, sometimes there's no output for
    # many iterations, but it comes back
    puts "#{prev} => #{n}"
    prev = n
  end

  # since Ctrl-C from a terminal can't stop this loop,
  # allow the user to just hit any key to stop
  break if STDIN.wait(0)
end

The above growth only affects fork WITHOUT exec, so some
servers which do in-place upgrades aren't affected.

[ruby-core:90288] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Eric Wong]

Btw, I think pch_file needs to be hard-linked or copied for
child process (and pch_owner_pid needs to be updated) in
mjit_child_after_fork, since a child may outlive the parent.

[ruby-core:90307] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Takashi Kokubun]

Oh, I didn't care about that. That sounds true.
2018年12月5日(水) 5:29 Eric Wong <normalperson@yhbt.net>:
>
> Btw, I think pch_file needs to be hard-linked or copied for
> child process (and pch_owner_pid needs to be updated) in
> mjit_child_after_fork, since a child may outlive the parent.

[ruby-core:90308] Re: [ruby-cvs:73101] k0kubun:r66001 (trunk): vm_trace.c: MJIT-limited thread-safety for postponed_job

[Takashi Kokubun]

Rather than updating pch_owner_pid, I would like to delete the
variable and just copy it with a different name (and update only
pch_file).
2018年12月5日(水) 5:29 Eric Wong <normalperson@yhbt.net>:
>
> Btw, I think pch_file needs to be hard-linked or copied for
> child process (and pch_owner_pid needs to be updated) in
> mjit_child_after_fork, since a child may outlive the parent.