| Message ID | 20200704152852.39935-1-kuniyu@amazon.co.jp |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2,net-next] inet: Remove an unnecessary argument of syn_ack_recalc(). | expand |
On 7/4/20 8:28 AM, Kuniyuki Iwashima wrote: > Commit 0c3d79bce48034018e840468ac5a642894a521a3 ("tcp: reduce SYN-ACK > retrans for TCP_DEFER_ACCEPT") introduces syn_ack_recalc() which decides > if a minisock is held and a SYN+ACK is retransmitted or not. > > If rskq_defer_accept is not zero in syn_ack_recalc(), max_retries always > has the same value because max_retries is overwritten by rskq_defer_accept > in reqsk_timer_handler(). > > This commit adds two changes: > - remove max_retries from the arguments of syn_ack_recalc() and use > rskq_defer_accept instead. > - rename thresh to max_retries for readability. > Honestly this looks unnecessary code churn to me. This will make future backports more error prone. Real question is : why do you want this change in the first place ?
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Sun, 5 Jul 2020 10:08:08 -0700 > On 7/4/20 8:28 AM, Kuniyuki Iwashima wrote: > > Commit 0c3d79bce48034018e840468ac5a642894a521a3 ("tcp: reduce SYN-ACK > > retrans for TCP_DEFER_ACCEPT") introduces syn_ack_recalc() which decides > > if a minisock is held and a SYN+ACK is retransmitted or not. > > > > If rskq_defer_accept is not zero in syn_ack_recalc(), max_retries always > > has the same value because max_retries is overwritten by rskq_defer_accept > > in reqsk_timer_handler(). > > > > This commit adds two changes: > > - remove max_retries from the arguments of syn_ack_recalc() and use > > rskq_defer_accept instead. > > - rename thresh to max_retries for readability. > > > > Honestly this looks unnecessary code churn to me. > > This will make future backports more error prone. > > Real question is : why do you want this change in the first place ? The current code does non-zero checks for rskq_defer_accept twice in reqsk_timer_handler() and syn_ack_recalc(), the former of which is redundant. Also, max_retries can have two meanings in reqsk_timer_handler() depending on TCP_DEFER_ACCEPT: - the number of retries to resend SYN+ACK (unused) - the number of retries to drop bare ACK On the other hand, the max_retries in reqsk_timer_handler() has only the latter meaning and is confusing because rskq_defer_accept has the same (original) value and the both values are used. As far as I see, in the original code, the non-zero check was reasonable because it was done once and the max_retries was evaluated through the function (tcp_synack_timer()). $ git blame net/ipv4/tcp_timer.c 1944972d3bb651474a5021c9da8d0166ae19f1eb ... ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 464) static void tcp_synack_timer(struct sock *sk) ... ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 468) int max_retries = tp->syn_retries ? : sysctl_tcp_synack_retries; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 469) int thresh = max_retries; ... ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 505) if (tp->defer_accept) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 506) max_retries = tp->defer_accept; ... ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 515) if ((req->retrans < thresh || ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 516) (req->acked && req->retrans < max_retries)) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 517) && !req->class->rtx_syn_ack(sk, req, NULL)) { Currently, the code already looks a bit churned and error-prone. It might be because of the ambiguity of the name of max_retries. rskq_defer_accept is assigned to max_retries but not always "max". The code checks thresh at first, and then max_retries. So, as a result of the evaluation order, it can be "max" (also may be smaller than thresh). Moreover, in this context, there are three kinds of "retries": timer (num_timeout), resending SYN+ACK (thresh), and dropping bare ACK (max_retries and rskq_defer_accept). In the original code, it was OK because we did not use rskq_defer_accept twice. The commit introduces syn_ack_recalc() and delegates the decision of retries to the function. I think it is better to - remove the redundant check of rskq_defer_accept - pass only necessary arguments to syn_ack_recalc() - use a more understandable name instead of max_retries in two functions. For example, max_resends and rskq_defer_accept, or max_syn_ack_retries and rskq_defer_accept. (I am not confident about what is the most understandable name for anyone.) So, I would like to respin the patch rephrasing max_retries to the proper name. What would you think about this? Sincerely, Kuniyuki
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index afaf582a5aa9..323cdb8ce901 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -648,20 +648,22 @@ struct dst_entry *inet_csk_route_child_sock(const struct sock *sk, EXPORT_SYMBOL_GPL(inet_csk_route_child_sock); /* Decide when to expire the request and when to resend SYN-ACK */ -static inline void syn_ack_recalc(struct request_sock *req, const int thresh, - const int max_retries, +static inline void syn_ack_recalc(struct request_sock *req, const int max_retries, const u8 rskq_defer_accept, int *expire, int *resend) { if (!rskq_defer_accept) { - *expire = req->num_timeout >= thresh; + *expire = req->num_timeout >= max_retries; *resend = 1; return; } - *expire = req->num_timeout >= thresh && - (!inet_rsk(req)->acked || req->num_timeout >= max_retries); - /* - * Do not resend while waiting for data after ACK, + /* If a bare ACK has already been dropped, the client is alive, so + * do not free the request_sock to drop a bare ACK at most + * rskq_defer_accept times and wait for data. + */ + *expire = req->num_timeout >= max_retries && + (!inet_rsk(req)->acked || req->num_timeout >= rskq_defer_accept); + /* Do not resend while waiting for data after ACK, * start to resend on end of deferring period to give * last chance for data or ACK to create established socket. */ @@ -720,15 +722,12 @@ static void reqsk_timer_handler(struct timer_list *t) struct net *net = sock_net(sk_listener); struct inet_connection_sock *icsk = inet_csk(sk_listener); struct request_sock_queue *queue = &icsk->icsk_accept_queue; - int qlen, expire = 0, resend = 0; - int max_retries, thresh; - u8 defer_accept; + int max_retries, qlen, expire = 0, resend = 0; if (inet_sk_state_load(sk_listener) != TCP_LISTEN) goto drop; max_retries = icsk->icsk_syn_retries ? : net->ipv4.sysctl_tcp_synack_retries; - thresh = max_retries; /* Normally all the openreqs are young and become mature * (i.e. converted to established socket) for first timeout. * If synack was not acknowledged for 1 second, it means @@ -750,17 +749,14 @@ static void reqsk_timer_handler(struct timer_list *t) if ((qlen << 1) > max(8U, READ_ONCE(sk_listener->sk_max_ack_backlog))) { int young = reqsk_queue_len_young(queue) << 1; - while (thresh > 2) { + while (max_retries > 2) { if (qlen < young) break; - thresh--; + max_retries--; young <<= 1; } } - defer_accept = READ_ONCE(queue->rskq_defer_accept); - if (defer_accept) - max_retries = defer_accept; - syn_ack_recalc(req, thresh, max_retries, defer_accept, + syn_ack_recalc(req, max_retries, READ_ONCE(queue->rskq_defer_accept), &expire, &resend); req->rsk_ops->syn_ack_timeout(req); if (!expire &&
Commit 0c3d79bce48034018e840468ac5a642894a521a3 ("tcp: reduce SYN-ACK retrans for TCP_DEFER_ACCEPT") introduces syn_ack_recalc() which decides if a minisock is held and a SYN+ACK is retransmitted or not. If rskq_defer_accept is not zero in syn_ack_recalc(), max_retries always has the same value because max_retries is overwritten by rskq_defer_accept in reqsk_timer_handler(). This commit adds two changes: - remove max_retries from the arguments of syn_ack_recalc() and use rskq_defer_accept instead. - rename thresh to max_retries for readability. Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> Reviewed-by: Benjamin Herrenschmidt <benh@amazon.com> CC: Julian Anastasov <ja@ssi.bg> --- net/ipv4/inet_connection_sock.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)