netdev - Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+98ifRj9SJQbK+QJrCde2UJvWr1h31gAZSuxt4i_U=iw@mail.gmail.com>
Date: Wed, 13 Dec 2023 15:10:00 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Salvatore Dipietro <dipiets@...zon.com>, davem@...emloft.net, dsahern@...nel.org, 
	kuba@...nel.org, netdev@...r.kernel.org, blakgeof@...zon.com, 
	alisaidi@...zon.com, benh@...zon.com, dipietro.salvatore@...il.com
Subject: Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY
 flag is set

On Tue, Dec 12, 2023 at 11:58 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On Fri, 2023-12-08 at 10:20 -0800, Salvatore Dipietro wrote:
> > Based on the tcp man page, if TCP_NODELAY is set, it disables Nagle's algorithm
> > and packets are sent as soon as possible. However in the `tcp_push` function
> > where autocorking is evaluated the `nonagle` value set by TCP_NODELAY is not
> > considered which can trigger unexpected corking of packets and induce delays.
> >
> > For example, if two packets are generated as part of a server's reply, if the
> > first one is not transmitted on the wire quickly enough, the second packet can
> > trigger the autocorking in `tcp_push` and be delayed instead of sent as soon as
> > possible. It will either wait for additional packets to be coalesced or an ACK
> > from the client before transmitting the corked packet. This can interact badly
> > if the receiver has tcp delayed acks enabled, introducing 40ms extra delay in
> > completion times. It is not always possible to control who has delayed acks
> > set, but it is possible to adjust when and how autocorking is triggered.
> > Patch prevents autocorking if the TCP_NODELAY flag is set on the socket.
> >
> > Patch has been tested using an AWS c7g.2xlarge instance with Ubuntu 22.04 and
> > Apache Tomcat 9.0.83 running the basic servlet below:
> >
> > import java.io.IOException;
> > import java.io.OutputStreamWriter;
> > import java.io.PrintWriter;
> > import javax.servlet.ServletException;
> > import javax.servlet.http.HttpServlet;
> > import javax.servlet.http.HttpServletRequest;
> > import javax.servlet.http.HttpServletResponse;
> >
> > public class HelloWorldServlet extends HttpServlet {
> >     @Override
> >     protected void doGet(HttpServletRequest request, HttpServletResponse response)
> >       throws ServletException, IOException {
> >         response.setContentType("text/html;charset=utf-8");
> >         OutputStreamWriter osw = new OutputStreamWriter(response.getOutputStream(),"UTF-8");
> >         String s = "a".repeat(3096);
> >         osw.write(s,0,s.length());
> >         osw.flush();
> >     }
> > }
> >
> > Load was applied using  wrk2 (https://github.com/kinvolk/wrk2) from an AWS
> > c6i.8xlarge instance.  With the current auto-corking behavior and TCP_NODELAY
> > set an additional 40ms latency from P99.99+ values are observed.  With the
> > patch applied we see no occurrences of 40ms latencies. The patch has also been
> > tested with iperf and uperf benchmarks and no regression was observed.
> >
> > # No patch with tcp_autocorking=1 and TCP_NODELAY set on all sockets
> > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.49.177:8080/hello/hello'
> >   ...
> >  50.000%    0.91ms
> >  75.000%    1.12ms
> >  90.000%    1.46ms
> >  99.000%    1.73ms
> >  99.900%    1.96ms
> >  99.990%   43.62ms   <<< 40+ ms extra latency
> >  99.999%   48.32ms
> > 100.000%   49.34ms
> >
> > # With patch
> > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.49.177:8080/hello/hello'
> >   ...
> >  50.000%    0.89ms
> >  75.000%    1.13ms
> >  90.000%    1.44ms
> >  99.000%    1.67ms
> >  99.900%    1.78ms
> >  99.990%    2.27ms   <<< no 40+ ms extra latency
> >  99.999%    3.71ms
> > 100.000%    4.57ms
> >
> > Signed-off-by: Salvatore Dipietro <dipiets@...zon.com>
> > ---
> >  net/ipv4/tcp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index d3456cf840de..87751a2a6fff 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -716,7 +716,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
> >
> >       tcp_mark_urg(tp, flags);
> >
> > -     if (tcp_should_autocork(sk, skb, size_goal)) {
> > +     if (!nonagle && tcp_should_autocork(sk, skb, size_goal)) {
>
> It looks like the above disables autocorking even after the userspace
> sets TCP_CORK. Am I reading it correctly?Sal Is that expected?
>

Yes, it seems the patch went too far.

Also I wonder about these 40ms delays, TCP small queue handler should
kick when the prior skb is TX completed.

It seems the issue is on the driver side ?

Salvatore, which driver are you using ?