From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id D753C6EC56; Sat, 20 Mar 2021 03:49:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D753C6EC56 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616201368; bh=1VGRwXNwFzeWushNEAteCuSQkSv5+ObqqdMlZXusqQk=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=Zm8S8vPkTTggetCrQjzlpVZteif1+aHeRBmWhL+dLcZbepFz52buS2XISmbCGggqU gKfY3S8VhoKEfaAxwv/DQsFp2lmfRYTgF3HfHzv4ppRrX509UFem0tfRX1Pul83etY HiNH2rPKLdq/b8IDQqhVXPJKYcRgw09WQAhXuMhg= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E6B5C6C7D4 for ; Sat, 20 Mar 2021 03:43:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E6B5C6C7D4 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lNPhs-00076U-2I; Sat, 20 Mar 2021 03:43:08 +0300 To: tarantool-patches@dev.tarantool.org, gorcunov@gmail.com, sergepetrenko@tarantool.org Date: Sat, 20 Mar 2021 01:42:43 +0100 Message-Id: X-Mailer: git-send-email 2.24.3 (Apple Git-128) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD95D6E7CC48CB1F5F1C82687294EF6886B62CC997B0B8C95C4182A05F5380850403A26C68E37FEE69328EDF67BAA5C893D5B54A460EE3BC46FCE1D64C85C24E810 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE745C0EDBD94D46193EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E8F1A1743CF948808638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C81A23F326053F827974AD6909CE9E844D51B28736E80BB5FA471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23DF004C9065253843057739F23D657EF2B13377AFFFEAFD26923F8577A6DFFEA7CB1724D34C644744093EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B6578ACBA422F0BC35089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E70227F01F88B6EF2528BD9CCCA9EDD067B1EDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2BBE337FB72E923155C0AF1600DCBC20BD3311767B7455A3391383219FBEA9B96 X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975C81A23F326053F827974AD6909CE9E844D51B28736E80BB5F9C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EFF532FBFD8162E58C699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34697E0FA301E28215C8A6F77A60BF8561533143CD15A60F3F51CC42434F29E3A278406FBFBD405DB61D7E09C32AA3244C90BC157DC1D5419A8E18FFE9194C707181560E2432555DBBFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj8xHC0Ak4ylZ0e1+U3w8jDQ== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822D2EB13B6955D81550EB449DD5904C0F83841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: [Tarantool-patches] [PATCH 06/16] cord_buf: introduce ownership management X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" The global ibuf used for hot Lua and Lua C code didn't have ownership management. As a result, it could be reused in some unexpected ways during Lua GC via __gc handlers, even if it was currently in use in some code below the stack. The patch makes cord_ibuf_take() steal the global buffer from its global stash, and assign to the current fiber. cord_ibuf_put() puts it back to the stash, and detaches from the fiber. If yield happens before cord_ibuf_put(), the buffer is detached automatically. Fiber attach/detach is done via on_yield/on_stop triggers. The buffer is not supposed to survive a yield, so this allows to free/put the buffer back to the stash even if the owner didn't do that. For instance, if a Lua exception was raised before cord_ibuf_put() was called. This makes cord buffer being safe to use in any yield-free code, even if Lua GC might be started. And in non-Lua code as well. Part of #5632 --- src/lib/core/cord_buf.c | 150 +++++++++++++++++++++++++++++++---- src/lib/core/cord_buf.h | 6 +- test/app-tap/buffer.test.lua | 59 ++++++++++++++ 3 files changed, 199 insertions(+), 16 deletions(-) create mode 100755 test/app-tap/buffer.test.lua diff --git a/src/lib/core/cord_buf.c b/src/lib/core/cord_buf.c index cac508c3d..9450d75bc 100644 --- a/src/lib/core/cord_buf.c +++ b/src/lib/core/cord_buf.c @@ -5,6 +5,7 @@ */ #include "cord_buf.h" #include "fiber.h" +#include "trigger.h" #include "small/ibuf.h" @@ -13,35 +14,154 @@ enum { CORD_IBUF_START_CAPACITY = 16384, }; -static struct ibuf *cord_buf_global = NULL; +/** Global buffer with automatic collection on fiber yield. */ +struct cord_buf { + /** Base buffer. */ + struct ibuf base; + /** + * Triggers on fiber stop/yield when the buffer is either destroyed or + * cached to the global stash for later reuse. + */ + struct trigger on_stop; + struct trigger on_yield; +#if !NDEBUG + /** + * Fiber owning the buffer right now. Used for debug and sanity checks + * only. + */ + struct fiber *owner; +#endif +}; -struct ibuf * -cord_ibuf_take(void) +/** + * The global buffer last saved to the cache. Having it here is supposed to + * help to reuse the buffer's already allocated data sometimes. + */ +static struct cord_buf *cord_buf_global = NULL; + +static inline void +cord_buf_put(struct cord_buf *buf); + +static void +cord_buf_delete(struct cord_buf *buf); + +static inline void +cord_buf_set_owner(struct cord_buf *buf) { - assert(cord_is_main()); - struct ibuf *buf = cord_buf_global; - if (buf != NULL) { - ibuf_reset(buf); - return buf; - } - buf = malloc(sizeof(*buf)); + assert(buf->owner == NULL); + struct fiber *f = fiber(); + trigger_add(&f->on_stop, &buf->on_stop); + trigger_add(&f->on_yield, &buf->on_yield); +#if !NDEBUG + buf->owner = f; +#endif + ibuf_reset(&buf->base); +} + +static inline void +cord_buf_clear_owner(struct cord_buf *buf) +{ + assert(buf->owner == fiber()); + trigger_clear(&buf->on_stop); + trigger_clear(&buf->on_yield); +#if !NDEBUG + buf->owner = NULL; +#endif +} + +static int +cord_buf_on_stop(struct trigger *trigger, void *event) +{ + (void)event; + struct cord_buf *buf = trigger->data; + assert(trigger == &buf->on_stop); + cord_buf_put(buf); + return 0; +} + +static int +cord_buf_on_yield(struct trigger *trigger, void *event) +{ + (void)event; + struct cord_buf *buf = trigger->data; + assert(trigger == &buf->on_yield); + cord_buf_put(buf); + return 0; +} + +static struct cord_buf * +cord_buf_new(void) +{ + struct cord_buf *buf = malloc(sizeof(*buf)); if (buf == NULL) panic("Couldn't allocate thread buffer"); - ibuf_create(buf, &cord()->slabc, CORD_IBUF_START_CAPACITY); - cord_buf_global = buf; + ibuf_create(&buf->base, &cord()->slabc, CORD_IBUF_START_CAPACITY); + trigger_create(&buf->on_stop, cord_buf_on_stop, buf, NULL); + trigger_create(&buf->on_yield, cord_buf_on_yield, buf, NULL); +#if !NDEBUG + buf->owner = NULL; +#endif + return buf; +} + +static inline void +cord_buf_put(struct cord_buf *buf) +{ + assert(cord_is_main()); + cord_buf_clear_owner(buf); + /* + * Delete if the stash is busy. It could happen if there was >= 2 + * buffers at some point and one of them is already saved back to the + * stash. + * + * XXX: in future it might be useful to consider saving the buffers into + * a list. Maybe keep always at most 2 buffers, because usually there + * are at most 2 contexts: normal Lua and Lua during GC. Recursive + * GC is supposed to be rare, no need to optimize it. + */ + if (cord_buf_global == NULL) + cord_buf_global = buf; + else + cord_buf_delete(buf); +} + +static inline struct cord_buf * +cord_buf_take(void) +{ + assert(cord_is_main()); + struct cord_buf *buf = cord_buf_global; + if (buf != NULL) + cord_buf_global = NULL; + else + buf = cord_buf_new(); + cord_buf_set_owner(buf); return buf; } +static void +cord_buf_delete(struct cord_buf *buf) +{ + assert(buf->owner == NULL); + ibuf_destroy(&buf->base); + TRASH(buf); + free(buf); +} + +struct ibuf * +cord_ibuf_take(void) +{ + return &cord_buf_take()->base; +} + void cord_ibuf_put(struct ibuf *ibuf) { - (void)ibuf; - assert(ibuf == cord_buf_global); + cord_buf_put((struct cord_buf *)ibuf); } void cord_ibuf_drop(struct ibuf *ibuf) { ibuf_reinit(ibuf); - assert(ibuf == cord_buf_global); + cord_ibuf_put(ibuf); } diff --git a/src/lib/core/cord_buf.h b/src/lib/core/cord_buf.h index 59f429c8f..5e65d138b 100644 --- a/src/lib/core/cord_buf.h +++ b/src/lib/core/cord_buf.h @@ -18,7 +18,9 @@ struct ibuf * cord_ibuf_take(void); /** - * Put the global ibuf back. + * Put the global ibuf back. It is not necessary - the buffer is put back on the + * next yield. But then it can't be reused/freed until the yield. Put it back + * manually when possible. */ void cord_ibuf_put(struct ibuf *ibuf); @@ -29,6 +31,8 @@ cord_ibuf_put(struct ibuf *ibuf); * because it is often needed from Lua, and allows not to call :recycle() there, * which would be an additional FFI call before cord_ibuf_put(). * + * Drop is not necessary though, see the put() comment. + * * XXX: recycle of the global buffer is a workaround for the ibuf being used in * some places working with Lua API, where it wasn't wanted to "reuse" it * anyhow. Instead, the global buffer is used to protect from the buffer leak in diff --git a/test/app-tap/buffer.test.lua b/test/app-tap/buffer.test.lua new file mode 100755 index 000000000..f57b3cf45 --- /dev/null +++ b/test/app-tap/buffer.test.lua @@ -0,0 +1,59 @@ +#!/usr/bin/env tarantool + +local tap = require('tap') +local fiber = require('fiber') +local buffer = require('buffer') +local cord_ibuf_take = buffer.internal.cord_ibuf_take +local cord_ibuf_put = buffer.internal.cord_ibuf_put +local cord_ibuf_drop = buffer.internal.cord_ibuf_drop + +local function test_cord_ibuf(test) + test:plan(10) + + local ibuf1 = cord_ibuf_take() + test:is(ibuf1:size(), 0, 'is empty') + ibuf1:alloc(1) + test:is(ibuf1:size(), 1, 'alloc 1') + cord_ibuf_put(ibuf1) + + ibuf1 = cord_ibuf_take() + test:is(ibuf1:size(), 0, 'is empty again') + ibuf1:alloc(1) + cord_ibuf_drop(ibuf1) + + ibuf1 = cord_ibuf_take() + test:is(ibuf1:capacity(), 0, 'has no capacity') + local pos1 = ibuf1:alloc(1) + pos1[0] = 1 + + local ibuf2 = cord_ibuf_take() + test:isnt(ibuf1, ibuf2, 'can have 2 cord buffers') + test:is(ibuf2:size(), 0, 'second is empty') + local pos2 = ibuf2:alloc(1) + pos2[0] = 2 + test:is(pos1[0], 1, 'change does not affect the first buffer') + cord_ibuf_put(ibuf2) + ibuf1 = ibuf2 + + fiber.yield() + ibuf2 = cord_ibuf_take() + test:is(ibuf1, ibuf2, 'yield drops the ownership') + cord_ibuf_put(ibuf2) + + ibuf1 = nil + local f = fiber.new(function() + ibuf1 = cord_ibuf_take() + end) + f:set_joinable(true) + f:join() + test:isnt(ibuf1, nil, 'took a cord buf in a new fiber') + ibuf2 = cord_ibuf_take() + test:is(ibuf1, ibuf2, 'was freed on fiber stop and reused') + cord_ibuf_put(ibuf2) +end + +local test = tap.test('buffer') +test:plan(1) +test:test("cord buffer", test_cord_ibuf) + +os.exit(test:check() and 0 or 1) -- 2.24.3 (Apple Git-128)