Re: [PATCH net v2] Revert "tcp: change pingpong threshold to 3"

From: Jiri Slaby <jirislaby@kernel.org>
To: Wei Wang <weiwan@google.com>, David Miller <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org
Cc: Soheil Hassas Yeganeh <soheil@google.com>,
	Yuchung Cheng <ycheng@google.com>, LemmyHuang <hlm3280@163.com>,
	Neal Cardwell <ncardwell@google.com>,
	stable <stable@vger.kernel.org>
Subject: Re: [PATCH net v2] Revert "tcp: change pingpong threshold to 3"
Date: Sat, 6 Aug 2022 12:02:01 +0200	[thread overview]
Message-ID: <ca408271-8730-eb2b-f12e-3f66df2e643a@kernel.org> (raw)
In-Reply-To: <20220721204404.388396-1-weiwan@google.com>

On 21. 07. 22, 22:44, Wei Wang wrote:
> This reverts commit 4a41f453bedfd5e9cd040bad509d9da49feb3e2c.
> 
> This to-be-reverted commit was meant to apply a stricter rule for the
> stack to enter pingpong mode. However, the condition used to check for
> interactive session "before(tp->lsndtime, icsk->icsk_ack.lrcvtime)" is
> jiffy based and might be too coarse, which delays the stack entering
> pingpong mode.
> We revert this patch so that we no longer use the above condition to
> determine interactive session, and also reduce pingpong threshold to 1.
> 
> Fixes: 4a41f453bedf ("tcp: change pingpong threshold to 3")
> Reported-by: LemmyHuang <hlm3280@163.com>
> Suggested-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Wei Wang <weiwan@google.com>

This breaks python-eventlet [1] (and was backported to stable trees):
________________ TestHttpd.test_018b_http_10_keepalive_framing 
_________________

self = <tests.wsgi_test.TestHttpd 
testMethod=test_018b_http_10_keepalive_framing>

     def test_018b_http_10_keepalive_framing(self):
         # verify that if an http/1.0 client sends connection: keep-alive
         # that we don't mangle the request framing if the app doesn't 
read the request
         def app(environ, start_response):
             resp_body = {
                 '/1': b'first response',
                 '/2': b'second response',
                 '/3': b'third response',
             }.get(environ['PATH_INFO'])
             if resp_body is None:
                 resp_body = 'Unexpected path: ' + environ['PATH_INFO']
                 if six.PY3:
                     resp_body = resp_body.encode('latin1')
             # Never look at wsgi.input!
             start_response('200 OK', [('Content-type', 'text/plain')])
             return [resp_body]

         self.site.application = app
         sock = eventlet.connect(self.server_addr)
         req_body = b'GET /tricksy HTTP/1.1\r\n'
         body_len = str(len(req_body)).encode('ascii')

         sock.sendall(b'PUT /1 HTTP/1.0\r\nHost: 
localhost\r\nConnection: keep-alive\r\n'
                      b'Content-Length: ' + body_len + b'\r\n\r\n' + 
req_body)
         result1 = read_http(sock)
         self.assertEqual(b'first response', result1.body)
         self.assertEqual(result1.headers_original.get('Connection'), 
'keep-alive')

         sock.sendall(b'PUT /2 HTTP/1.0\r\nHost: 
localhost\r\nConnection: keep-alive\r\n'
                      b'Content-Length: ' + body_len + b'\r\nExpect: 
100-continue\r\n\r\n')
         # Client may have a short timeout waiting on that 100 Continue
         # and basically immediately send its body
         sock.sendall(req_body)
         result2 = read_http(sock)
         self.assertEqual(b'second response', result2.body)
         self.assertEqual(result2.headers_original.get('Connection'), 
'close')

 >       sock.sendall(b'PUT /3 HTTP/1.0\r\nHost: 
localhost\r\nConnection: close\r\n\r\n')

tests/wsgi_test.py:648:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _
eventlet/greenio/base.py:407: in sendall
     tail = self.send(data, flags)
eventlet/greenio/base.py:401: in send
     return self._send_loop(self.fd.send, data, flags)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
_ _ _ _

self = <eventlet.greenio.base.GreenSocket object at 0x7f5f2f73c9a0>
send_method = <built-in method send of socket object at 0x7f5f2f73d520>
data = b'PUT /3 HTTP/1.0\r\nHost: localhost\r\nConnection: close\r\n\r\n'
args = (0,), _timeout_exc = timeout('timed out'), eno = 32

     def _send_loop(self, send_method, data, *args):
         if self.act_non_blocking:
             return send_method(data, *args)

         _timeout_exc = socket_timeout('timed out')
         while True:
             try:
 >               return send_method(data, *args)
E               BrokenPipeError: [Errno 32] Broken pipe

eventlet/greenio/base.py:388: BrokenPipeError
====================

Reverting this revert on the top of 5.19 solves the issue.

Any ideas?

[1] https://github.com/eventlet/eventlet

> ---
> v2: added Fixes tag
> 
> ---
>   include/net/inet_connection_sock.h | 10 +---------
>   net/ipv4/tcp_output.c              | 15 ++++++---------
>   2 files changed, 7 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
> index 85cd695e7fd1..ee88f0f1350f 100644
> --- a/include/net/inet_connection_sock.h
> +++ b/include/net/inet_connection_sock.h
> @@ -321,7 +321,7 @@ void inet_csk_update_fastreuse(struct inet_bind_bucket *tb,
>   
>   struct dst_entry *inet_csk_update_pmtu(struct sock *sk, u32 mtu);
>   
> -#define TCP_PINGPONG_THRESH	3
> +#define TCP_PINGPONG_THRESH	1
>   
>   static inline void inet_csk_enter_pingpong_mode(struct sock *sk)
>   {
> @@ -338,14 +338,6 @@ static inline bool inet_csk_in_pingpong_mode(struct sock *sk)
>   	return inet_csk(sk)->icsk_ack.pingpong >= TCP_PINGPONG_THRESH;
>   }
>   
> -static inline void inet_csk_inc_pingpong_cnt(struct sock *sk)
> -{
> -	struct inet_connection_sock *icsk = inet_csk(sk);
> -
> -	if (icsk->icsk_ack.pingpong < U8_MAX)
> -		icsk->icsk_ack.pingpong++;
> -}
> -
>   static inline bool inet_csk_has_ulp(struct sock *sk)
>   {
>   	return inet_sk(sk)->is_icsk && !!inet_csk(sk)->icsk_ulp_ops;
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index c38e07b50639..d06e72e141ac 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -167,16 +167,13 @@ static void tcp_event_data_sent(struct tcp_sock *tp,
>   	if (tcp_packets_in_flight(tp) == 0)
>   		tcp_ca_event(sk, CA_EVENT_TX_START);
>   
> -	/* If this is the first data packet sent in response to the
> -	 * previous received data,
> -	 * and it is a reply for ato after last received packet,
> -	 * increase pingpong count.
> -	 */
> -	if (before(tp->lsndtime, icsk->icsk_ack.lrcvtime) &&
> -	    (u32)(now - icsk->icsk_ack.lrcvtime) < icsk->icsk_ack.ato)
> -		inet_csk_inc_pingpong_cnt(sk);
> -
>   	tp->lsndtime = now;
> +
> +	/* If it is a reply for ato after last received
> +	 * packet, enter pingpong mode.
> +	 */
> +	if ((u32)(now - icsk->icsk_ack.lrcvtime) < icsk->icsk_ack.ato)
> +		inet_csk_enter_pingpong_mode(sk);
>   }
>   
>   /* Account for an ACK we sent. */

thanks,
-- 
js
suse labs