Mechanik20051988/gh 5645 several iproto threads new by EvgenyMekhanik · Pull Request #5877 · tarantool/tarantool

EvgenyMekhanik · 2021-03-11T09:42:57Z

iproto: implement ability to run multiple iproto threads

There are users that have specific workloads where iproto thread
is the bottleneck of throughput: iproto thread's code is 100% loaded
while TX thread's core is not. For such cases it would be nice to have
a capability to create several iproto threads.

Closes #5645

alyapunov

A left several comments. I think there are ways to improve this patchset.

alyapunov · 2021-03-22T11:43:48Z

src/rmean.c

 	int64_t mean = 0;
 	for (size_t j = 1; j <= RMEAN_WINDOW; j++)
-		mean += rmean->stats[name].value[j];
+		mean += __atomic_load_n(&rmean->stats[name].value[j], __ATOMIC_ACQUIRE);


I think we may assume that values 1..RMEAN_WINDOW are always accessed from main thread and thus should not be synchronized.

alyapunov · 2021-03-22T12:00:58Z

src/rmean.c


-	rmean->stats[name].value[0] += value;
-	rmean->stats[name].total += value;
+	__atomic_add_fetch(&rmean->stats[name].value[0], value, __ATOMIC_RELEASE);


Why not __ATOMIC_RELAXED? I don't see a reason of using __ATOMIC_RELEASE here.

alyapunov · 2021-03-22T12:07:35Z

src/rmean.c

 		for (size_t j = 0; j < RMEAN_WINDOW + 1; j++)
-			rmean->stats[i].value[j] = 0;
-		rmean->stats[i].total = 0;
+			__atomic_store_n(&rmean->stats[i].value[j], 0, __ATOMIC_RELEASE);


I think that only value[0] should have a fence, since only it can be modified from other thread.

alyapunov · 2021-03-22T12:28:12Z

