Re: [Tarantool-patches] [PATCH v2 1/2] fiber: introduce a callback for fibers switch-over

["sergos@tarantool.org" <sergos@tarantool.org>]

Hi!

Thank you for the patch, see my single comment below.

Regards,
Sergos

> On 23 Sep 2020, at 22:06, Igor Munkin <imun@tarantool.org> wrote:
> 
> Tarantool integrates several complex environments together and there are
> issues occurring at their junction leading to the platform failures.
> E.g. fiber switch-over is implemented outside the Lua world, so when one
> lua_State substitutes another one, main LuaJIT engines, such as JIT and
> GC, are left unnotified leading to the further platform misbehaviour.
> 
> To solve this severe integration drawback <cord_on_yield> function is
> introduced. This routine encloses the checks and actions to be done when
> the running fiber yields the execution.
> 
> Unfortunately the way callback is implemented introduces a circular
> dependency. Considering linker symbol resolving methods for static build
> an auxiliary translation unit is added to the particular tests mocking
> (i.e. exporting) <cord_on_yield> undefined symbol.
> 
> Part of #1700
> Relates to #4491
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Here are also the benchmark results for the Release build:
> * Vanilla (8477b6c) -> Patched (86bfd45) (min, median, mean, max):
> | fibers: 10; iters: 100	0%	0%	0%	-3%
> | fibers: 10; iters: 1000	2%	2%	1%	-1%
> | fibers: 10; iters: 10000	6%	4%	4%	5%
> | fibers: 10; iters: 100000	4%	3%	3%	-2%
> | fibers: 100; iters: 100	0%	0%	0%	0%
> | fibers: 100; iters: 1000	0%	0%	0%	-8%
> | fibers: 100; iters: 10000	1%	0%	0%	1%
> | fibers: 100; iters: 100000	0%	0%	1%	2%
> | fibers: 1000; iters: 100	0%	0%	0%	0%
> | fibers: 1000; iters: 1000	0%	0%	0%	0%
> | fibers: 1000; iters: 10000	0%	0%	0%	1%
> | fibers: 1000; iters: 100000	0%	0%	0%	0%
> | fibers: 10000; iters: 100	-1%	-3%	-1%	0%
> | fibers: 10000; iters: 1000	0%	0%	0%	0%
> | fibers: 10000; iters: 10000	0%	0%	0%	0%
> | fibers: 10000; iters: 100000	0%	-1%	-2%	-6%
> 
> src/lib/core/fiber.c        | 10 +++++++
> src/lua/utils.c             |  8 +++++
> test/unit/CMakeLists.txt    | 59 +++++++++++++++++++------------------
> test/unit/core_test_utils.c | 37 +++++++++++++++++++++++
> 4 files changed, 86 insertions(+), 28 deletions(-)
> create mode 100644 test/unit/core_test_utils.c
> 
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index 483ae3ce1..1b7d255ff 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -46,6 +46,8 @@
> #if ENABLE_FIBER_TOP
> #include <x86intrin.h> /* __rdtscp() */
> 

> +extern void cord_on_yield(void);
> +

The definition is under ENABLE_FIBER_TOP, while its use isn’t. 
The build will fail for ARM target for exmaple.
AFAICS the changes are not releveant to the fiber top in any way.

