Followup items from #758 Async IO threads

[madolson]

List of pending items from #758, all of these are future enhancements, and not pending issues that need to be merged before Valkey 8 (but they could be).

• Improvements for TLS with I/O threads #1271, remove err_clean in tls
• Offload reading the replication stream to IO threads #1449, replication traffic
• Offload replication writes to IO threads #1485, writes to replicas
• client struct: lazy init components and optimize struct layout #1405
• Parse multiple commands (look ahead) #2044
• Extend IO free offload to dbDelete - handle defrag
• lazy free to use IO threads?
• Offload TLS negotiation to I/O threads #1338
• Move all client’s fields accessed by the IO threads to a sub-struct.
• Allow dynamic modification of IO threads num.

[uriyage]

• Avoid recalculating cluster slot when the I/O thread has already done so #2077

[zuiderkwast]

• Offload PING command

Edit: Let's not do it. See discussion below.

[rjd15372]

Hi all, I'll start working on the "Offload PING command".

[rjd15372]

I've been working on the offload of the PING command and I came up with two approaches that I would like your opinion on.

Approach A)

The IO thread that is choose to handle the read of the PING command, read, parses, and executes the command, and then also writes the reply, before processing a new IO job,

Approach B)

After the PING command has been read and parsed, the main thread schedules the execution of the PING command (enqueues a new job in the IO queue). The IO thread that picks this "execution" job, executes the job and enqueues the write job in the IO queue.

Approach A is more simple, but it might block the IO thread if the socket is not available to write, while approach B is more complex but more asynchronous.

I'm in favor of approach B, even though it will require a generalization of the IO threads component to support a different kind (non-IO job), and also support the concurrent insertion of jobs in the shared queue.

@zuiderkwast @madolson thoughts?

[zuiderkwast]

@uriyage, you know IO threading better than anyone. Can you help?

@rjd15372 Approach B sounds like it will not be worth the overhead. Executing PING is very fast, maybe even faster executing in the main thread than delegating it to some thread to execute it.

I like approach A, which should add zero work to the main thread if the socket is writable, which is probably the normal case. If it's not writable, we need to register a write handler in the event loop and add the client to the global list server.clients_pending_io_write, which is not thread safe, so this probably needs to be done by the main thread (unless we refactor this in some way).

When the main thread delegates a read-and-parse-command job to an IO thread, it does among other things..

    c->io_read_state = CLIENT_PENDING_IO;
    connSetPostponeUpdateState(c->conn, 1);
    IOJobQueue_push(jq, ioThreadReadQueryFromClient, c);
    c->flag.pending_read = 1;
    listLinkNodeTail(server.clients_pending_io_read, &c->pending_read_list_node);

Later, the main thread checks if the clients are ready by calling processIOThreadsReadDone which loops over all clients in this list. Here, can we indicate that instead of parsing a command, the thread wants to write something to the client instead? We have the c->io_read_state and c->io_write_state variables to indicate these things back to the main thread. Or we can add a new flag in c->read_flags for this special case. I'm not quite sure about this. @uriyage WDYT?

Now, I'm just thinking, is it possible that the main thread has delegated read-and-parse for a client to one IO thread and also delegated write to that same client to another IO thread? That may be a problem... Have we've already prevented this due to TLS?

[uriyage]

@zuiderkwast @rjd15372
Approach A could lead to potential issues if the client already has pending data in the reply buffer or if the main thread simultaneously writes to the client after delegating the read (e.g., if the client is subscribed to a channel or MONITOR).
As you mentioned, we have read flags that the main thread can use to indicate whether the I/O thread can write to the client (when the client has no pending replies and is not subscribed or in a similar state). We currently use the same flags to indicate if the I/O thread can parse a command (if the client is not blocked) or if it should just read.
When the read job returns, we will need to:

1. Use flags to indicate that a command was executed
2. Update the cmd_stat and other relevant statistics
3. Handle the out-bytes stats as well

If the socket is not writeable, we will have to store the command output (or partial output if we succeed in writing part of it) in the client's reply buffer to be sent later.

Overall, I believe the performance gain from both approaches is marginal and may not justify the added complexity. Perhaps we should consider this approach only for more time-consuming commands that can be delegated to the I/O threads.

Regarding your question, we already ensure that the same thread that reads is the one that writes to the client.

[rjd15372]

@uriyage @zuiderkwast
I also agree that for the specific case of PING, we will not gain a lot by doing this, but the changes that we make for PING will be the same for any other commands that we will want to offload in the future.

My idea is to make it generic, and add a new command flag that states if the command can be offloaded.

[zuiderkwast]

Thanks Uri! Good points. I guess offloading PING may be not worth it then.

I don't see the point of doing it for PING just as a preparation for other commands. It's better to do it for another command where it actually matters. I don't know any command that doesn't need to access any global data though. Do you?

Perhaps it's better to focus on another of the follow-up items?

[uriyage]

• Remove err_clean in tls

#761

[uriyage]

• Offload TLS negotiation.
  Offload TLS negotiation to I/O threads #1338