I believe the focus here should not be solely on TLS. TLS is quite obvious, but it is definitely not the only consideration.

Please read the discussion here first. All of my discussion/argument is still completely on-point: #10524

This boils down to a prioritization issue. When the event loop is saturated (CPU is 100% at the micro level), a prioritization is required. Do we delay new connections? or do we delay existing work? It's a binary decision. We must bias towards processing existing work - otherwise we risk timeout/retries. By increasing the number of connections accepted greater than 1, we are choosing to bias towards accepting connections - which is the wrong decision.

Side note: it's also valid to question if there are cases where the accept count should be reduced to something LESS THAN 1. This would bias us even more to existing work. In practice I have found this unnecessary and IMO it would add unnecessary complexity.

The issue with accept count is quite obvious in a TLS storm. Each accept implies the need for expensive negotiation messages. When we accept too much, the negotiation can't complete without timeout. Retries happen. Eventually we get to a point where NONE of the connections complete negotiation, and the engine is saturated with connection attempts. By prioritizing the negotiation over new accepts, SOME of the connections will complete without timeout. Over a short period of time, a TLS storm will dissipate.

REAL WORLD SCENARIOS that I have witnessed:

1. We've been talking about TLS storms. I've personally witnessed TLS storms occurring when the connections-per-cycle has been adjusted down to 16. Once connections-per-cycle is reduced to 1, the problem is effectively eliminated. You could try to "tune" this to a number between 1 and 16, however IMO there's not much value to this. A higher setting does not result in significantly different performance, and just increases risk of storm in unexpected conditions.
2. KEYS. We know that the KEYS command is dangerous. I witnessed a system where a large number of clients were attaching and immediately issuing a KEYS command. This command is expensive (like TLS negotiation) and once several clients were simultaneously trying to execute KEYS, they all started timing out. The event loop lengthened - to MINUTES. Redis was executing these expensive commands, only to find that the client had timed out before receiving the response. The clients immediately retried resulting in the same type of "storm" situation. By adjusting connections-per-cycle to 1, it allowed the KEYS commands that were in-flight to complete. This immediately made the event loop more responsive. Rather than 100% timeout/failure, the system was now responsive and successfully processing some of the commands.
3. CLUSTER INFO. Some clients issue the CLUSTER INFO command immediately after connecting to collect topology information. Especially on large clusters, this can be an expensive command. I have witnessed a system where the event loop was saturated with CLUSTER INFO. The resultant behavior was identical to the TLS storm or the KEYS scenario above. The event loop slowed. 100% of the CLUSTER INFO commands timed out. Once connections-per-cycle was adjusted to 1, the system immediately recovered. The event loop became responsive. Connections were established.

In summary, if the event loop isn't saturated, this change makes no difference. The event loop spins quickly and connections are established. It's only in the case that the event loop is saturated, that connections-per-cycle has any impact. The impact is immediate and beneficial.

P.S. If it were up to me, I'd remove the configuration and inner connection loop completely. Over the last 2 years, I've seen no evidence that using a value greater than 1 is ever warranted. But I agree that a tunable parameter is the less controversial option.

But I agree that a tunable parameter is the less controversial option.

As a note, this is often a more controversial option here compared to our internal AWS fork since it's a new config we have to expose, document, and maintain.

In summary, if the event loop isn't saturated, this change makes no difference. The event loop spins quickly and connections are established. It's only in the case that the event loop is saturated, that connections-per-cycle has any impact. The impact is immediate and beneficial.

@JimB123 Can you confirm this is also the case when there's very large spike of inbound connections? I'm concerned the extra syscalls will reduce accept rate and potentially have an impact on the TCP backlog.

@JimB123 Can you confirm this is also the case when there's very large spike of inbound connections? I'm concerned the extra syscalls will reduce accept rate and potentially have an impact on the TCP backlog.

@yossigo You are correct that there are more syscalls. If accepting connections was the only measure of success, you would be right to be concerned. However, the measure of success should be accepting connections AND doing work. When originally working on this, I did some rough measurements of pure event loop speed and found the event loop to spin much faster than any reasonable connection rate. In the TLS case, the extra overhead is very small in comparison to the TLS negotiation.

If I was to TRY to show a degradation in overall performance/throughput, I'd set up a scenario as follows:

• No TLS
• No AUTH
• Each connection does a single simple GET command and then disconnects

I think in this case, there might be some measurable performance delta. However, I don't think we should optimize for such an anti-pattern.

@JimB123 I wasn't suggesting to optimize for it, but use it as a test case to understand the worst case impact. Anyway, based on running memtier_benchmark --reconnect-interval 1 -n 5000 -x 3 it seems like we can expect around 2% worst case impact:

unstable:

Totals      13162.03         0.00     11964.29        12.56837        11.26300        29.82300        39.42300       558.46 

This PR:

Totals      12871.28         0.00     11700.00        12.85873        11.58300        30.20700        37.63100       546.12 

Very nice @yossigo. 2%, worst case, is better than I was expecting. Of course, in practice, we're very unlikely to see any throughput impact. The connection (& disconnection) rate would have to be extremely high, and CPU would have to be saturated.

@yossigo @JimB123 So I'm still going to restate that my preference is we should just update these values to 1 and not have a config.

