Update listen backlog on reload and stop enforcing default backlog #12977
Conversation
stop enforcing the default listen backlog because some setups might legitimately require lower values.
|
Interested which setup requires this.. ? Also read the linux kernel commit that raised this limit to 4096.. torvalds/linux@19f92a0 |
|
There are multiple cases where this could be useful. Anything that requires quick rejection when all workers are busy, instead of having them wait on a large backlog (can be used for retries / failovers etc). Another thing is that tcp_syncookies changes the behavior of what happens when the backlog queue is full which in combination with low backlogs, can allow more flexibility in how failures are detected and handled. The default on the kernel is fine and not an issue here. But it also allows you to lower it should you want to, so that's also a good example how it shouldn't artificially be risen for people who need lower values. |
There was a problem hiding this comment.
In general I'm fine with those changes but we need to add some tests at least to have it partially covered.
I guess there might be some use case for this but not sure if tcp_syncookies are good example as from the docs, it says:
When syncookies are enabled there is no logical maximum length and this setting is
ignored.
Or do I miss anything?
| @@ -278,6 +278,7 @@ static int fpm_sockets_get_listening_socket(struct fpm_worker_pool_s *wp, struct | |||
|
|
|||
| sock = fpm_sockets_hash_op(0, sa, 0, wp->listen_address_domain, FPM_GET_USE_SOCKET); | |||
| if (sock >= 0) { | |||
| listen(sock, wp->config->listen_backlog); /* change backlog via listen() */ | |||
There was a problem hiding this comment.
For correctness we should check the return value of listen even though it is unlikely to fail. This is actually good to verify the we got a correct socket.
There was a problem hiding this comment.
It would be also good to have a test that does reload and changes the configuration to make that covered.
| @@ -915,8 +915,7 @@ static int fpm_conf_process_all_pools(void) | |||
| } | |||
|
|
|||
| if (config->listen_backlog < FPM_BACKLOG_DEFAULT) { | |||
| zlog(ZLOG_WARNING, "[pool %s] listen.backlog(%d) was too low for the ondemand process manager. I updated it for you to %d.", wp->config->name, config->listen_backlog, FPM_BACKLOG_DEFAULT); | |||
| config->listen_backlog = FPM_BACKLOG_DEFAULT; | |||
| zlog(ZLOG_WARNING, "[pool %s] listen.backlog(%d) is too low for the ondemand process manager. I suggest updating it to at least %d.", wp->config->name, config->listen_backlog, FPM_BACKLOG_DEFAULT); | |||
There was a problem hiding this comment.
we should probably verify why this is an issue for ondemand mode and test it with lower listen backlog. This was added together with ondemand mode from checking the changes so there might be a reason for adding it. There should be also a test added for setting it low and using ondemand. I would prefer to separate it to a new PR as it's unrelated to other change.
Add support for adjusting the listen backlog when reloading php-fpm and stop enforcing the default listen backlog because some setups might legitimately require lower values.