From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C593C469719 for ; Thu, 24 Sep 2020 15:54:13 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) From: "sergos@tarantool.org" In-Reply-To: <12d3cc3577e45946a82c8d23b69508a4c5653346.1600862684.git.imun@tarantool.org> Date: Thu, 24 Sep 2020 15:54:11 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <12d3cc3577e45946a82c8d23b69508a4c5653346.1600862684.git.imun@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 1/2] fiber: introduce a callback for fibers switch-over List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Hi! Thank you for the patch, see my single comment below. Regards, Sergos > On 23 Sep 2020, at 22:06, Igor Munkin wrote: >=20 > 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. >=20 > To solve this severe integration drawback function is > introduced. This routine encloses the checks and actions to be done = when > the running fiber yields the execution. >=20 > 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) undefined symbol. >=20 > Part of #1700 > Relates to #4491 >=20 > Signed-off-by: Igor Munkin > --- >=20 > 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% >=20 > 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 >=20 > 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 /* __rdtscp() */ >=20 > +extern void cord_on_yield(void); > + The definition is under ENABLE_FIBER_TOP, while its use isn=E2=80=99t.=20= 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 =3D caller; > callee->flags |=3D 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); >=20 > assert(callee->flags & FIBER_IS_READY || callee =3D=3D = &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 =3D 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) >=20 > 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) >=20 > -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) >=20 > -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) >=20 > 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 () >=20 > -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) >=20 > -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) >=20 > -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) >=20 > -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) >=20 > -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) >=20 > -add_executable(cbus.test cbus.c) > +add_executable(cbus.test cbus.c core_test_utils.c) > target_link_libraries(cbus.test core unit stat) >=20 > 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 () >=20 > -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) >=20 > if (ENABLE_BUNDLED_MSGPUCK) > @@ -132,7 +132,7 @@ if (ENABLE_BUNDLED_MSGPUCK) > target_link_libraries(msgpack.test ${MSGPUCK_LIBRARIES}) > endif () >=20 > -add_executable(scramble.test scramble.c) > +add_executable(scramble.test scramble.c core_test_utils.c) > target_link_libraries(scramble.test scramble) >=20 > 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}) >=20 > -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}) >=20 > -add_executable(say.test say.c) > +add_executable(say.test say.c core_test_utils.c) > target_link_libraries(say.test core unit) >=20 > 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) >=20 > -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}) >=20 > 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}) >=20 > add_executable(column_mask.test > - column_mask.c) > + column_mask.c > + core_test_utils.c) > target_link_libraries(column_mask.test tuple unit) >=20 > 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}) >=20 > -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}) >=20 > -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}) >=20 > -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) >=20 > add_executable(checkpoint_schedule.test > @@ -235,23 +238,23 @@ add_executable(checkpoint_schedule.test > ) > target_link_libraries(checkpoint_schedule.test m unit) >=20 > -add_executable(sio.test sio.c) > +add_executable(sio.test sio.c core_test_utils.c) > target_link_libraries(sio.test unit core) >=20 > -add_executable(crypto.test crypto.c) > +add_executable(crypto.test crypto.c core_test_utils.c) > target_link_libraries(crypto.test crypto unit) >=20 > 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) >=20 > 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) >=20 > 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) >=20 > 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) >=20 > -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 ``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 > + * 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) > +{ > +} > --=20 > 2.25.0 >=20