Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use io_uring to batch handle clients pending writes to reduce SYSCALL count. #13139

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from

Conversation

lipzhu
Copy link

@lipzhu lipzhu commented Mar 14, 2024

Description

This patch try to benefit the io_uring batching feature to reduce the SYSCALL count for Redis when handleClientsWithPendingWrites.
With this patch, we can observe more than 4% perf gain for SET/GET, and didn't see an obvious performance regression.

NOTE

  • Since io_uring was adopted from kernel 5.1, if kernel doesn't support io_uring yet, it will use the origin implementation.
  • This patch introduce the liburing dependency, it is installed in my local env, to keep it simple, I didn't include liburing dependency in this patch, the CI build may failed.

Test Result

Test Env

  • OS: CentOS Stream 8
  • Kernel: 6.2.0
  • Platform: Intel Xeon Platinum 8380
  • Base: 5b9fc46
  • Server and Client in same socket.

Test Steps

  1. Start redis-server taskset -c 0-3 ~/src/redis-server /tmp/redis_1.conf with below config.
port 9001
bind * -::*
daemonize yes
protected-mode no
save ""
  1. Start redis-becnmark taskset -c 16-19 ~/src/redis-benchmark -p 9001 -t set -d 100 -r 1000000 -n 5000000 -c 50 --threads 4 to ensure redis-server CPU utilized is 1(fully utilized).

Test Result

With this patch, the QPS can increase 4.3%.
And both the cycles for each query and SYSCALL count are reduced as expected.
perf stat -p `pidof redis-server` sleep 10
Table is the normalized cycles/ins for each query.

Base Opt Opt/Base-1
cycles/query 14,677 14,079 -4.1%
instructions/query 19,541 19,350 -1.0%
IPC 1.33 1.37 3.2%
CPU utilized 1 1 0%

bcc/syscount -p `pidof redis-server` -d 10
In this patch, io_uring_enter is the replacement of write.

SYSCALL COUNT Base Opt
read 1,950,855 2,081,797
write 1,950,726 0
epoll_wait 42,248 45,485
io_uring_enter 0 45,484
Total 3,943,829 2,172,766

From above data, the SYSCALL is expensive, by reducing the SYSCALL related instructions, IPC improves.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io_uring is very cool, but I don't understand it well enough yet. I commented on some things related to conditional compilation.

I think we can mark it as draft, because there are problems in CI that needs to be solved.

src/Makefile Show resolved Hide resolved
src/Makefile Outdated
Comment on lines 281 to 284
ifneq ($(USE_IO_URING),no)
FINAL_CFLAGS+= -DUSE_IO_URING
FINAL_LIBS+= -luring
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will always be true. USE_IO_URING is not set to "no" anywhere. Why not ifeq ($(USE_IO_URING),yes)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, my original intention is to turn on io_uring by default unless the user manually turns off io_uring by make USE_IO_URING=no. ): It works on Linux, but for other platform, we should disable io_uring default.

Sure, I will change to ifeq ($(USE_IO_URING),yes) as you suggested. Thanks.

src/server.h Outdated
@@ -3725,6 +3732,14 @@ void quitCommand(client *c);
void resetCommand(client *c);
void failoverCommand(client *c);

/* io_uring.c -- io_uring related operations */
#include <liburing.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't include this on platforms that don't have it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, maybe we can solve this by ifeq ($(USE_IO_URING),yes). And use USE_IO_URING to protect it.

@zuiderkwast zuiderkwast marked this pull request as draft March 15, 2024 16:36
@lipzhu
Copy link
Author

lipzhu commented Mar 16, 2024

@zuiderkwast Thanks for your comments.

io_uring is very cool, but I don't understand it well enough yet. I commented on some things related to conditional compilation.

Yes, io_uring is very cool, really excited to see io_uring can help Redis on this scenario and gain perf. The scenario is very straightforward, appreciate that the community can help on this.

I think we can mark it as draft, because there are problems in CI that needs to be solved.

