Upgrade connection to websocket and provision new connection #1594
Conversation
Signed-off-by: Aleksey Mikhaylov <aym@tempesta-tech.com>
ttaym
force-pushed
the
am-755-update-srv-shceduler
branch
from
baa327a
to
eac58dd
Compare
Signed-off-by: Aleksey Mikhaylov <aym@tempesta-tech.com>
Signed-off-by: Aleksey Mikhaylov <aym@tempesta-tech.com>
Signed-off-by: Aleksey Mikhaylov <aym@tempesta-tech.com>
Signed-off-by: Aleksey Mikhaylov <aym@tempesta-tech.com>
There was a problem hiding this comment.
It seems we have severe race, which may lead to crashes on multi-core workload with concurrent websocket creations, and need to find a simpler solution leaving #710 for later phases.
| if (unlikely(ci++ == srv->conn_n)) | ||
| goto err; | ||
| *conn++ = srv_conn; | ||
|
|
||
| cl_copy->conns[ci-1] = srv_conn; | ||
| } | ||
| if (unlikely(ci != srv->conn_n)) | ||
| goto err; |
There was a problem hiding this comment.
It seems I was wrong with the proposal of the functions unification in both the schedulers.
Previously we called tfw_sock_srv_new_conn(), which adds a new connection to the connection list, only from process context (since we have constant number of server connections and just failover TCP connections inside them). Moreover, tfw_peer_add_conn() uses a spin lock to add a connection. I believe we have a mutex on concurrent configuraion reloads, so we rely on the fact that tfw_sched_ratio_add_srv() always works in single context.
Now we can call this function concurrently with other websocket creation and with the configuration reload, so we definitely have races in all the places in the function. We can back to the separate functions and use the spin lock in the new function, but we'll also need to use the spin lock in all other functions iterating the connections list, which introduces lock contention.
What you're doing in the patch is almost solving #710 , which is a hard task on it's own to upgrade the schedulers data structures without locks.
Initially with #755 I assumed that you just "steal" sk from TfwSrvConn and initiate the existing connection failovering mechanism.
|
The code and the only one not-addressed comment were moved to #1595 . Please don't forget to remove the branch, when it's not needed |
Contributes to #755
Signed-off-by: Aleksey Mikhaylov aym@tempesta-tech.com