Re: [Tarantool-patches] [PATCH luajit] sysprof: move symtab update into profile hook

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the patch!
Please, consider my comments below.

On 23.05.23, Maxim Kokryashkin wrote:
> Before the patch, the symtab update was done in the signal
> handler. That update requires memory allocation, which
> can't be done safely in a signal handler. This patch reuses
> LuaJIT's HOOK_PROFILE for symtab update routine execution, so
> it is now possible to update it safely after the signal
> handler exit.
> 
> Resolves tarantool/tarantool#8140
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-8140-sysprof-allocator-crash
> Issue: https://github.com/tarantool/tarantool/issues/8140
> PR: https://github.com/tarantool/tarantool/pull/8691
> 
>  src/lj_dispatch.c                             | 10 ++++
>  src/lj_sysprof.c                              | 34 +++++++++++++-
>  src/lj_sysprof.h                              |  4 ++
>  test/tarantool-tests/CMakeLists.txt           |  1 +
>  .../gh-8140-sysprof-allocator-crash.test.lua  | 46 +++++++++++++++++++
>  .../CMakeLists.txt                            |  9 ++++
>  .../sysprofalloc.c                            | 17 +++++++
>  7 files changed, 119 insertions(+), 2 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua
>  create mode 100644 test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt
>  create mode 100644 test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c
> 
> diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
> index ee735450..65948a0c 100644
> --- a/src/lj_dispatch.c
> +++ b/src/lj_dispatch.c

<snipped>

> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
> index 2e9ed9b3..5b5cbe2a 100644
> --- a/src/lj_sysprof.c
> +++ b/src/lj_sysprof.c
> @@ -76,6 +76,7 @@ struct sysprof {
>    lj_profile_timer timer; /* Profiling timer. */
>    int saved_errno; /* Saved errno when profiler failed. */
>    uint32_t lib_adds; /* Number of libs loaded. Monotonic. */
> +  volatile sig_atomic_t symtab_update_needed; /* Symtab update request flag. */
>  };
>  /*
>  ** XXX: Only one VM can be profiled at a time.
> @@ -88,6 +89,36 @@ static struct sysprof sysprof = {0};
>  static const uint8_t ljp_header[] = {'l', 'j', 'p', LJP_FORMAT_VERSION,
>                                        0x0, 0x0, 0x0};
>  
> +void lj_symtab_update_hook(lua_State *L) {
> +  struct sysprof *sp = &sysprof;
> +  global_State *g = G(L);
> +  uint8_t mask;
> +  mask = (g->hookmask & ~HOOK_PROFILE);

Nit: I suppose, that two lines above may be joined.

> +  sp->symtab_update_needed = 0;
> +  if (!(mask & HOOK_VMEVENT)) {

Does it mean, that if we have signal inside VMEVENT we don't update our
symtab table (since flag is reset) (*)? Should we move the line
| sp->symtab_update_needed = 0;
inside if condition?


> +    g->hookmask = HOOK_VMEVENT;
> +    lj_dispatch_update(g);
> +    lj_symtab_dump_newc(&sp->lib_adds, &sp->out, LJP_SYMTAB_CFUNC_EVENT, L);
> +  }
> +  g->hookmask = mask;
> +  lj_dispatch_update(g);
> +}
> +
> +int lj_symtab_update_requested() {
> +  struct sysprof *sp = &sysprof;
> +  return sp->symtab_update_needed;
> +}
> +
> +static void setup_symtab_update_hook(struct sysprof *sp) {
> +  global_State *g = sp->g;
> +  uint8_t mask = g->hookmask;
> +  if (!(mask & (HOOK_PROFILE|HOOK_VMEVENT|HOOK_GC))) {

Why GC hook is mentioned here?

> +    sp->symtab_update_needed = 1;
> +    g->hookmask = (mask | HOOK_PROFILE);
> +    lj_dispatch_update(g);
> +  }

The similar to (*) question here: If we have a signal inside VMEVENT, does
this mean that there is no symbols to load for sure?
If so, coment is highly desired.

> +}
> +
>  static int stream_is_needed(struct sysprof *sp)
>  {
>    return sp->opt.mode != LUAM_SYSPROF_DEFAULT;
> @@ -240,8 +271,7 @@ static void stream_guest(struct sysprof *sp, uint32_t vmstate)

<snipped>

> diff --git a/src/lj_sysprof.h b/src/lj_sysprof.h
> index 7e8c2e6e..456ca76f 100644
> --- a/src/lj_sysprof.h
> +++ b/src/lj_sysprof.h
> @@ -108,4 +108,8 @@ void lj_sysprof_add_proto(const struct GCproto *pt);

<snipped>

> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index a428d009..25244733 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt

<snipped>

> diff --git a/test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua b/test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua
> new file mode 100644
> index 00000000..afce83e2
> --- /dev/null
> +++ b/test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua
> @@ -0,0 +1,46 @@
> +local tap = require('tap')
> +local test = tap.test('gh-8140-sysprof-allocator-crash'):skipcond({
> +  ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
> +                                               jit.arch ~= "x64",
> +  ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
> +})

I've reverted your changes and test still passes before the patch:

| /home/burii/reviews/luajit/gh-8140-sysprof-in-alloc/test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua ............. ok   
| All tests successful.

I've restored the sysprof.[ch] and dispatch changes, and leave tests as
is.

| gst
| On branch fckxorg/gh-8140-sysprof-allocator-crash
| Your branch is behind 'origin/fckxorg/gh-8140-sysprof-allocator-crash' by 1 commit, and can be fast-forwarded.
|   (use "git pull" to update your local branch)
|
| Changes to be committed:
|   (use "git restore --staged <file>..." to unstage)
|         modified:   test/tarantool-tests/CMakeLists.txt
|         new file:   test/tarantool-tests/gh-8140-sysprof-allocator-crash.test.lua
|         new file:   test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt
|         new file:   test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c


I've tried several times and test still passes.

> +test:plan(2)
> +
> +local profilename = require("utils").profilename
> +local profile = require('jit.profile')
> +
> +local TMP_BINFILE = profilename("sysprofdata.tmp.bin")
> +local callback_called = false
> +
> +local function payload()
> +  local r = 0
> +  for i = 1, 1e8 do
> +    r = r + i
> +  end
> +  return r
> +end

May be we should use more GC-specific payload?

> +
> +local function callback(_, _, _)
> +    callback_called = true
> +end
> +
> +profile.start('f', callback)
> +payload()
> +profile.stop()
> +
> +test:ok(callback_called, 'LuaJIT profiler callback was not called.')

Looks like it should be "was called"?

> +
> +jit.off()
> +misc.sysprof.start{mode='C', interval=1, path=TMP_BINFILE}
> +for _ = 1, 1e4 do
> +    require('sysprofalloc').get_string()

Why do you try to require the module several times?
Is it saved in `package.loaded` anyway?

> +    -- Make sure that C library is collected, so it will be loaded
> +    -- again on the next iteration.
> +    collectgarbage()
> +end
> +misc.sysprof.stop()
> +
> +test:ok(true, 'Sysprof has crashed.')

Looks like it should be "has not crashed"?

> +
> +os.remove(TMP_BINFILE)
> +os.exit(test:check() and 0 or 1)
> diff --git a/test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt b/test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt
> new file mode 100644
> index 00000000..dbe60219
> --- /dev/null
> +++ b/test/tarantool-tests/gh-8140-sysprof-allocator-crash/CMakeLists.txt
> @@ -0,0 +1,9 @@
> +if (NOT(CMAKE_SYSTEM_NAME STREQUAL "Darwin"))
> +  BuildTestCLib(sysprofalloc sysprofalloc.c)
> +  # Unfortunately, <target_link_options> command is introduced
> +  # since CMake 3.13, so we can't use it now considering ancient
> +  # distros support. Just build linker flags by hands.
> +  set(CMAKE_SHARED_LINKER_FLAGS
> +    "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--hash-style=both"

Do we need both hashes here?

> +  )
> +endif()
> diff --git a/test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c b/test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c
> new file mode 100644
> index 00000000..d3c41d42
> --- /dev/null
> +++ b/test/tarantool-tests/gh-8140-sysprof-allocator-crash/sysprofalloc.c
> @@ -0,0 +1,17 @@
> +#include <lua.h>
> +#include <lauxlib.h>
> +
> +int get_string(lua_State *L) {
> +    lua_pushstring(L, "test string");
> +    return 1;

Something strange with indentation: please, use tabs for __our__ C
code in tests.

> +}
> +
> +static const struct luaL_Reg sysprofalloc [] = {
> +    {"get_string", get_string},
> +    {NULL, NULL}
> +};
> +
> +int luaopen_sysprofalloc(lua_State *L) {
> +    luaL_register(L, "sysprofalloc", sysprofalloc);
> +    return 1;
> +}
> -- 
> 2.40.1
> 

-- 
Best regards,
Sergey Kaplun