For the CI build failure, it is caused by the liburing is not exist in CI environment, I am thinking how to add the liburing dependency for Redis: manually copy liburing to deps/ folder or refer to the method of ssl. What's your suggestion?

@zuiderkwast
Copy link
Contributor

I am thinking how to add the liburing dependency for Redis: manually copy liburing to deps/ folder or refer to the method of ssl. What's your suggestion?

I think we can auto-detect it like we do for C11 atomics here: https://github.com/redis/redis/blob/7.2.4/src/Makefile#L46. It creates a small file and tries to compile it.

@lipzhu
Copy link
Author

lipzhu commented Mar 18, 2024

I am thinking how to add the liburing dependency for Redis: manually copy liburing to deps/ folder or refer to the method of ssl. What's your suggestion?

I think we can auto-detect it like we do for C11 atomics here: https://github.com/redis/redis/blob/7.2.4/src/Makefile#L46. It creates a small file and tries to compile it.

The approach still need user manually install liburing even kernel has io_uring support, and sometimes the build env is not total same with runtime env, can we include https://github.com/axboe/liburing/ in deps module like jemalloc if license compliance?

@zuiderkwast
Copy link
Contributor

user manually install liburing even kernel has io_uring support

Ok, now I understand. I think we can include it under deps if it is small. What's the total size of it? The core team needs to make the final decision.

An idea is to introduce it in two steps. First, as a beta feature, we can we require the user to manually install liburing. We can modify one of the CI jobs to install and use it in tests. Later, when it's found to work well, it can be enabled by default and included under deps.

@lipzhu
Copy link
Author

lipzhu commented Mar 20, 2024

user manually install liburing even kernel has io_uring support

Ok, now I understand. I think we can include it under deps if it is small. What's the total size of it? The core team needs to make the final decision.

With liburing, the total size of redis-server binary changed from 21279160 to 21451704.

An idea is to introduce it in two steps. First, as a beta feature, we can we require the user to manually install liburing. We can modify one of the CI jobs to install and use it in tests. Later, when it's found to work well, it can be enabled by default and included under deps.

Agree, I just upload the liburing-2.5 to deps to check if the CI could success, this is a temporary solution. The final solution need core team make decision.


for (i = 0; i < qd; i++) {
sqe = io_uring_get_sqe(&ring);
int roff = (off + i) % table_size;

Check failure

Code scanning / CodeQL

Uncontrolled data in arithmetic expression High

This arithmetic expression depends on an
uncontrolled value
, potentially causing an overflow.
if (ret == -1)
t_error(1, errno, "recv");
if (ret != cfg_payload_len)
t_error(1, 0, "recv: ret=%u != %u", ret, cfg_payload_len);

Check failure

Code scanning / CodeQL

Wrong type of arguments to formatting function High

This argument should be of type 'unsigned int' but is of type 'long'.
@lipzhu
Copy link
Author

lipzhu commented Mar 21, 2024

Update: The patch makes below 4 unit tests hang, the others passed, I am trying to fix it, if any help will be appreciated. :)

  • Client output buffer hard limit is enforced
  • Client output buffer soft limit is enforced if time is overreached
  • Client output buffer soft limit is not enforced too early and is enforced when no traffic
  • slave buffer are counted correctly

BTW, @madolson @oranagra @zuiderkwast @enjoy-binbin do you think this make sense for Redis to adopt io_uring from this scenario?

@CLAassistant
Copy link

CLAassistant commented Mar 23, 2024

CLA assistant check
All committers have signed the CLA.

@lipzhu lipzhu marked this pull request as ready for review March 24, 2024 11:48
@oranagra
Copy link
Member

@lipzhu can you save me some time and improve the top comment describing the change in the flow.
i.e. i imagine it adds the buffers to be written without a syscall (and without copying the data), and then later waits for the writes in one system call before proceeding to the event loop? please add some details to explain the flow before i go read the code.

additionally, i see the benchmark mentions the reducing of cycles per commands, but not any improvement in throughput or latency, can we add one? how will it be affected by adding a pipeline (-P) to the benchmark?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To review
Development

Successfully merging this pull request may close these issues.

None yet

4 participants