From: Wei Lin Guay Date: Fri, 28 Apr 2017 23:18:58 +0000 (+0200) Subject: net/rds: Revert "RDS: add reconnect retry scheme for stalled connections" X-Git-Tag: v4.1.12-105.0.20170622_2100~3 X-Git-Url: https://www.infradead.org/git/?a=commitdiff_plain;h=1e1fbcc5ce19b2cbc020a8831379ab053ab16b8d;p=users%2Fjedix%2Flinux-maple.git net/rds: Revert "RDS: add reconnect retry scheme for stalled connections" This reverts commit 5acb959ad59966b0b6905802ed720d26c560c3c5. Commit "RDS: add reconnect retry scheme for stalled connections" introduces the rds_reconnect_timeout to retry the connection establishment after sysctl_reconnect_retry_ms (default is 1000ms). Nevertheless, this proactive mechanism is overkilled and it is causing long brownout time in the virtualized environment. In short, below are the justifications why commit 5acb959ad59966b0b6905802ed720d26c560c3c5 is reverted. a) The retry counter starts ticking after RDS received an ADDR_CHANGE event. After receiving an ADDR_CHANGE event, RDS needs to perform shutdown via shutdown_worker. Then, initiate a new connection via connect_worker. Eventually, a CM REQ is only sent out after rds received RDMA_CM_EVENT_ADDR_RESOLVED and RDMA_CM_EVENT_ROUTE_RESOLVED events. If the retry is made to cater for stalled connection due to missing CM messages, the retry should only happen after a CM REQ is sent. With the current retry scheme (and with the default 1000 ms) that happens after ADDR_CHANGE event, it introduces congestion in the single threaded workqueue. b) Assuming that we modify the retry counter to start ticking after a CM REQ message is sent out. By introducing another retry timeout, it complicates the system tuning. Why? First, the sysctl_reconnect_retry_ms relies on the underlying cma_response_timeout. Any modication of cma_response_timeout requires to tune sysctl_reconnect_retry_ms. Second, it is hard to find a universal timing that fits all configurations (bare-metal, virtualized, mixed environment, and homo/heterogenous system). Orabug: 25521901 Signed-off-by: Wei Lin Guay Reviewed-by: Håkon Bugge Tested-by: Dib Chatterjee Tested-by: Rosa Isela Lopez Romero Acked-by: Ajaykumar Hotchandani --- diff --git a/net/rds/connection.c b/net/rds/connection.c index 97639fb0d5ee..19e259945d38 100644 --- a/net/rds/connection.c +++ b/net/rds/connection.c @@ -251,9 +251,6 @@ static struct rds_connection *__rds_conn_create(struct net *net, cp = &conn->c_path[i]; __rds_conn_path_init(conn, cp, is_outgoing); cp->cp_index = i; - cp->cp_reconnect_retry = rds_sysctl_reconnect_retry_ms; - cp->cp_reconnect_retry_count = 0; - if (conn->c_loopback) cp->cp_wq = rds_local_wq; else diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c index c4103f4c0ad0..4485f02d8119 100644 --- a/net/rds/ib_cm.c +++ b/net/rds/ib_cm.c @@ -206,7 +206,7 @@ static int rds_ib_match_acl(struct rdma_cm_id *cm_id, __be32 saddr) __be64 fguid = cm_id->route.path_rec->dgid.global.interface_id; __be64 fsubnet = cm_id->route.path_rec->dgid.global.subnet_prefix; struct ib_cm_dpp dpp; - u32 addr; + u32 addr; ib_cm_dpp_init(&dpp, cm_id->device, cm_id->port_num, ntohs(cm_id->route.path_rec->pkey)); @@ -929,24 +929,22 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id, rds_ib_stats_inc(s_ib_listen_closed_stale); } else if (rds_conn_state(conn) == RDS_CONN_CONNECTING) { unsigned long now = get_seconds(); - unsigned long retry = - conn->c_path[0].cp_reconnect_retry; - - /* after retry seconds, give up on - * existing connection attempts and try again. - * At this point it's no longer backoff race but - * something has gone horribly wrong. + /* + * after 15 seconds, give up on existing connection + * attempts and make them try again. At this point + * it's no longer a race but something has gone + * horribly wrong */ - retry = DIV_ROUND_UP(retry, 1000); if (now > conn->c_connection_start && - now - conn->c_connection_start > retry) { - pr_info("RDS/IB: conn <%pI4,%pI4,%d> racing for more than %lus, retry\n", - &conn->c_laddr, &conn->c_faddr, - conn->c_tos, retry); - set_bit(RDS_RECONNECT_TIMEDOUT, - &conn->c_path[0].cp_reconn_flags); - rds_conn_drop(conn, DR_RECONNECT_TIMEOUT); + now - conn->c_connection_start > 15) { + printk(KERN_CRIT "RDS/IB: connection " + "<%u.%u.%u.%u,%u.%u.%u.%u,%d> " + "racing for 15s, forcing reset ", + NIPQUAD(conn->c_laddr), + NIPQUAD(conn->c_faddr), + conn->c_tos); + rds_conn_drop(conn, DR_IB_REQ_WHILE_CONNECTING); rds_ib_stats_inc(s_ib_listen_closed_stale); } else { /* Wait and see - our connect may still be succeeding */ diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c index 8b472640f82d..4b4e442aef1d 100644 --- a/net/rds/rdma_transport.c +++ b/net/rds/rdma_transport.c @@ -306,12 +306,8 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id, "ADDR_CHANGE: calling rds_conn_drop <%u.%u.%u.%u,%u.%u.%u.%u,%d>\n", NIPQUAD(conn->c_laddr), NIPQUAD(conn->c_faddr), conn->c_tos); - if (!rds_conn_self_loopback_passive(conn)) { - queue_delayed_work(conn->c_path[0].cp_wq, - &conn->c_path[0].cp_reconn_w, - msecs_to_jiffies(conn->c_path[0].cp_reconnect_retry)); + if (!rds_conn_self_loopback_passive(conn)) rds_conn_drop(conn, DR_IB_ADDR_CHANGE); - } } break; diff --git a/net/rds/rds.h b/net/rds/rds.h index 5cb7109da4bd..9c2ec849d22e 100644 --- a/net/rds/rds.h +++ b/net/rds/rds.h @@ -140,14 +140,10 @@ enum { #define RDS_RDMA_RESOLVE_TO_MAX_INDEX 5 #define RDS_ADDR_RES_TM_INDEX_MAX 5 -/* Bits for c_reconn_flags */ -#define RDS_RECONNECT_TIMEDOUT 0 - /* Max number of multipaths per RDS connection. Must be a power of 2 */ #define RDS_MPATH_WORKERS 8 #define RDS_MPATH_HASH(rs, n) (jhash_1word((rs)->rs_bound_port, \ (rs)->rs_hash_initval) & ((n) - 1)) - enum rds_conn_drop_src { /* rds-core */ DR_DEFAULT, @@ -320,6 +316,15 @@ struct rds_connection { unsigned int c_version; struct net *c_net; + /* Re-connect stall diagnostics */ + unsigned long c_reconnect_start; + unsigned int c_reconnect_drops; + int c_reconnect_warn; + int c_reconnect_err; + int c_to_index; + + unsigned int c_reconnect; + /* Qos support */ u8 c_tos; @@ -1126,8 +1131,6 @@ extern unsigned long rds_sysctl_trace_flags; extern unsigned int rds_sysctl_trace_level; extern unsigned int rds_sysctl_shutdown_trace_start_time; extern unsigned int rds_sysctl_shutdown_trace_end_time; -extern unsigned long rds_sysctl_reconnect_retry_ms; -extern unsigned int rds_sysctl_reconnect_max_retries; /* threads.c */ int rds_threads_init(void); diff --git a/net/rds/sysctl.c b/net/rds/sysctl.c index 64b4e77f7855..b22e8b8b6b89 100644 --- a/net/rds/sysctl.c +++ b/net/rds/sysctl.c @@ -52,13 +52,6 @@ unsigned int rds_sysctl_ping_enable = 1; unsigned int rds_sysctl_shutdown_trace_start_time; unsigned int rds_sysctl_shutdown_trace_end_time; -unsigned long rds_sysctl_reconnect_retry_ms = 1000; -static unsigned long reconnect_retry_ms_min = 100; -static unsigned long reconnect_retry_ms_max = 15000; - -unsigned int rds_sysctl_reconnect_max_retries = 60; -static unsigned long reconnect_min_retries = 15; - /* * We have official values, but must maintain the sysctl interface for existing * software that expects to find these values here. @@ -133,25 +126,6 @@ static struct ctl_table rds_sysctl_rds_table[] = { .maxlen = sizeof(int), .mode = 0644, .proc_handler = &proc_dointvec, - - }, - { - .procname = "reconnect_retry_ms", - .data = &rds_sysctl_reconnect_retry_ms, - .maxlen = sizeof(unsigned long), - .mode = 0644, - .proc_handler = proc_dointvec_minmax, - .extra1 = &reconnect_retry_ms_min, - .extra2 = &reconnect_retry_ms_max, - }, - { - .procname = "reconnect_max_retries", - .data = &rds_sysctl_reconnect_max_retries, - .maxlen = sizeof(unsigned int), - .mode = 0644, - .proc_handler = proc_dointvec_minmax, - .extra1 = &reconnect_min_retries, - .extra2 = &rds_sysctl_reconnect_max_retries, }, { } }; diff --git a/net/rds/threads.c b/net/rds/threads.c index dda33cda1331..dc76c0c90df3 100644 --- a/net/rds/threads.c +++ b/net/rds/threads.c @@ -93,8 +93,6 @@ void rds_connect_path_complete(struct rds_conn_path *cp, int curr) conn, &conn->c_laddr, &conn->c_faddr, conn->c_tos); cp->cp_reconnect_jiffies = 0; - cp->cp_reconnect_retry = rds_sysctl_reconnect_retry_ms; - cp->cp_reconnect_retry_count = 0; set_bit(0, &conn->c_map_queued); queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 0); queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0); @@ -148,8 +146,7 @@ void rds_queue_reconnect(struct rds_conn_path *cp) return; set_bit(RDS_RECONNECT_PENDING, &cp->cp_flags); - if (cp->cp_reconnect_jiffies == 0 || - test_and_clear_bit(RDS_RECONNECT_TIMEDOUT, &cp->cp_reconn_flags)) { + if (cp->cp_reconnect_jiffies == 0) { cp->cp_reconnect_jiffies = rds_sysctl_reconnect_min_jiffies; queue_delayed_work(cp->cp_wq, &cp->cp_conn_w, 0); return; @@ -319,29 +316,12 @@ void rds_reconnect_timeout(struct work_struct *work) cp_reconn_w.work); struct rds_connection *conn = cp->cp_conn; - if (cp->cp_reconnect_retry_count > rds_sysctl_reconnect_max_retries) { - pr_info("RDS: connection <%pI4,%pI4,%d> reconnect retries(%d) exceeded, stop retry\n", + if (!rds_conn_path_up(cp)) { + rds_rtd(RDS_RTD_CM, + "conn <%pI4,%pI4,%d> not up, retry(%d)\n", &conn->c_laddr, &conn->c_faddr, conn->c_tos, cp->cp_reconnect_retry_count); - return; - } - - if (!rds_conn_path_up(cp)) { - if (rds_conn_path_state(cp) == RDS_CONN_DISCONNECTING) { - queue_delayed_work(cp->cp_wq, &cp->cp_reconn_w, - msecs_to_jiffies(100)); - } else { - cp->cp_reconnect_retry_count++; - rds_rtd(RDS_RTD_CM, - "conn <%pI4,%pI4,%d> not up, retry(%d)\n", - &conn->c_laddr, &conn->c_faddr, conn->c_tos, - cp->cp_reconnect_retry_count); - queue_delayed_work(cp->cp_wq, &cp->cp_reconn_w, - msecs_to_jiffies( - cp->cp_reconnect_retry)); - set_bit(RDS_RECONNECT_TIMEDOUT, &cp->cp_reconn_flags); - rds_conn_path_drop(cp, DR_RECONNECT_TIMEOUT); - } + rds_conn_path_drop(cp, DR_RECONNECT_TIMEOUT); } }