Re: [Tarantool-patches] [PATCH luajit 1/2] jit: abort trace recording and execution for C API

[Sergey Ostanevich <sergos@tarantool.org>]

On 28 мар 23:30, Igor Munkin wrote:
> Sergos,
> 
> Thanks for you review!
> 
> On 28.03.20, Sergey Ostanevich wrote:
> > Hi!
> > 
> > Thanks for the patch, see below.
> > On 27 мар 13:47, Igor Munkin wrote:
> > > JIT recording semantics assumes FFI calls are atomic regarding the
> > > LuaJIT VM: if the execution exited Lua world through FFI it is not
> > > re-entering Lua world again.
> > > 
> > > However, there are ways to break this atomicity via FFI: one can
> > > re-enter LuaJIT VM via Lua C API inside the C routine called via FFI. As
> > > a result the following stack mix is created:
> > > | Lua-FFI -> С routine -> Lua-C API -> Lua VM
> 
> Fixed typo <D0><A1> (cyrillic С letter) to an ASCII C.
> 
> > > This sort of re-entrancy is explicitly not supported by LuaJIT compiler
> > > machinery. @mraleph named such layout an "FFI sandwich" and I'm strictly
> > > against eating it.
> > > 
> > > E.g. there is a trigger machinery in Tarantool calling Lua functions
> > > from C space. As a result of FFI function call with a trigger underneath
> > > the "FFI sandwich" is created.
> > > 
> > > This changeset introduces the mechanism for Lua-C API callbacks similar
> > > to the one existing for Lua-FFI: trace recording is aborted whe the
> >                                                               when
> 
> Thanks, fixed.
> 
> > > execution re-enters LuaJIT VM. Sandwich detection while mcode execution
> > > leads to platform panic.
> > Could you elaborate here in comment on this a little: it means this 
> > particular Lua call to be aborted, fail for the fiber Lua state, or for 
> > the whole Tarantool?
> > 
> 
> The whole Tarantool finishes its execution with EXIT_FAILURE code and
> calls tarantool_panic_handler routine prior to the exit. Should I add
> this remark into commit message?
I would like to see it in commit message.

> 
> > > 
> > > Relates to tarantool/tarantool#4427
> > > 
> > > Co-authored-by: Vyacheslav Egorov <vegorov@google.com>
> > > Co-authored-by: Sergey Ostanevich <sergos@tarantool.org>
> > > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > > ---
> > >  src/lj_api.c                             | 35 ++++++++++----
> > >  src/lj_errmsg.h                          |  1 +
> > >  test/gh-4427-ffi-sandwich/CMakeLists.txt |  1 +
> > >  test/gh-4427-ffi-sandwich/libsandwich.c  | 59 ++++++++++++++++++++++++
> > >  test/gh-4427-ffi-sandwich/test.lua       | 26 +++++++++++
> > >  5 files changed, 113 insertions(+), 9 deletions(-)
> > >  create mode 100644 test/gh-4427-ffi-sandwich/CMakeLists.txt
> > >  create mode 100644 test/gh-4427-ffi-sandwich/libsandwich.c
> > >  create mode 100644 test/gh-4427-ffi-sandwich/test.lua
> > > 
> > > diff --git a/src/lj_api.c b/src/lj_api.c
> > > index a5e2465..c1f53e0 100644
> > > --- a/src/lj_api.c
> > > +++ b/src/lj_api.c
> > > @@ -76,6 +76,17 @@ static GCtab *getcurrenv(lua_State *L)
> > >    return fn->c.gct == ~LJ_TFUNC ? tabref(fn->c.env) : tabref(L->env);
> > >  }
> > >  
> > > +static void call(lua_State *L, TValue *base, int nres) {
> > I would recommend to name the function in a more descriptive way, like
> > sane_call. 
> 
> IMHO no_trace_call / no_jit_call looks good to me, but feel free to
> propose the naming you prefer more.
If you want to mention that this call aborts the trace generation,
then I would put it like 'call_w_trace_abort'
But it goes beyond that AFAU. Can you name it like 'ffi_safe_call'?
Although, safety resulting in a panic sounds paradoxical. 

BTW, should it be under 'LJ_HASFFI' and keep the original call
otherwise?

> 
> > 
> > > +  global_State *g = G(L);
> > > +  if (tvref(g->jit_base)) {
> > > +    setstrV(L, L->top++, lj_err_str(L, LJ_ERR_JITCALL));
> > > +    if (g->panic) g->panic(L);
> > > +    exit(EXIT_FAILURE);
> > > +  }
> > > +  lj_trace_abort(g);  /* Never record across Lua VM entrance */
> > > +  lj_vm_call(L, base, nres);
> > > +}
> > > +
> > >  /* -- Miscellaneous API functions ----------------------------------------- */
> > >  
> > >  LUA_API int lua_status(lua_State *L)
> > 
> > Also I have a question on perf impact - have you measured any
> > benchmarks - Lua only, or as part of Tarantool
> 
> I compared two sysbench logs ([1] and [2]) via vimdiff and see no
> critical difference.
> 
> > 
> > Regards,
> > Sergos
> 
> [1]: https://gitlab.com/tarantool/tarantool/-/jobs/489420257/raw
> [2]: https://gitlab.com/tarantool/tarantool/-/jobs/486707332/raw
> 
> -- 
> Best regards,
> IM