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 690636FFA3; Thu, 25 Mar 2021 00:30:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 690636FFA3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616621442; bh=Ks1Z4S7Nmlw5j7crutf6MxXW2yI1UIPZAjPprZJdoQA=; 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=xUzYrQXEoisG+TAkQ7h1vh8H+tX5V7xsF8PKWjVqOxcetaszDS4KxywKV/KHh4vN4 43i2/CcD6EClqiC4qhRJlnVEmwbHbKrHVKhFKx3r9NwTGqVuSlM/i22ieNXdUpQTPF qQQWUSViF15wUIVw46NrMfRV8tfvNQLkDeOoy4Hs= Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 3D0AD68F40 for ; Thu, 25 Mar 2021 00:24:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3D0AD68F40 Received: by smtp48.i.mail.ru with esmtpa (envelope-from ) id 1lPAzj-0004ib-PE; Thu, 25 Mar 2021 00:24:52 +0300 To: tarantool-patches@dev.tarantool.org, kyukhin@tarantool.org Date: Wed, 24 Mar 2021 22:24:32 +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: 4F1203BC0FB41BD9064ADF4728AA0EE9AECA9F3C9C9885BEE78E91CF33279E24182A05F5380850408A86BFCA153F15FFB54A78EA65668A85275C85F4C78484AE47068EB3C1E92C7B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A3295C83650092F9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375B4C42A189C515578638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C5DD32608FC869F5D51241B4B9D3471AC841FA1CEC635DE12A471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE7F852901E648D89D8D32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C04D4611528FA38C58CD04E86FAF290E2D7E9C4E3C761E06A71DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF5012EF20D2F80756B5F868A13BD56FB6657E2021AF6380DFAD18AA50765F79006372E808ACE2090B5E1C4224003CC836476EC64975D915A344093EC92FD9297F6718AA50765F7900637B8F435DEDE9E76EBA7F4EDE966BC389F395957E7521B51C24C7702A67D5C33162DBA43225CD8A89F83C798A30B85E16BC6EABA9B74D0DA47B5C8C57E37DE458B4C7702A67D5C3316FA3894348FB808DB985633C00BAEBE4F574AF45C6390F7469DAA53EE0834AAEE X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824AC8B6CDF511875BC4E8F7B195E1C97831148145A0506FFD48B1D76F471CF06CBF X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975C5DD32608FC869F5D51241B4B9D3471AC841FA1CEC635DE129C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EF0417BEADF48D1460699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34F38194B2C99DC128E1CD602A5D20AB8163E6A829DA938E2D5FC2F8FF3F80B01E7DD7662B691A31351D7E09C32AA3244CBF651FFED38EB132BFEC5826143BF286F522A1CF68F4BE05729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojjqzNotmU+gfd6mYSu2Gp7g== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA3110A906815429F9D960ED2EBDDD5B0EA85CA3B68E5AF1B865DF07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: [Tarantool-patches] [PATCH 06/15] 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 (cherry picked from commit c20e0449c52e36a987fb1c8fa3c18f398a395851) --- src/cord_buf.c | 148 +++++++++++++++++++++++++++++++---- src/cord_buf.h | 6 +- test/app-tap/buffer.test.lua | 59 ++++++++++++++ 3 files changed, 197 insertions(+), 16 deletions(-) create mode 100755 test/app-tap/buffer.test.lua diff --git a/src/cord_buf.c b/src/cord_buf.c index cac508c3d..661c60010 100644 --- a/src/cord_buf.c +++ b/src/cord_buf.c @@ -5,6 +5,7 @@ */ #include "cord_buf.h" #include "fiber.h" +#include "trigger.h" #include "small/ibuf.h" @@ -13,35 +14,152 @@ 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; +#ifndef 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); +#ifndef 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); +#ifndef NDEBUG + buf->owner = NULL; +#endif +} + +static void +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); +} + +static void +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); +} + +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); +#ifndef 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/cord_buf.h b/src/cord_buf.h index 59f429c8f..5e65d138b 100644 --- a/src/cord_buf.h +++ b/src/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)