@madolson I'd play it safe and leave it configurable for now.

@yossigo in that case are we okay with a default of 1? Otherwise people will need to be impacted before they know to fix it.

@madolson Yes.

i have one more concern.
during loading, we process events only once in a while (e.g. 2mb of rdb data), when that happens, we spin the event loop 4 times:

 * It calls the event loop in order to process a few events. Specifically we
 * try to call the event loop 4 times as long as we receive acknowledge that
 * some event was processed, in order to go forward with the accept, read,
 * write, close sequence needed to serve a client.

this is necessary in order to still be responsive for PING and INFO.
with this change, it means we'll only be responsive for one (or actually two) newly connected clients.
i.e. if we have more than one script processing redis-cli ping watching us, some can hang / fail and consider redis is dead (maybe some kind of WD that will try to restart redis).

this isn't normally a problem since 2mb of rdb file are probably quite fast, and we'll get many of these per second, but there are cases where the rate of such calls to processEventsWhileBlocked drops.
let's imagine for a second that it drops to one call per 10 seconds.
so maybe we want to limit the rate of connections per second, rather than per event loop cycle?

@JimB123 I wasn't suggesting to optimize for it, but use it as a test case to understand the worst case impact. Anyway, based on running memtier_benchmark --reconnect-interval 1 -n 5000 -x 3 it seems like we can expect around 2% worst case impact:

unstable:

Totals      13162.03         0.00     11964.29        12.56837        11.26300        29.82300        39.42300       558.46 

This PR:

Totals      12871.28         0.00     11700.00        12.85873        11.58300        30.20700        37.63100       546.12 

I think we need re-benchmark it, 13162.03 qps in unstable is too slow for Redis

we discussed this in a core-team meeting.

we dismissed the concern of processEventsWhileBlocked, considering it an edge case that would only cause issues when several types of abuse are combined (a monitoring software that re-connects each time, several of them, and a very slow rate of processEventsWhileBlocked calls).

we conceptually approved this pending on a re-benchmark (on a better hardware?) of the impact on clients that re-connect for each command they send.

@AshMosh Can you help with the performance testing above? Ideally we want to test a more realworld scenarios than on the laptop which Yossi did.

With max-new-connections-per-cycle=5, reconnect-interval=10:

With max-new-connections-per-cycle=10, reconnect-interval=10:

I also reran with reconnect-interval = 1 for a more extreme connection storm simulation:

With max-new-connections-per-cycle=5, reconnect-interval=1:

With max-new-connections-per-cycle=10, reconnect-interval=1:

With max-new-connections-per-cycle=1000, reconnect-interval=1:

So I am a bit inclined to update my position and say we should update the PR and use a default value of 10. It seems to still provide most of the protective benefits against TLS based connection storms while still providing minimal detriment to the workloads that open a lot of connections. @soloestoy Thoughts?

Anything larger than 1 risks timeouts on your real workload and death loop.
We WANT connections to timeout/retry under these conditions. We don't want regular commands to start failing.

Set up a scenario with a high connection rate, using TLS, and a workload with small timeouts for regular commands. Make sure you drive CPU to 100%.

Decision for now is to split into two separate configs:

max-new-tcp-connections-per-cycle
max-new-tls-connections-per-cycle

With two different default values. Default of 1 for TLS and 10 for TCP.

The motivation was we wanted to be as defensive as possible for not introducing performance regression for TCP based workloads that do single client + single command workloads (like with PHP).

I have reconsidered, and if we are concerned about the potential time consumption of accept, perhaps we can set a time threshold for accept, such as 1000 microseconds. This way, we don't need to differentiate between TLS and TCP or worry about the number of accepted connections. Using time as a limit can be a better approach.

#12178 (comment) @murphyjacob4 the results are based on this PR (limiting to 1 or 10) or unstable?

Additionally, Redis initially accept only one connection, which was later increased to 1000 starting from version 2.8, see more details in f3d3c60. Does anyone have knowledge about this history?

As mentioned in the commit description, this change was made to optimize scenarios where a connect-disconnect pattern is frequently used. In this scenario, accepting a new connection should be also considered a high-priority event, even if the CPU usage is at 100%. Therefore, if we intend to reduce the default value now, we need to exercise caution and carefully consider the implications.

personally, i don't like the use of plaintext. none of the other configs we have that are different for TLS and plain TCP, have a plaintext marking (no to mention that it's not necessarily "text" that is transmitted, so i don't like that term at all for plain-TCP).
i'd go with max-new-connections-per-cycle and max-new-tls-connections-per-cycle-tls.

I have reconsidered, and if we are concerned about the potential time consumption of accept, perhaps we can set a time threshold for accept, such as 1000 microseconds. This way, we don't need to differentiate between TLS and TCP or worry about the number of accepted connections. Using time as a limit can be a better approach.

The problem isn't in the accept per-say it's that we're signing up for future work with the TLS negotiation. I think a better alternative, which I've mentioned before, is to limit the number of TLS connections in the handshaking state. Which bottlenecks incoming TCP connections much less since they move to accept immediately.

@oranagra I didn't like tcp either, since it also applies to unix, so I'm fine just differentiating it on tls. Your suggestion has tls twice, I still prefer the original name max-new-tls-connections-per-cycle as opposed to adding tls as a suffix though.