From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH 3/3] iproto: allow to configure IPROTO_MSG_MAX References: <9a34dae0a53841d2d3f977f1096768396bb2835b.1524264646.git.v.shpilevoy@tarantool.org> <20180423113456.4ui5m6w3f4hur2yh@esperanza> From: Vladislav Shpilevoy Message-ID: Date: Mon, 23 Apr 2018 20:00:13 +0300 MIME-Version: 1.0 In-Reply-To: <20180423113456.4ui5m6w3f4hur2yh@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: Hello. Thanks for review! See my comments below. During fixing the patch I found and fixed a new problem in iproto, that I pushed as a separate commit. And now the patchset is grossly changed (1 commit is removed, 1 is added, 1 has big diff) so I will push a new one with v2. On 23/04/2018 14:34, Vladimir Davydov wrote: > On Sat, Apr 21, 2018 at 01:52:11AM +0300, Vladislav Shpilevoy wrote: >> IPROTO_MSG_MAX is a constant that restricts count of requests in >> fly. It allows to do not produce too many fibers in TX thread, >> that would lead to too big overhead on fibers switching, their >> stack storing. >> >> But some users have powerful metal on which Tarantool >> IPROTO_MSG_MAX constant is not serious. The patch exposes it as >> a configuration runtime parameter. >> >> 'iproto_msg_max' is its name. If a user sees that IProto thread >> is stuck due to too many requests, it can change iproto_msg_max >> in runtime, and IProto thread immediately starts processing >> pending requests. >> >> 'iproto_msg_max' can be decreased, but obviously it can not stop >> already runned requests, so if now in IProto thread request count >> is > new 'iproto_msg_max' value, then it takes some time until >> some requests will be finished. >> >> Closes #3320 >> --- >> src/box/box.cc | 7 +++ >> src/box/box.h | 1 + >> src/box/iproto.cc | 70 +++++++++++++++++------ >> src/box/iproto.h | 3 + >> src/box/lua/cfg.cc | 12 ++++ >> src/box/lua/load_cfg.lua | 3 + >> test/app-tap/init_script.result | 73 ++++++++++++------------ >> test/box/admin.result | 2 + >> test/box/cfg.result | 24 ++++++++ >> test/box/cfg.test.lua | 9 +++ >> test/box/request_limit.result | 119 +++++++++++++++++++++++++++++++++++++++- >> test/box/request_limit.test.lua | 55 ++++++++++++++++++- >> 12 files changed, 322 insertions(+), 56 deletions(-) >> >> -/* The number of iproto messages in flight */ >> -enum { IPROTO_MSG_MAX = 768 }; >> +enum { IPROTO_MSG_MAX_MIN = 768 }; > > Why do you forbid to set iproto_msg_max to 1 for instance? Why do we > have to allow at least 768 messages in flight? If there's no specific > reason, I'd prefer to remove this artificial lower bound. No concrete reason. Just wanted to protect a user from setting too small limit. But ok, I fixed this on branch, and set a limit to 2 (it can not be 1 - there must be availability to have 1 message in fly from iproto to tx and back (see cpipe_set_max_input - it is msg_max / 2 for net_pipe and tx_pipe). >> + if (cfg_msg.need_update_uri) { >> + if (evio_service_is_active(&binary)) >> + evio_service_stop(&binary); >> + if (cfg_msg.uri != NULL) >> + evio_service_bind(&binary, cfg_msg.uri); >> + } >> + if (cfg_msg.need_update_msg_max) { >> + cpipe_set_max_input(&tx_pipe, >> + cfg_msg.iproto_msg_max / 2); >> + int old = iproto_msg_max; >> + iproto_msg_max = cfg_msg.iproto_msg_max; >> + if (old < iproto_msg_max) >> + iproto_resume(); >> + } > > This is a matter of personal taste, but I'd prefer to not introduce > these extra flags, i.e. It is not possible for URI, because URI == NULL is possible update. So for URI the flag is necessary. For msg_max I added this for unifying. If you want, I can remove need_update_msg_max. Must I do it? > > if (cfg_msg.uri != NULL) > /* set uri */ > > if (cfg_msg.iproto_msg_max > 0) > /* update iproto_msg max */ > >> + cpipe_set_max_input(&net_pipe, new_iproto_msg_max / 2); >> +} > > AFAIR IPROTO_MSG_MAX is related to FIBER_POOL_SIZE so if we increase the > former, we should increase the latter as well, no? Yes, it is related. I made it configurable on the branch. >> box.schema.user.revoke('guest', 'read,write,execute', 'universe') >> -box.cfg{readahead = old_readahead} >> +box.cfg{readahead = old_readahead, iproto_msg_max = limit} > > If I run the test several times in a row, it fails. Please fix. Done.