Re: [PATCH net-next 1/3] net/rds: Use a single TCP socket for both send and receive.
From: santosh shilimkar
Date: Wed Sep 30 2015 - 12:05:00 EST
On 9/30/2015 8:58 AM, Sowmini Varadhan wrote:
On (09/30/15 08:50), santosh shilimkar wrote:
minor nit though not a strict rule. Just to be consistent based on
what we are following.
- core RDS patches "RDS:"
- RDS IB patches "RDS: IB:" or "RDS/IB:"
- RDS IW patches "RDS: IW:" or
- RDS TCP can use "RDS: TCP" or "RDS/TCP:"
Ok, but in this case patch 1/3 the changes affect both core and rds-tcp
modules.
As I said, these are not strict rules but just what have been followed.
I would use "RDS: TCP:" for first patch as well but I let you
take a call :-)
Working on patchv2 that will address Sergei's comments and the
kbuild-test-robot warning as well
OK. How about the dropped WARN_ON() question ?
$subject
s/net/rds:/RDS:
On 9/30/2015 6:45 AM, Sowmini Varadhan wrote:
Commit f711a6ae062c ("net/rds: RDS-TCP: Always create a new rds_sock
for an incoming connection.") modified rds-tcp so that an incoming SYN
would ignore an existing "client" TCP connection which had the local
port set to the transient port. The motivation for ignoring the existing
"client" connection in f711a6ae was to avoid race conditions and an
endless duel of reconnect attempts triggered by a restart/abort of one
of the nodes in the TCP connection.
However, having separate sockets for active and passive sides
is avoidable, and the simpler model of a single TCP socket for
both send and receives of all RDS connections associated with
that tcp socket makes for easier observability. We avoid the race
conditions from f711a6ae by attempting reconnects in rds_conn_shutdown
if, and only if, the (new) c_outgoing bit is set for RDS_TRANS_TCP.
The c_outgoing bit is initialized in __rds_conn_create().
A side-effect of re-using the client rds_connection for an incoming
SYN is the potential of encountering duelling SYNs, i.e., we
have an outgoing RDS_CONN_CONNECTING socket when we get the incoming
SYN. The logic to arbitrate this criss-crossing SYN exchange in
rds_tcp_accept_one() has been modified to emulate the BGP state
machine: the smaller IP address should back off from the connection attempt.
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@xxxxxxxxxx>
---
net/rds/connection.c | 22 ++++++----------------
net/rds/rds.h | 4 +++-
net/rds/tcp_listen.c | 19 +++++++------------
3 files changed, 16 insertions(+), 29 deletions(-)
[...]
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 444d78d..ee70d13 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -110,28 +110,23 @@ int rds_tcp_accept_one(struct socket *sock)
goto out;
}
/* An incoming SYN request came in, and TCP just accepted it.
- * We always create a new conn for listen side of TCP, and do not
- * add it to the c_hash_list.
*
* If the client reboots, this conn will need to be cleaned up.
* rds_tcp_state_change() will do that cleanup
*/
rs_tcp = (struct rds_tcp_connection *)conn->c_transport_data;
- WARN_ON(!rs_tcp || rs_tcp->t_sock);
+ if (rs_tcp->t_sock && inet->inet_saddr < inet->inet_daddr) {
+ struct sock *nsk = new_sock->sk;
Any reason you dropped the WARN_ON. Note that till we got commit
74e98eb0 (" RDS: verify the underlying transport exists before creating
a connection") merged, we had an issue. That guards it now.
Am curious about WARN_ON() and hence the question.
Rest of the patch looks fine to me.
Acked-by: Santosh Shilimkar <santosh.shilimkar@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/