> static inline void
> clock_stat_add_delta(struct clock_stat *stat, uint64_t clock_delta)
> {
> @@ -416,6 +418,10 @@ fiber_call(struct fiber *callee)
> 	/** By convention, these triggers must not throw. */
> 	if (! rlist_empty(&caller->on_yield))
> 		trigger_run(&caller->on_yield, NULL);
> +
> +	if (cord_is_main())
> +		cord_on_yield();
> +
> 	clock_set_on_csw(caller);
> 	callee->caller = caller;
> 	callee->flags |= FIBER_IS_READY;
> @@ -645,6 +651,10 @@ fiber_yield(void)
> 	/** By convention, these triggers must not throw. */
> 	if (! rlist_empty(&caller->on_yield))
> 		trigger_run(&caller->on_yield, NULL);
> +
> +	if (cord_is_main())
> +		cord_on_yield();
> +
> 	clock_set_on_csw(caller);
> 
> 	assert(callee->flags & FIBER_IS_READY || callee == &cord->sched);
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index af114b0a2..8e98f7607 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -1308,3 +1308,11 @@ tarantool_lua_utils_init(struct lua_State *L)
> 	luaT_newthread_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> 	return 0;
> }
> +
> +/**
> + * This routine encloses the checks and actions to be done when
> + * the running fiber yields the execution.
> + */
> +void cord_on_yield(void)
> +{
> +}
> diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
> index 419477748..aace8cf50 100644
> --- a/test/unit/CMakeLists.txt
> +++ b/test/unit/CMakeLists.txt
> @@ -50,7 +50,7 @@ add_executable(bitset_index.test bitset_index.c)
> target_link_libraries(bitset_index.test bitset)
> add_executable(base64.test base64.c)
> target_link_libraries(base64.test misc unit)
> -add_executable(uuid.test uuid.c)
> +add_executable(uuid.test uuid.c core_test_utils.c)
> target_link_libraries(uuid.test uuid unit)
> 
> add_executable(bps_tree.test bps_tree.cc)
> @@ -69,53 +69,53 @@ add_executable(bloom.test bloom.cc)
> target_link_libraries(bloom.test salad)
> add_executable(vclock.test vclock.cc)
> target_link_libraries(vclock.test vclock unit)
> -add_executable(xrow.test xrow.cc)
> +add_executable(xrow.test xrow.cc core_test_utils.c)
> target_link_libraries(xrow.test xrow unit)
> add_executable(decimal.test decimal.c)
> target_link_libraries(decimal.test core unit)
> -add_executable(mp_error.test mp_error.cc)
> +add_executable(mp_error.test mp_error.cc core_test_utils.c)
> target_link_libraries(mp_error.test box_error core unit)
> 
> -add_executable(fiber.test fiber.cc)
> +add_executable(fiber.test fiber.cc core_test_utils.c)
> set_source_files_properties(fiber.cc PROPERTIES COMPILE_FLAGS -O0)
> target_link_libraries(fiber.test core unit)
> 
> -add_executable(fiber_stack.test fiber_stack.c)
> +add_executable(fiber_stack.test fiber_stack.c core_test_utils.c)
> set_source_files_properties(fiber_stack.c PROPERTIES COMPILE_FLAGS -O0)
> target_link_libraries(fiber_stack.test core unit)
> 
> if (NOT ENABLE_GCOV)
>     # This test is known to be broken with GCOV
> -    add_executable(guard.test guard.cc)
> +    add_executable(guard.test guard.cc core_test_utils.c)
>     target_link_libraries(guard.test core unit)
> endif ()
> 
> -add_executable(fiber_stress.test fiber_stress.cc)
> +add_executable(fiber_stress.test fiber_stress.cc core_test_utils.c)
> target_link_libraries(fiber_stress.test core)
> 
> -add_executable(fiber_cond.test fiber_cond.c unit.c)
> +add_executable(fiber_cond.test fiber_cond.c unit.c core_test_utils.c)
> target_link_libraries(fiber_cond.test core)
> 
> -add_executable(fiber_channel.test fiber_channel.cc unit.c)
> +add_executable(fiber_channel.test fiber_channel.cc unit.c core_test_utils.c)
> target_link_libraries(fiber_channel.test core)
> 
> -add_executable(fiber_channel_stress.test fiber_channel_stress.cc)
> +add_executable(fiber_channel_stress.test fiber_channel_stress.cc core_test_utils.c)
> target_link_libraries(fiber_channel_stress.test core)
> 
> -add_executable(cbus_stress.test cbus_stress.c)
> +add_executable(cbus_stress.test cbus_stress.c core_test_utils.c)
> target_link_libraries(cbus_stress.test core stat)
> 
> -add_executable(cbus.test cbus.c)
> +add_executable(cbus.test cbus.c core_test_utils.c)
> target_link_libraries(cbus.test core unit stat)
> 
> include(CheckSymbolExists)
> check_symbol_exists(__GLIBC__ features.h GLIBC_USED)
> if (GLIBC_USED)
> -    add_executable(cbus_hang.test cbus_hang.c)
> +    add_executable(cbus_hang.test cbus_hang.c core_test_utils.c)
>     target_link_libraries(cbus_hang.test core unit stat)
> endif ()
> 
> -add_executable(coio.test coio.cc)
> +add_executable(coio.test coio.cc core_test_utils.c)
> target_link_libraries(coio.test core eio bit uri unit)
> 
> if (ENABLE_BUNDLED_MSGPUCK)
> @@ -132,7 +132,7 @@ if (ENABLE_BUNDLED_MSGPUCK)
>     target_link_libraries(msgpack.test ${MSGPUCK_LIBRARIES})
> endif ()
> 
> -add_executable(scramble.test scramble.c)
> +add_executable(scramble.test scramble.c core_test_utils.c)
> target_link_libraries(scramble.test scramble)
> 
> add_executable(guava.test guava.c)
> @@ -155,7 +155,7 @@ target_link_libraries(csv.test csv)
> add_executable(json.test json.c)
> target_link_libraries(json.test json unit ${ICU_LIBRARIES})
> 
> -add_executable(rmean.test rmean.cc)
> +add_executable(rmean.test rmean.cc core_test_utils.c)
> target_link_libraries(rmean.test stat unit)
> add_executable(histogram.test histogram.c)
> target_link_libraries(histogram.test stat unit)
> @@ -170,7 +170,7 @@ target_link_libraries(luaL_iterator.test unit server coll core misc
>     ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES}
>     ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES} ${LIB_DL})
> 
> -add_executable(say.test say.c)
> +add_executable(say.test say.c core_test_utils.c)
> target_link_libraries(say.test core unit)
> 
> set(ITERATOR_TEST_SOURCES
> @@ -182,7 +182,7 @@ set(ITERATOR_TEST_SOURCES
>     ${PROJECT_SOURCE_DIR}/src/box/vy_cache.c)
> set(ITERATOR_TEST_LIBS core tuple xrow unit)
> 
> -add_executable(vy_mem.test vy_mem.c ${ITERATOR_TEST_SOURCES})
> +add_executable(vy_mem.test vy_mem.c ${ITERATOR_TEST_SOURCES} core_test_utils.c)
> target_link_libraries(vy_mem.test ${ITERATOR_TEST_LIBS} ${LIB_DL})
> 
> add_executable(vy_point_lookup.test
> @@ -204,11 +204,13 @@ add_executable(vy_point_lookup.test
>     ${PROJECT_SOURCE_DIR}/src/box/index_def.c
>     ${PROJECT_SOURCE_DIR}/src/box/schema_def.c
>     ${PROJECT_SOURCE_DIR}/src/box/identifier.c
> +    core_test_utils.c
> )
> target_link_libraries(vy_point_lookup.test core tuple xrow xlog unit ${LIB_DL})
> 
> add_executable(column_mask.test
> -    column_mask.c)
> +    column_mask.c
> +    core_test_utils.c)
> target_link_libraries(column_mask.test tuple unit)
> 
> add_executable(vy_write_iterator.test
> @@ -217,16 +219,17 @@ add_executable(vy_write_iterator.test
>     ${PROJECT_SOURCE_DIR}/src/box/vy_upsert.c
>     ${PROJECT_SOURCE_DIR}/src/box/vy_write_iterator.c
>     ${ITERATOR_TEST_SOURCES}
> +    core_test_utils.c
> )
> target_link_libraries(vy_write_iterator.test xlog ${ITERATOR_TEST_LIBS} ${LIB_DL})
> 
> -add_executable(vy_cache.test vy_cache.c ${ITERATOR_TEST_SOURCES})
> +add_executable(vy_cache.test vy_cache.c ${ITERATOR_TEST_SOURCES} core_test_utils.c)
> target_link_libraries(vy_cache.test ${ITERATOR_TEST_LIBS} ${LIB_DL})
> 
> -add_executable(coll.test coll.cpp)
> +add_executable(coll.test coll.cpp core_test_utils.c)
> target_link_libraries(coll.test coll unit misc ${LIB_DL})
> 
> -add_executable(tuple_bigref.test tuple_bigref.c)
> +add_executable(tuple_bigref.test tuple_bigref.c core_test_utils.c)
> target_link_libraries(tuple_bigref.test tuple unit)
> 
> add_executable(checkpoint_schedule.test
> @@ -235,23 +238,23 @@ add_executable(checkpoint_schedule.test
> )
> target_link_libraries(checkpoint_schedule.test m unit)
> 
> -add_executable(sio.test sio.c)
> +add_executable(sio.test sio.c core_test_utils.c)
> target_link_libraries(sio.test unit core)
> 
> -add_executable(crypto.test crypto.c)
> +add_executable(crypto.test crypto.c core_test_utils.c)
> target_link_libraries(crypto.test crypto unit)
> 
> add_executable(swim.test swim.c swim_test_transport.c swim_test_ev.c
> -               swim_test_utils.c ${PROJECT_SOURCE_DIR}/src/version.c)
> +               swim_test_utils.c ${PROJECT_SOURCE_DIR}/src/version.c core_test_utils.c)
> target_link_libraries(swim.test unit swim)
> 
> add_executable(swim_proto.test swim_proto.c swim_test_transport.c swim_test_ev.c
> -               swim_test_utils.c ${PROJECT_SOURCE_DIR}/src/version.c)
> +               swim_test_utils.c ${PROJECT_SOURCE_DIR}/src/version.c core_test_utils.c)
> target_link_libraries(swim_proto.test unit swim)
> 
> add_executable(swim_errinj.test swim_errinj.c swim_test_transport.c
>                swim_test_ev.c swim_test_utils.c
> -               ${PROJECT_SOURCE_DIR}/src/version.c)
> +               ${PROJECT_SOURCE_DIR}/src/version.c core_test_utils.c)
> target_link_libraries(swim_errinj.test unit swim)
> 
> add_executable(merger.test merger.test.c)
> @@ -264,5 +267,5 @@ target_link_libraries(snap_quorum_delay.test box core unit)
> # Client for popen.test
> add_executable(popen-child popen-child.c)
> 
> -add_executable(popen.test popen.c)
> +add_executable(popen.test popen.c core_test_utils.c)
> target_link_libraries(popen.test misc unit core)
> diff --git a/test/unit/core_test_utils.c b/test/unit/core_test_utils.c
> new file mode 100644
> index 000000000..23452bbfd
> --- /dev/null
> +++ b/test/unit/core_test_utils.c
> @@ -0,0 +1,37 @@
> +/*
> + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +/**
> + * Mock function to resolve circular dependencies in unit tests
> + */
> +void cord_on_yield(void)
> +{
> +}
> -- 
> 2.25.0
>