Re: [PATCH net-next 1/3] net/rds: Use a single TCP socket for both send and receive.

[Sowmini Varadhan]

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. 

Working on patchv2 that will address Sergei's comments and the
kbuild-test-robot warning as well

> 
> $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/