From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755185AbbI3PuU (ORCPT ); Wed, 30 Sep 2015 11:50:20 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:42878 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752671AbbI3PuP (ORCPT ); Wed, 30 Sep 2015 11:50:15 -0400 Subject: Re: [PATCH net-next 1/3] net/rds: Use a single TCP socket for both send and receive. To: Sowmini Varadhan , netdev@vger.kernel.org, linux-kernel@vger.kernel.org References: <603874beb1dedd0a989d84740d22565bfc568c97.1443569239.git.sowmini.varadhan@oracle.com> Cc: davem@davemloft.net, rds-devel@oss.oracle.com, ajaykumar.hotchandani@oracle.com, igor.maximov@oracle.com From: santosh shilimkar Organization: Oracle Corporation Message-ID: <560C04AA.4050201@oracle.com> Date: Wed, 30 Sep 2015 08:50:02 -0700 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <603874beb1dedd0a989d84740d22565bfc568c97.1443569239.git.sowmini.varadhan@oracle.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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:" $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 > --- > 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