[tarantool-patches] Re: [PATCH 3/3] iproto: allow to configure IPROTO_MSG_MAX
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Apr 23 20:00:13 MSK 2018
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.
More information about the Tarantool-patches
mailing list