src/rmean.c

 rmean_roll(int64_t *value, double dt)
 {
-	value[0] /= dt;
+	int64_t tmp = __atomic_load_n(&value[0], __ATOMIC_ACQUIRE);


That function was not a good piece of code, and you make it worse.
What's a point of several atomic accesses of value[0] in this function? If you are expecting that somebody else is changing it - what would be a correct way to handle it?
I don't like thoughtless changing of access to everything suspicious to store+RELEASE and load+ACQUIRE. A code should do everything it must and not more.

alyapunov · 2021-03-22T12:30:21Z

src/rmean.h

 rmean_total(struct rmean *rmean, size_t name)
 {
-	return rmean->stats[name].total;
+	return __atomic_load_n(&rmean->stats[name].total, __ATOMIC_ACQUIRE);


Similarly, it think __ATOMIC_RELAXED would be enough. Not a big deal btw.

alyapunov · 2021-03-22T14:05:32Z

src/box/iproto.cc

+static inline void
+iproto_thread_init_routes(struct iproto_thread *iproto_thread)
+{
+	assert(iproto_thread != NULL);


I didn't verify each line of this function. I hope it's correct.

alyapunov · 2021-03-22T14:13:04Z

src/box/iproto.cc

-	return mempool_count(&iproto_msg_pool);
+	size_t count = 0;
+	for( auto& iproto_thread : iproto_threads )
+		count += mempool_count(&iproto_thread->iproto_msg_pool);


What about synchronization here?

alyapunov · 2021-03-22T14:13:51Z

src/box/iproto.cc

-	return mempool_count(&iproto_connection_pool);
+	size_t count = 0;
+	for( auto& iproto_thread : iproto_threads )
+		count += mempool_count(&iproto_thread->iproto_connection_pool);


What about synchronization here?

alyapunov · 2021-03-22T14:13:59Z

src/box/iproto.cc

+	size_t mem = 0;
+	for( auto& iproto_thread : iproto_threads )
+		mem += slab_cache_used(&iproto_thread->net_cord.slabc) +
+		       slab_cache_used(&iproto_thread->net_slabc);


What about synchronization here?

alyapunov · 2021-03-22T14:23:12Z

src/box/iproto.cc

+	if (uri != NULL) {
+		if (evio_service_bind(&binary, uri) != 0)
+			diag_raise();
+		for( auto& iproto_thread : iproto_threads ) {


I don't like this solutions. Actually I don't know how to do it good, but I didn't think a lot. I see the following problems to be fixed:

You set here iproto_thread's members and it's not obvious why it is safe. Please don't explain me, I understand, but you should make the code understandable by itself.

This new meesage IPROTO_CFG_STOP is not documented fine. It seems that doesn't require the service to be started. At the same time IPROTO_CFG_LISTEN requires (!) service to be stopped. That's very confusing.

You do a lot work right here, while doing a lot if work in IPROTO_CFG_LISTEN handler. That's not obvious either.

Korablev77 · 2021-04-08T15:06:07Z

src/box/iproto.cc

+}
+
+void
+iproto_listen(const char *uri)


Two funcs with the same name - I'd rather rename one of them

Korablev77 · 2021-04-08T15:51:27Z

test/box/gh-5645-several-iproto-threads.test.lua

+test_run:cmd('start server test with args="8"')
+server_addr = test_run:cmd("eval test 'return box.cfg.listen'")[1]
+fibers_count = fibers_count * 2
+assert(test_run:grep_log("test", "stopping input on connection") == nil)


Could you please explain what is the purpose of the test and what are you checking?

The reason why we are making this patch is that: there are users that have specific workloads where iproto thread
is the bottleneck of throughput: iproto thread's code is 100% loaded while TX thread's core is not. I don't know how to check full iproto thread loading in test, except for indirect signs, one of which is the message in the log "stopping connection on input". So in test first of all i start instance with one iproto thread, and send requests in a large number of fibers at the same time to this instance. I increase the number of fibers by 4 times until I get "stopping connection on input" in log. Then i restart instance with 8 itproto threads increase the number of fibers by 4 times again and check that there is no such message. Based on this, I conclude that 8 threads can withstand at least 4 times the more load

If you have any ideas on how to test this better, that would be great!

Korablev77 · 2021-04-13T13:27:10Z

src/box/iproto.cc

+	evio_service_init(loop(), &iproto_thread->binary, "binary",
+			  iproto_on_accept, iproto_thread);
+
+	static const unsigned int endpoint_name_max = ENDPOINT_NAME_MAX;


Why do you need this var? Why not directly use ENDPOINT_NAME_MAX?

Korablev77 · 2021-04-13T13:32:12Z

src/box/iproto.cc

+		iproto_thread->stopped_connections =
+			RLIST_HEAD_INITIALIZER(iproto_thread->
+					       stopped_connections);
+		static const unsigned int endpoint_name_max = 10;


Use added enum

Korablev77 · 2021-04-13T14:13:38Z

src/box/iproto.cc

-	cpipe_set_max_input(&net_pipe, iproto_msg_max / 2);
+/** Initialize the iproto subsystem and start network io thread */
+void
+iproto_init(unsigned int threads_count)


cfg_geti returns int, so better make this function take int param. Also I'd add some check verifying rationality of threads_count value. I mean raise warning/error in case user passes too large value. For instance, I doubt that it makes sense to set value more than 2*cores. I don't insist on this check tho.

The fields of the rmean structure can be accessed from multiple threads, so we must use atomic operations to get/set fields in this structure. Also in the comments to the functions i wrote in which threads they should be called to correctly access the fields of the rmean structure.

There was two problems with struct rmean: - For correct access for rmean struct fields, this struct should be created in tx thread. - In case when rmean_new return NULL in net_cord_f tarantool hangs and does not terminate in any way except on SIGKILL. Also net_slabc cache was not destroyed. Moved allocation and deallocation of rmean structure to iproto_init/iproto_free respectively. Added slab_cache_destroy for net_slabc for graceful resource releases.

@TarantoolBot

There are users that have specific workloads where iproto thread is the bottleneck of throughput: iproto thread's code is 100% loaded while TX thread's core is not. For such cases it would be nice to have a capability to create several iproto threads. Closes #5645 @TarantoolBot document Title: implement ability to run multiple iproto threads Implement ability to run multiple iproto threads, which is useful in some specific workloads where iproto thread is the bottleneck of throughput. To specify count of iproto threads, user should used iproto_threads option in box.cfg. For example if user want to start 8 iproto threads, he must enter `box.cfg{iproto_threads=8}`. Default iproto threads count == 1. This option is not dynamic, so user can't change it after first setting, until server restart. Distribution of connections per threads is managed by OS kernel.

Korablev77

LGTM

Gerold103 · 2021-04-13T23:59:51Z

Well, hello. I suppose the review wasn't really thorough, was it?

Why is there just one review for such a big and important feature?
Why is such a huge feature is merged in a monolithic commit along with tons of refactoring and new minor formatting issues? A truly enormous time was spent on making iproto.cc code readable and conform to our code style at least to a notable extent and to document everything. iproto_thread struct should have been introduced separately before its count can be configured. Otherwise it is really hard where are the functional changes. Also there is no a single mentioning in the comments how the clients sharing works, and why it works like that (because Mons said we don't have time to make it properly).
You didn't think about net_msg_max. After this patch the real number of maximal messages becomes multiple of the number of threads. And that affects the TX thread. How does they work now together?
Why the heck the stop and stat commands are a part of iproto_cfg_op now? It is for configuration, not for getting info or terminating the subsystem! You can't blindly piggyback stuff to code which just happened to do something similar. We are going to live with this code. Not to hand it out to a lecturer and forget about it.
I just managed to create a tarantool instance with 0 iproto threads. Did you intend that behaviour? At the same time, box.cfg.listen is "successfully" specified. Was the feature considered too trivial to test it properly?

EvgenyMekhanik requested a review from alyapunov March 11, 2021 09:42

EvgenyMekhanik self-assigned this Mar 11, 2021

EvgenyMekhanik added the do not merge label Mar 11, 2021

EvgenyMekhanik force-pushed the mechanik20051988/gh-5645-several-iproto-threads-new branch 6 times, most recently from e77b431 to 5003a9b Compare March 12, 2021 12:01

EvgenyMekhanik assigned alyapunov and unassigned EvgenyMekhanik Mar 12, 2021

EvgenyMekhanik removed the do not merge label Mar 13, 2021

alyapunov suggested changes Mar 22, 2021

View reviewed changes

EvgenyMekhanik force-pushed the mechanik20051988/gh-5645-several-iproto-threads-new branch 2 times, most recently from 5edb2a0 to 34b285c Compare March 26, 2021 16:16

EvgenyMekhanik assigned EvgenyMekhanik and unassigned alyapunov Mar 29, 2021

EvgenyMekhanik force-pushed the mechanik20051988/gh-5645-several-iproto-threads-new branch 13 times, most recently from d0fa199 to eb657a2 Compare March 31, 2021 08:52

Korablev77 reviewed Apr 8, 2021

View reviewed changes

EvgenyMekhanik force-pushed the mechanik20051988/gh-5645-several-iproto-threads-new branch 6 times, most recently from 27b5d3d to 5d87828 Compare April 13, 2021 11:11

Korablev77 reviewed Apr 13, 2021

View reviewed changes

EvgenyMekhanik force-pushed the mechanik20051988/gh-5645-several-iproto-threads-new branch 4 times, most recently from 9bddee4 to 5c90dc2 Compare April 13, 2021 18:34

EvgenyMekhanik added 2 commits April 13, 2021 21:42

EvgenyMekhanik force-pushed the mechanik20051988/gh-5645-several-iproto-threads-new branch 5 times, most recently from 31244da to bb1c6e4 Compare April 13, 2021 21:13

Korablev77 force-pushed the mechanik20051988/gh-5645-several-iproto-threads-new branch from bb1c6e4 to 0081b6f Compare April 13, 2021 22:24

Korablev77 force-pushed the mechanik20051988/gh-5645-several-iproto-threads-new branch from 0081b6f to 754f64c Compare April 13, 2021 22:58

Korablev77 approved these changes Apr 13, 2021

View reviewed changes

Korablev77 merged commit 2ede3be into master Apr 13, 2021

Korablev77 deleted the mechanik20051988/gh-5645-several-iproto-threads-new branch April 13, 2021 23:15

pgulutzan mentioned this pull request Apr 14, 2021

[3pt] implement ability to run multiple iproto threads tarantool/doc#2059

Closed

Conversation

EvgenyMekhanik commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyapunov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Korablev77 left a comment

Choose a reason for hiding this comment

Uh oh!

Gerold103 commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EvgenyMekhanik commented Mar 11, 2021 •

edited

Loading