| Message ID | 20200710141053.65581-1-kuniyu@amazon.co.jp |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3,net-next] inet: Remove an unnecessary argument of syn_ack_recalc(). | expand |
On Fri, Jul 10, 2020 at 7:11 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> 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 three changes: > - remove redundant non-zero check for rskq_defer_accept in > reqsk_timer_handler(). > - remove max_retries from the arguments of syn_ack_recalc() and use > rskq_defer_accept instead. > - rename thresh to max_syn_ack_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 | 33 +++++++++++++++------------------ > 1 file changed, 15 insertions(+), 18 deletions(-) > > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > index afaf582a5aa9..21bc80a3c7cf 100644 > --- a/net/ipv4/inet_connection_sock.c > +++ b/net/ipv4/inet_connection_sock.c > @@ -648,20 +648,23 @@ 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, While we are at it, please remove the inline keyword. > + const int max_syn_ack_retries, > const u8 rskq_defer_accept, > int *expire, int *resend) > { > if (!rskq_defer_accept) { > - *expire = req->num_timeout >= thresh; > + *expire = req->num_timeout >= max_syn_ack_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. > + */ I honestly do not believe this comment is needed. The bare ack has not been 'dropped' since it had the effect of validating the 3WHS. I find it confusing, and not describing the order of the conditions expressed in the C code. > + *expire = req->num_timeout >= max_syn_ack_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 +723,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_syn_ack_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; > + max_syn_ack_retries = icsk->icsk_syn_retries ? : net->ipv4.sysctl_tcp_synack_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 +750,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_syn_ack_retries > 2) { > if (qlen < young) > break; > - thresh--; > + max_syn_ack_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_syn_ack_retries, READ_ONCE(queue->rskq_defer_accept), > &expire, &resend); > req->rsk_ops->syn_ack_timeout(req); > if (!expire && > -- > 2.17.2 (Apple Git-113) >
From: Eric Dumazet <edumazet@google.com> Date: Fri, 10 Jul 2020 08:20:03 -0700 > On Fri, Jul 10, 2020 at 7:11 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> 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 three changes: > > - remove redundant non-zero check for rskq_defer_accept in > > reqsk_timer_handler(). > > - remove max_retries from the arguments of syn_ack_recalc() and use > > rskq_defer_accept instead. > > - rename thresh to max_syn_ack_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 | 33 +++++++++++++++------------------ > > 1 file changed, 15 insertions(+), 18 deletions(-) > > > > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > > index afaf582a5aa9..21bc80a3c7cf 100644 > > --- a/net/ipv4/inet_connection_sock.c > > +++ b/net/ipv4/inet_connection_sock.c > > @@ -648,20 +648,23 @@ 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, > > While we are at it, please remove the inline keyword. I will remove 'inline' in next spin. > > + const int max_syn_ack_retries, > > const u8 rskq_defer_accept, > > int *expire, int *resend) > > { > > if (!rskq_defer_accept) { > > - *expire = req->num_timeout >= thresh; > > + *expire = req->num_timeout >= max_syn_ack_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. > > + */ > > I honestly do not believe this comment is needed. > The bare ack has not been 'dropped' since it had the effect of > validating the 3WHS. > I find it confusing, and not describing the order of the conditions > expressed in the C code. Exactly, thank you for correcting my misunderstanding. I will remove the comment. Best Regards, Kuniyuki
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index afaf582a5aa9..21bc80a3c7cf 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -648,20 +648,23 @@ 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_syn_ack_retries, const u8 rskq_defer_accept, int *expire, int *resend) { if (!rskq_defer_accept) { - *expire = req->num_timeout >= thresh; + *expire = req->num_timeout >= max_syn_ack_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_syn_ack_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 +723,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_syn_ack_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; + max_syn_ack_retries = icsk->icsk_syn_retries ? : net->ipv4.sysctl_tcp_synack_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 +750,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_syn_ack_retries > 2) { if (qlen < young) break; - thresh--; + max_syn_ack_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_syn_ack_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 three changes: - remove redundant non-zero check for rskq_defer_accept in reqsk_timer_handler(). - remove max_retries from the arguments of syn_ack_recalc() and use rskq_defer_accept instead. - rename thresh to max_syn_ack_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 | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-)