Re: [tarantool-patches] Re: [PATCH 3/3] iproto: allow to configure IPROTO_MSG_MAX

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.