[Tarantool-patches] [PATCH luajit] sysprof: move symtab update into profile hook
Sergey Kaplun
skaplun at tarantool.org
Wed May 24 12:46:34 MSK 2023
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
More information about the Tarantool-patches
mailing list