[PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Salvatore Dipietro]

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@amazon.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)) {
 
 		/* avoid atomic op if TSQ_THROTTLED bit is already set */
 		if (!test_bit(TSQ_THROTTLED, &sk->sk_tsq_flags)) {
-- 
2.42.0

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Eric Dumazet]

On Fri, Dec 8, 2023 at 7:23 PM Salvatore Dipietro <dipiets@amazon.com> 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;
>
> Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> ---

Very nice catch.

I suggest this patch lands in net tree, what about we add a Fixes: tag ?

Fixes: f54b311142a9 ("tcp: auto corking")
Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks !

[PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Hazem Mohamed Abuelfotoh]

It will be good to submit another version changing the documentation https://docs.kernel.org/networking/timestamping.html editing "It can prevent the situation by always flushing the TCP stack in between requests, for instance by enabling TCP_NODELAY and disabling TCP_CORK and autocork." to "It can prevent the situation by always flushing the TCP stack in between requests, for instance by enabling TCP_NODELAY and disabling TCP_CORK."

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Paolo Abeni]

On Mon, 2023-12-11 at 15:58 +0000, Hazem Mohamed Abuelfotoh wrote:
> It will be good to submit another version changing the documentation
> https://docs.kernel.org/networking/timestamping.html editing "It can
> prevent the situation by always flushing the TCP stack in between
> requests, for instance by enabling TCP_NODELAY and disabling TCP_CORK
> and autocork." to "It can prevent the situation by always flushing
> the TCP stack in between requests, for instance by enabling
> TCP_NODELAY and disabling TCP_CORK."

The documentation update could be a follow-up.

Please fix your email client, the message you sent was not properly
trimmed.

Cheers,

Paolo

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Eric Dumazet]

On Tue, Dec 12, 2023 at 11:41 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Mon, 2023-12-11 at 15:58 +0000, Hazem Mohamed Abuelfotoh wrote:
> > It will be good to submit another version changing the documentation
> > https://docs.kernel.org/networking/timestamping.html editing "It can
> > prevent the situation by always flushing the TCP stack in between
> > requests, for instance by enabling TCP_NODELAY and disabling TCP_CORK
> > and autocork." to "It can prevent the situation by always flushing
> > the TCP stack in between requests, for instance by enabling
> > TCP_NODELAY and disabling TCP_CORK."
>
> The documentation update could be a follow-up.

About timestamping documentation I sent something  cleaner I think

https://patchwork.kernel.org/project/netdevbpf/patch/20231212110608.3673677-1-edumazet@google.com/




>
> Please fix your email client, the message you sent was not properly
> trimmed.
>
> Cheers,
>
> Paolo
>

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Paolo Abeni]

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@amazon.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? Is that expected?

Cheers,

Paolo

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Eric Dumazet]

On Tue, Dec 12, 2023 at 11:58 AM Paolo Abeni <pabeni@redhat.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@amazon.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 ?

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Jakub Kicinski]

On Wed, 13 Dec 2023 15:10:00 +0100 Eric Dumazet wrote:
> > 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.

Reverted to avoid it getting to Linus in the meantime, FWIW.

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Eric Dumazet]

On Wed, Dec 13, 2023 at 8:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 13 Dec 2023 15:10:00 +0100 Eric Dumazet wrote:
> > > 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.
>
> Reverted to avoid it getting to Linus in the meantime, FWIW.

Thanks Jakub.

RE: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Salvatore Dipietro]

> It looks like the above disables autocorking even after the userspace
> sets TCP_CORK. Am I reading it correctly? Is that expected?

I have tested a new version of the patch which can target only TCP_NODELAY.
Results using previous benchmark are identical. I will submit it in a new 
patch version.

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -716,7 +716,8 @@

        tcp_mark_urg(tp, flags);

-       if (tcp_should_autocork(sk, skb, size_goal)) {
+       if (!(nonagle & TCP_NAGLE_OFF) &&
+           tcp_should_autocork(sk, skb, size_goal)) {

                /* avoid atomic op if TSQ_THROTTLED bit is already set */
                if (!test_bit(TSQ_THROTTLED, &sk->sk_tsq_flags)) {



> 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 ?

I am using ENA driver.

Eric can you please clarify where do you think the problem is?

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Eric Dumazet]

On Wed, Dec 13, 2023 at 10:30 PM Salvatore Dipietro <dipiets@amazon.com> wrote:
>
> > It looks like the above disables autocorking even after the userspace
> > sets TCP_CORK. Am I reading it correctly? Is that expected?
>
> I have tested a new version of the patch which can target only TCP_NODELAY.
> Results using previous benchmark are identical. I will submit it in a new
> patch version.

Well, I do not think we will accept a patch there, because you
basically are working around the root cause
for a certain variety of workloads.

Issue would still be there for applications not using TCP_NODELAY

>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -716,7 +716,8 @@
>
>         tcp_mark_urg(tp, flags);
>
> -       if (tcp_should_autocork(sk, skb, size_goal)) {
> +       if (!(nonagle & TCP_NAGLE_OFF) &&
> +           tcp_should_autocork(sk, skb, size_goal)) {
>
>                 /* avoid atomic op if TSQ_THROTTLED bit is already set */
>                 if (!test_bit(TSQ_THROTTLED, &sk->sk_tsq_flags)) {
>
>
>
> > 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 ?
>
> I am using ENA driver.
>
> Eric can you please clarify where do you think the problem is?

The problem is that TSQ logic is not working properly, probably
because the driver
holds a packet that has been sent.

TX completion seems to be delayed until the next transmit happens on
the transmit queue.

I suspect some kind of missed interrupt or a race.

virtio_net is known to have a similar issue (not sure if this has been
fixed lately)

ena_io_poll() and ena_intr_msix_io() logic, playing with
ena_napi->interrupts_masked seem
convoluted/risky to me.

ena_start_xmit() also seems to have bugs vs xmit_more logic, but this
is orthogonal.

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index c44c44e26ddfe74a93b7f1fb3c3ca90f978909e2..5282e718699ba9e64765bea2435e1c5a55aaa89b
100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -3235,6 +3235,8 @@ static netdev_tx_t ena_start_xmit(struct sk_buff
*skb, struct net_device *dev)

 error_drop_packet:
        dev_kfree_skb(skb);
+       /* Make sure to ring the doorbell. */
+       ena_ring_tx_doorbell(tx_ring);
        return NETDEV_TX_OK;
 }

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Eric Dumazet]

On Thu, Dec 14, 2023 at 9:40 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Dec 13, 2023 at 10:30 PM Salvatore Dipietro <dipiets@amazon.com> wrote:
> >
> > > It looks like the above disables autocorking even after the userspace
> > > sets TCP_CORK. Am I reading it correctly? Is that expected?
> >
> > I have tested a new version of the patch which can target only TCP_NODELAY.
> > Results using previous benchmark are identical. I will submit it in a new
> > patch version.
>
> Well, I do not think we will accept a patch there, because you
> basically are working around the root cause
> for a certain variety of workloads.
>
> Issue would still be there for applications not using TCP_NODELAY
>
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -716,7 +716,8 @@
> >
> >         tcp_mark_urg(tp, flags);
> >
> > -       if (tcp_should_autocork(sk, skb, size_goal)) {
> > +       if (!(nonagle & TCP_NAGLE_OFF) &&
> > +           tcp_should_autocork(sk, skb, size_goal)) {
> >
> >                 /* avoid atomic op if TSQ_THROTTLED bit is already set */
> >                 if (!test_bit(TSQ_THROTTLED, &sk->sk_tsq_flags)) {
> >
> >
> >
> > > 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 ?
> >
> > I am using ENA driver.
> >
> > Eric can you please clarify where do you think the problem is?
>
> The problem is that TSQ logic is not working properly, probably
> because the driver
> holds a packet that has been sent.
>
> TX completion seems to be delayed until the next transmit happens on
> the transmit queue.
>
> I suspect some kind of missed interrupt or a race.
>
> virtio_net is known to have a similar issue (not sure if this has been
> fixed lately)
>
> ena_io_poll() and ena_intr_msix_io() logic, playing with
> ena_napi->interrupts_masked seem
> convoluted/risky to me.
>
> ena_start_xmit() also seems to have bugs vs xmit_more logic, but this
> is orthogonal.
>
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index c44c44e26ddfe74a93b7f1fb3c3ca90f978909e2..5282e718699ba9e64765bea2435e1c5a55aaa89b
> 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -3235,6 +3235,8 @@ static netdev_tx_t ena_start_xmit(struct sk_buff
> *skb, struct net_device *dev)
>
>  error_drop_packet:
>         dev_kfree_skb(skb);
> +       /* Make sure to ring the doorbell. */
> +       ena_ring_tx_doorbell(tx_ring);
>         return NETDEV_TX_OK;
>  }

ena_io_poll() has a race against
u64_stats_update_begin(&tx_ring->syncp);/u64_stats_update_end(&tx_ring->syncp);

This should be done by this thread, while it still owns the NAPI SCHED bit.

Doing anything that might be racy after napi_complete_done() is a bug.
In this case, this could brick foreverer ena_get_stats64().

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index c44c44e26ddfe74a93b7f1fb3c3ca90f978909e2..e3464adfd0b791af621c92a651125ced2ad2de8a
100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -2017,7 +2017,6 @@ static int ena_io_poll(struct napi_struct *napi,
int budget)
        int tx_work_done;
        int rx_work_done = 0;
        int tx_budget;
-       int napi_comp_call = 0;
        int ret;

        tx_ring = ena_napi->tx_ring;
@@ -2038,6 +2037,11 @@ static int ena_io_poll(struct napi_struct
*napi, int budget)
        if (likely(budget))
                rx_work_done = ena_clean_rx_irq(rx_ring, napi, budget);

+       u64_stats_update_begin(&tx_ring->syncp);
+       tx_ring->tx_stats.tx_poll++;
+       u64_stats_update_end(&tx_ring->syncp);
+       WRITE_ONCE(tx_ring->tx_stats.last_napi_jiffies, jiffies);
+
        /* If the device is about to reset or down, avoid unmask
         * the interrupt and return 0 so NAPI won't reschedule
         */
@@ -2047,7 +2051,9 @@ static int ena_io_poll(struct napi_struct *napi,
int budget)
                ret = 0;

        } else if ((budget > rx_work_done) && (tx_budget > tx_work_done)) {
-               napi_comp_call = 1;
+               u64_stats_update_begin(&tx_ring->syncp);
+               tx_ring->tx_stats.napi_comp++;
+               u64_stats_update_end(&tx_ring->syncp);

                /* Update numa and unmask the interrupt only when schedule
                 * from the interrupt context (vs from sk_busy_loop)
@@ -2071,13 +2077,6 @@ static int ena_io_poll(struct napi_struct
*napi, int budget)
                ret = budget;
        }

-       u64_stats_update_begin(&tx_ring->syncp);
-       tx_ring->tx_stats.napi_comp += napi_comp_call;
-       tx_ring->tx_stats.tx_poll++;
-       u64_stats_update_end(&tx_ring->syncp);
-
-       tx_ring->tx_stats.last_napi_jiffies = jiffies;
-
        return ret;
 }

@@ -3235,6 +3234,8 @@ static netdev_tx_t ena_start_xmit(struct sk_buff
*skb, struct net_device *dev)

 error_drop_packet:
        dev_kfree_skb(skb);
+       /* Make sure to ring the doorbell. */
+       ena_ring_tx_doorbell(tx_ring);
        return NETDEV_TX_OK;
 }

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Eric Dumazet]

On Wed, Dec 13, 2023 at 10:30 PM Salvatore Dipietro <dipiets@amazon.com> wrote:
>
> > It looks like the above disables autocorking even after the userspace
> > sets TCP_CORK. Am I reading it correctly? Is that expected?
>
> I have tested a new version of the patch which can target only TCP_NODELAY.
> Results using previous benchmark are identical. I will submit it in a new
> patch version.
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -716,7 +716,8 @@
>
>         tcp_mark_urg(tp, flags);
>
> -       if (tcp_should_autocork(sk, skb, size_goal)) {
> +       if (!(nonagle & TCP_NAGLE_OFF) &&
> +           tcp_should_autocork(sk, skb, size_goal)) {
>
>                 /* avoid atomic op if TSQ_THROTTLED bit is already set */
>                 if (!test_bit(TSQ_THROTTLED, &sk->sk_tsq_flags)) {
>
>
>
> > 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 ?
>
> I am using ENA driver.
>
> Eric can you please clarify where do you think the problem is?
>

Following bpftrace program could double check if ena driver is
possibly holding TCP skbs too long:

bpftrace -e 'k:dev_hard_start_xmit {
 $skb = (struct sk_buff *)arg0;
 if ($skb->fclone == 2) {
  @start[$skb] = nsecs;
 }
}
k:__kfree_skb {
 $skb = (struct sk_buff *)arg0;
 if ($skb->fclone == 2 && @start[$skb]) {
  @tx_compl_usecs = hist((nsecs - @start[$skb])/1000);
  delete(@start[$skb]);
}
} END { clear(@start); }'

iroa21:/home/edumazet# ./trace-tx-completion.sh
Attaching 3 probes...
^C


@tx_compl_usecs:
[2, 4)                13 |                                                    |
[4, 8)               182 |                                                    |
[8, 16)          2379007 |@@@@@@@@@@@@@@@                                     |
[16, 32)         7865369 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
[32, 64)         6040939 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@             |
[64, 128)         199255 |@                                                   |
[128, 256)          9235 |                                                    |
[256, 512)            89 |                                                    |
[512, 1K)             37 |                                                    |
[1K, 2K)              19 |                                                    |
[2K, 4K)              56 |                                                    |

RE: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Geoff Blake]

[-- Attachment #1: Type: text/plain, Size: 3780 bytes --]

Thanks for helping dig in here Eric, but what is supposed to happen on TX 
completion? We're unfamiliar with TCP small queues beside finding your old 
LKML listing that states a tasklet is supposed to run if there is pending 
data.  So need a bit more guidance if you could.

I think its supposed to call tcp_free() when the skb is destructed and 
that invokes the tasklet?  There is also sock_wfree(), it does not appear 
to have the linkage to the tasklet by design.

We did attach probes at one point to look at whether there was a chance an 
interrupt went missing (but don't have them on-hand anymore), but we 
always saw the TX completion happen. When the 40ms latency happened 
we'd see that the completion had happened just after the other packet decided to 
be corked.  But it certainly doesn't hurt to double check.  

- Geoff Blake

On Thu, 14 Dec 2023, Eric Dumazet wrote:

> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Wed, Dec 13, 2023 at 10:30 PM Salvatore Dipietro <dipiets@amazon.com> wrote:
> >
> > > It looks like the above disables autocorking even after the userspace
> > > sets TCP_CORK. Am I reading it correctly? Is that expected?
> >
> > I have tested a new version of the patch which can target only TCP_NODELAY.
> > Results using previous benchmark are identical. I will submit it in a new
> > patch version.
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -716,7 +716,8 @@
> >
> >         tcp_mark_urg(tp, flags);
> >
> > -       if (tcp_should_autocork(sk, skb, size_goal)) {
> > +       if (!(nonagle & TCP_NAGLE_OFF) &&
> > +           tcp_should_autocork(sk, skb, size_goal)) {
> >
> >                 /* avoid atomic op if TSQ_THROTTLED bit is already set */
> >                 if (!test_bit(TSQ_THROTTLED, &sk->sk_tsq_flags)) {
> >
> >
> >
> > > 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 ?
> >
> > I am using ENA driver.
> >
> > Eric can you please clarify where do you think the problem is?
> >
> 
> Following bpftrace program could double check if ena driver is
> possibly holding TCP skbs too long:
> 
> bpftrace -e 'k:dev_hard_start_xmit {
>  $skb = (struct sk_buff *)arg0;
>  if ($skb->fclone == 2) {
>   @start[$skb] = nsecs;
>  }
> }
> k:__kfree_skb {
>  $skb = (struct sk_buff *)arg0;
>  if ($skb->fclone == 2 && @start[$skb]) {
>   @tx_compl_usecs = hist((nsecs - @start[$skb])/1000);
>   delete(@start[$skb]);
> }
> } END { clear(@start); }'
> 
> iroa21:/home/edumazet# ./trace-tx-completion.sh
> Attaching 3 probes...
> ^C
> 
> 
> @tx_compl_usecs:
> [2, 4)                13 |                                                    |
> [4, 8)               182 |                                                    |
> [8, 16)          2379007 |@@@@@@@@@@@@@@@                                     |
> [16, 32)         7865369 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> [32, 64)         6040939 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@             |
> [64, 128)         199255 |@                                                   |
> [128, 256)          9235 |                                                    |
> [256, 512)            89 |                                                    |
> [512, 1K)             37 |                                                    |
> [1K, 2K)              19 |                                                    |
> [2K, 4K)              56 |                                                    |
> 

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[Eric Dumazet]

On Thu, Dec 14, 2023 at 4:52 PM Geoff Blake <blakgeof@amazon.com> wrote:
>
> Thanks for helping dig in here Eric, but what is supposed to happen on TX
> completion? We're unfamiliar with TCP small queues beside finding your old
> LKML listing that states a tasklet is supposed to run if there is pending
> data.  So need a bit more guidance if you could.
>
> I think its supposed to call tcp_free() when the skb is destructed and
> that invokes the tasklet?  There is also sock_wfree(), it does not appear
> to have the linkage to the tasklet by design.
>
> We did attach probes at one point to look at whether there was a chance an
> interrupt went missing (but don't have them on-hand anymore), but we
> always saw the TX completion happen. When the 40ms latency happened
> we'd see that the completion had happened just after the other packet decided to
> be corked.  But it certainly doesn't hurt to double check.

When TX completion happens, while autocorking was setup, TSQ_THROTTLED
bit was set on sk->sk_tsq_flags, so TSQ logic should call
tcp_tsq_handler() -> tcp_tsq_write() -> tcp_write_xmit()

tcp_write_xmit() should send the pending packet (if CWND and other
constraints allows this)

autocorking is all about giving chance to the application to
add more bytes on the pending skb before TX completion happens
(typically in less  than 100 usec on an idle qdisc/nic)

If your life depends on not waiting for this delay, you have two options :

1) use MSG_EOR
2) disable autocorking (/proc/sys/net/ipv4/tcp_autocorking



>
> - Geoff Blake
>
> On Thu, 14 Dec 2023, Eric Dumazet wrote:
>
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >
> >
> >
> > On Wed, Dec 13, 2023 at 10:30 PM Salvatore Dipietro <dipiets@amazon.com> wrote:
> > >
> > > > It looks like the above disables autocorking even after the userspace
> > > > sets TCP_CORK. Am I reading it correctly? Is that expected?
> > >
> > > I have tested a new version of the patch which can target only TCP_NODELAY.
> > > Results using previous benchmark are identical. I will submit it in a new
> > > patch version.
> > >
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -716,7 +716,8 @@
> > >
> > >         tcp_mark_urg(tp, flags);
> > >
> > > -       if (tcp_should_autocork(sk, skb, size_goal)) {
> > > +       if (!(nonagle & TCP_NAGLE_OFF) &&
> > > +           tcp_should_autocork(sk, skb, size_goal)) {
> > >
> > >                 /* avoid atomic op if TSQ_THROTTLED bit is already set */
> > >                 if (!test_bit(TSQ_THROTTLED, &sk->sk_tsq_flags)) {
> > >
> > >
> > >
> > > > 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 ?
> > >
> > > I am using ENA driver.
> > >
> > > Eric can you please clarify where do you think the problem is?
> > >
> >
> > Following bpftrace program could double check if ena driver is
> > possibly holding TCP skbs too long:
> >
> > bpftrace -e 'k:dev_hard_start_xmit {
> >  $skb = (struct sk_buff *)arg0;
> >  if ($skb->fclone == 2) {
> >   @start[$skb] = nsecs;
> >  }
> > }
> > k:__kfree_skb {
> >  $skb = (struct sk_buff *)arg0;
> >  if ($skb->fclone == 2 && @start[$skb]) {
> >   @tx_compl_usecs = hist((nsecs - @start[$skb])/1000);
> >   delete(@start[$skb]);
> > }
> > } END { clear(@start); }'
> >
> > iroa21:/home/edumazet# ./trace-tx-completion.sh
> > Attaching 3 probes...
> > ^C
> >
> >
> > @tx_compl_usecs:
> > [2, 4)                13 |                                                    |
> > [4, 8)               182 |                                                    |
> > [8, 16)          2379007 |@@@@@@@@@@@@@@@                                     |
> > [16, 32)         7865369 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> > [32, 64)         6040939 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@             |
> > [64, 128)         199255 |@                                                   |
> > [128, 256)          9235 |                                                    |
> > [256, 512)            89 |                                                    |
> > [512, 1K)             37 |                                                    |
> > [1K, 2K)              19 |                                                    |
> > [2K, 4K)              56 |                                                    |
> >

[PATCH v2] tcp: Add memory barrier to tcp_push()

[Salvatore Dipietro]

On CPUs with weak memory models, reads and updates performed by tcp_push to the
sk variables can get reordered leaving the socket throttled when it should not.
The tasklet running tcp_wfree() may also not observe the memory updates in time
and will skip flushing any packets throttled by tcp_push(), delaying the sending.
This can pathologically cause 40ms extra latency due to bad interactions with
delayed acks.

Modeling the memory access behavior of tcp_push() (P0) and tcp_wfree() (P1)
using the herd7 simulator, proves this behavior can occur. Below is the litmus
model which describes the functions:
```
C MP+tcp
{
  [flag] = 0;
  [sk] = 5;
  [corked] = 0;
}

P0(int *flag, int *sk, int *corked){
    int r0;
    int r1;
    int r2;

    r1 = READ_ONCE(*sk);
    if (r1 == 5) {

        r0 = READ_ONCE(*flag);
        if (r0 == 0) {
            WRITE_ONCE(*flag, 1);
        }

        // memory barrier added in this patch,
        // original code does not order the reads/writes
        smp_mb();

        r2 = READ_ONCE(*sk);
        if (r2 == 5 ) {
            WRITE_ONCE(*corked,1);
        }
    }
}

P1(int *flag, int *sk, int *corked){
    int r0;
    int r1;

    r1 = READ_ONCE(*sk);
    smp_store_release(sk, 0);

    r0 = smp_load_acquire(flag);
    if (r0 == 1) {
        smp_store_release(flag, 0);
    }
}
locations [0:r0; 0:r1; 0:r2; 1:r0; 1:r1; flag; sk; corked; ]
exists ( flag=1 /\ corked=1 )
```

Adding the memory barrier removes the positive witness from the memory model.
smp_mb__after_atomic() is used to not incur in unnecessary overhead on x86
since not affected.
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. Before the patch an additional 40ms latency from P99.99+
values is observed while, with the patch, the extra latency disappears.

# No patch and tcp_autocorking=1
./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
  ...
 50.000%    0.91ms
 75.000%    1.13ms
 90.000%    1.46ms
 99.000%    1.74ms
 99.900%    1.89ms
 99.990%   41.95ms  <<< 40+ ms extra latency
 99.999%   48.32ms
100.000%   48.96ms

# With patch and tcp_autocorking=1
./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
  ...
 50.000%    0.90ms
 75.000%    1.13ms
 90.000%    1.45ms
 99.000%    1.72ms
 99.900%    1.83ms
 99.990%    2.11ms  <<< no 40+ ms extra latency
 99.999%    2.53ms
100.000%    2.62ms

Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
affected by this issue and the patch doesn't introduce any additional
delay.

Fixes: f54b311142a9 ("tcp: auto corking")
Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
---
 net/ipv4/tcp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index ff6838ca2e58..ab9e3922393c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
 		/* It is possible TX completion already happened
 		 * before we set TSQ_THROTTLED.
 		 */
+		smp_mb__after_atomic();
 		if (refcount_read(&sk->sk_wmem_alloc) > skb->truesize)
 			return;
 	}
-- 
2.42.0

Re: [PATCH v2] tcp: Add memory barrier to tcp_push()

[Eric Dumazet]

On Wed, Jan 17, 2024 at 10:29 PM Salvatore Dipietro <dipiets@amazon.com> wrote:
>
> On CPUs with weak memory models, reads and updates performed by tcp_push to the
> sk variables can get reordered leaving the socket throttled when it should not.
> The tasklet running tcp_wfree() may also not observe the memory updates in time
> and will skip flushing any packets throttled by tcp_push(), delaying the sending.
> This can pathologically cause 40ms extra latency due to bad interactions with
> delayed acks.
>
> Modeling the memory access behavior of tcp_push() (P0) and tcp_wfree() (P1)
> using the herd7 simulator, proves this behavior can occur. Below is the litmus
> model which describes the functions:
> ```
> C MP+tcp
> {
>   [flag] = 0;
>   [sk] = 5;
>   [corked] = 0;
> }
>
> P0(int *flag, int *sk, int *corked){
>     int r0;
>     int r1;
>     int r2;
>
>     r1 = READ_ONCE(*sk);
>     if (r1 == 5) {
>
>         r0 = READ_ONCE(*flag);
>         if (r0 == 0) {
>             WRITE_ONCE(*flag, 1);
>         }
>
>         // memory barrier added in this patch,
>         // original code does not order the reads/writes
>         smp_mb();
>
>         r2 = READ_ONCE(*sk);
>         if (r2 == 5 ) {
>             WRITE_ONCE(*corked,1);
>         }
>     }
> }
>

Interesting.

Quite frankly I doubt this herd7 stuff needs to be in a changelog,
this is too verbose/pedantic.

What about instead referring to similar commit bf06200e732d
("tcp: tsq: fix nonagle handling")

This was exactly the same issue, but at that time tcp_push() was not
re-reading sk->sk_wmem_alloc




> P1(int *flag, int *sk, int *corked){
>     int r0;
>     int r1;
>
>     r1 = READ_ONCE(*sk);
>     smp_store_release(sk, 0);
>
>     r0 = smp_load_acquire(flag);
>     if (r0 == 1) {
>         smp_store_release(flag, 0);
>     }
> }
> locations [0:r0; 0:r1; 0:r2; 1:r0; 1:r1; flag; sk; corked; ]
> exists ( flag=1 /\ corked=1 )
> ```
>
> Adding the memory barrier removes the positive witness from the memory model.
> smp_mb__after_atomic() is used to not incur in unnecessary overhead on x86
> since not affected.
> 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. Before the patch an additional 40ms latency from P99.99+
> values is observed while, with the patch, the extra latency disappears.
>
> # No patch and tcp_autocorking=1
> ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
>   ...
>  50.000%    0.91ms
>  75.000%    1.13ms
>  90.000%    1.46ms
>  99.000%    1.74ms
>  99.900%    1.89ms
>  99.990%   41.95ms  <<< 40+ ms extra latency
>  99.999%   48.32ms
> 100.000%   48.96ms
>
> # With patch and tcp_autocorking=1
> ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
>   ...
>  50.000%    0.90ms
>  75.000%    1.13ms
>  90.000%    1.45ms
>  99.000%    1.72ms
>  99.900%    1.83ms
>  99.990%    2.11ms  <<< no 40+ ms extra latency
>  99.999%    2.53ms
> 100.000%    2.62ms
>
> Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
> affected by this issue and the patch doesn't introduce any additional
> delay.
>
> Fixes: f54b311142a9 ("tcp: auto corking")

Bug came with a181ceb501b3 ("tcp: autocork should not hold first
packet in write queue")



> Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> ---
>  net/ipv4/tcp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index ff6838ca2e58..ab9e3922393c 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
>                 /* It is possible TX completion already happened
>                  * before we set TSQ_THROTTLED.
>                  */
> +               smp_mb__after_atomic();
>                 if (refcount_read(&sk->sk_wmem_alloc) > skb->truesize)
>                         return;
>         }
> --
> 2.42.0
>

[PATCH v3] tcp: Add memory barrier to tcp_push()

[Salvatore Dipietro]

On CPUs with weak memory models, reads and updates performed by tcp_push to the
sk variables can get reordered leaving the socket throttled when it should not.
The tasklet running tcp_wfree() may also not observe the memory updates in time
and will skip flushing any packets throttled by tcp_push(), delaying the sending.
This can pathologically cause 40ms extra latency due to bad interactions with
delayed acks.

Adding a memory barrier in tcp_push before the sk_wmem_alloc read removes the
bug, similarly to the previous commit bf06200e732d ("tcp: tsq: fix nonagle
handling"). smp_mb__after_atomic() is used to not incur in unnecessary overhead
on x86 since not affected.

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. Before the patch an additional 40ms latency from P99.99+
values is observed while, with the patch, the extra latency disappears.

# No patch and tcp_autocorking=1
./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
  ...
 50.000%    0.91ms
 75.000%    1.13ms
 90.000%    1.46ms
 99.000%    1.74ms
 99.900%    1.89ms
 99.990%   41.95ms  <<< 40+ ms extra latency
 99.999%   48.32ms
100.000%   48.96ms

# With patch and tcp_autocorking=1
./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
  ...
 50.000%    0.90ms
 75.000%    1.13ms
 90.000%    1.45ms
 99.000%    1.72ms
 99.900%    1.83ms
 99.990%    2.11ms  <<< no 40+ ms extra latency
 99.999%    2.53ms
100.000%    2.62ms

Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
affected by this issue and the patch doesn't introduce any additional
delay.

Fixes: a181ceb501b3 ("tcp: autocork should not hold first packet in write
queue")
Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
---
 net/ipv4/tcp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index ff6838ca2e58..ab9e3922393c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
 		/* It is possible TX completion already happened
 		 * before we set TSQ_THROTTLED.
 		 */
+		smp_mb__after_atomic();
 		if (refcount_read(&sk->sk_wmem_alloc) > skb->truesize)
 			return;
 	}
-- 
2.42.0

Re: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Paolo Abeni]

On Wed, 2024-01-17 at 15:16 -0800, Salvatore Dipietro wrote:
> On CPUs with weak memory models, reads and updates performed by tcp_push to the
> sk variables can get reordered leaving the socket throttled when it should not.
> The tasklet running tcp_wfree() may also not observe the memory updates in time
> and will skip flushing any packets throttled by tcp_push(), delaying the sending.
> This can pathologically cause 40ms extra latency due to bad interactions with
> delayed acks.
> 
> Adding a memory barrier in tcp_push before the sk_wmem_alloc read removes the
> bug, similarly to the previous commit bf06200e732d ("tcp: tsq: fix nonagle
> handling"). smp_mb__after_atomic() is used to not incur in unnecessary overhead
> on x86 since not affected.
> 
> 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. Before the patch an additional 40ms latency from P99.99+
> values is observed while, with the patch, the extra latency disappears.
> 
> # No patch and tcp_autocorking=1
> ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
>   ...
>  50.000%    0.91ms
>  75.000%    1.13ms
>  90.000%    1.46ms
>  99.000%    1.74ms
>  99.900%    1.89ms
>  99.990%   41.95ms  <<< 40+ ms extra latency
>  99.999%   48.32ms
> 100.000%   48.96ms
> 
> # With patch and tcp_autocorking=1
> ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
>   ...
>  50.000%    0.90ms
>  75.000%    1.13ms
>  90.000%    1.45ms
>  99.000%    1.72ms
>  99.900%    1.83ms
>  99.990%    2.11ms  <<< no 40+ ms extra latency
>  99.999%    2.53ms
> 100.000%    2.62ms
> 
> Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
> affected by this issue and the patch doesn't introduce any additional
> delay.
> 
> Fixes: a181ceb501b3 ("tcp: autocork should not hold first packet in write
> queue")

Please read carefully the process documentation under
Documentation/process/ and specifically the netdev specific bits:

no resubmissions within the 24h grace period.

Please double-check your patch with checkpatch for formal errors: the
fixes tag must not be split across multiple lines.

And please do not sent new version in reply to a previous one: it will
confuse the bot.

> Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> ---
>  net/ipv4/tcp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index ff6838ca2e58..ab9e3922393c 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
>  		/* It is possible TX completion already happened
>  		 * before we set TSQ_THROTTLED.
>  		 */
> +		smp_mb__after_atomic();

Out of sheer ignorance I'm wondering if moving such barrier inside the
above 'if' just after 'set_bit' would suffice?

Thanks!

Paolo

Re: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Eric Dumazet]

On Thu, Jan 18, 2024 at 11:38 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Wed, 2024-01-17 at 15:16 -0800, Salvatore Dipietro wrote:
> > On CPUs with weak memory models, reads and updates performed by tcp_push to the
> > sk variables can get reordered leaving the socket throttled when it should not.
> > The tasklet running tcp_wfree() may also not observe the memory updates in time
> > and will skip flushing any packets throttled by tcp_push(), delaying the sending.
> > This can pathologically cause 40ms extra latency due to bad interactions with
> > delayed acks.
> >
> > Adding a memory barrier in tcp_push before the sk_wmem_alloc read removes the
> > bug, similarly to the previous commit bf06200e732d ("tcp: tsq: fix nonagle
> > handling"). smp_mb__after_atomic() is used to not incur in unnecessary overhead
> > on x86 since not affected.
> >
> > 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. Before the patch an additional 40ms latency from P99.99+
> > values is observed while, with the patch, the extra latency disappears.
> >
> > # No patch and tcp_autocorking=1
> > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> >   ...
> >  50.000%    0.91ms
> >  75.000%    1.13ms
> >  90.000%    1.46ms
> >  99.000%    1.74ms
> >  99.900%    1.89ms
> >  99.990%   41.95ms  <<< 40+ ms extra latency
> >  99.999%   48.32ms
> > 100.000%   48.96ms
> >
> > # With patch and tcp_autocorking=1
> > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> >   ...
> >  50.000%    0.90ms
> >  75.000%    1.13ms
> >  90.000%    1.45ms
> >  99.000%    1.72ms
> >  99.900%    1.83ms
> >  99.990%    2.11ms  <<< no 40+ ms extra latency
> >  99.999%    2.53ms
> > 100.000%    2.62ms
> >
> > Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
> > affected by this issue and the patch doesn't introduce any additional
> > delay.
> >
> > Fixes: a181ceb501b3 ("tcp: autocork should not hold first packet in write
> > queue")
>
> Please read carefully the process documentation under
> Documentation/process/ and specifically the netdev specific bits:
>
> no resubmissions within the 24h grace period.
>
> Please double-check your patch with checkpatch for formal errors: the
> fixes tag must not be split across multiple lines.
>
> And please do not sent new version in reply to a previous one: it will
> confuse the bot.
>
> > Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> > ---
> >  net/ipv4/tcp.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index ff6838ca2e58..ab9e3922393c 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
> >               /* It is possible TX completion already happened
> >                * before we set TSQ_THROTTLED.
> >                */
> > +             smp_mb__after_atomic();
>
> Out of sheer ignorance I'm wondering if moving such barrier inside the
> above 'if' just after 'set_bit' would suffice?

I think this would work just fine.

>
> Thanks!
>
> Paolo
>

Re: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Jakub Kicinski]

On Thu, 18 Jan 2024 11:42:40 +0100 Eric Dumazet wrote:
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index ff6838ca2e58..ab9e3922393c 100644
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
> > >               /* It is possible TX completion already happened
> > >                * before we set TSQ_THROTTLED.
> > >                */
> > > +             smp_mb__after_atomic();  
> >
> > Out of sheer ignorance I'm wondering if moving such barrier inside the
> > above 'if' just after 'set_bit' would suffice?  
> 
> I think this would work just fine.

Sorry, "this" as in Paolo's suggestion or "this" as in the v3 patch 
as posted? :)

RE: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Geoff Blake]

On Thu, 18 Jan 2024, Paolo Abeni wrote:

> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Wed, 2024-01-17 at 15:16 -0800, Salvatore Dipietro wrote:
> > On CPUs with weak memory models, reads and updates performed by tcp_push to the
> > sk variables can get reordered leaving the socket throttled when it should not.
> > The tasklet running tcp_wfree() may also not observe the memory updates in time
> > and will skip flushing any packets throttled by tcp_push(), delaying the sending.
> > This can pathologically cause 40ms extra latency due to bad interactions with
> > delayed acks.
> >
> > Adding a memory barrier in tcp_push before the sk_wmem_alloc read removes the
> > bug, similarly to the previous commit bf06200e732d ("tcp: tsq: fix nonagle
> > handling"). smp_mb__after_atomic() is used to not incur in unnecessary overhead
> > on x86 since not affected.
> >
> > 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. Before the patch an additional 40ms latency from P99.99+
> > values is observed while, with the patch, the extra latency disappears.
> >
> > # No patch and tcp_autocorking=1
> > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> >   ...
> >  50.000%    0.91ms
> >  75.000%    1.13ms
> >  90.000%    1.46ms
> >  99.000%    1.74ms
> >  99.900%    1.89ms
> >  99.990%   41.95ms  <<< 40+ ms extra latency
> >  99.999%   48.32ms
> > 100.000%   48.96ms
> >
> > # With patch and tcp_autocorking=1
> > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> >   ...
> >  50.000%    0.90ms
> >  75.000%    1.13ms
> >  90.000%    1.45ms
> >  99.000%    1.72ms
> >  99.900%    1.83ms
> >  99.990%    2.11ms  <<< no 40+ ms extra latency
> >  99.999%    2.53ms
> > 100.000%    2.62ms
> >
> > Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
> > affected by this issue and the patch doesn't introduce any additional
> > delay.
> >
> > Fixes: a181ceb501b3 ("tcp: autocork should not hold first packet in write
> > queue")
> 
> Please read carefully the process documentation under
> Documentation/process/ and specifically the netdev specific bits:
> 
> no resubmissions within the 24h grace period.
> 
> Please double-check your patch with checkpatch for formal errors: the
> fixes tag must not be split across multiple lines.
> 
> And please do not sent new version in reply to a previous one: it will
> confuse the bot.
> 
> > Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> > ---
> >  net/ipv4/tcp.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index ff6838ca2e58..ab9e3922393c 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
> >               /* It is possible TX completion already happened
> >                * before we set TSQ_THROTTLED.
> >                */
> > +             smp_mb__after_atomic();
> 
> Out of sheer ignorance I'm wondering if moving such barrier inside the
> above 'if' just after 'set_bit' would suffice?

According to the herd7 modeling tool, the answer is no for weak memory 
models.  If we put the smp_mb inside the if, it allows the machine to 
reorder the two reads to sk_wmem_alloc and we can get to the bad state 
this patch is fixing.  Placing it outside the if ensures 
the ordering between those two reads as well as ordering the write to the 
flags variable.

Re: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Paolo Abeni]

On Thu, 2024-01-18 at 11:46 -0600, Geoff Blake wrote:
> 
> On Thu, 18 Jan 2024, Paolo Abeni wrote:
> 
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Wed, 2024-01-17 at 15:16 -0800, Salvatore Dipietro wrote:
> > > On CPUs with weak memory models, reads and updates performed by tcp_push to the
> > > sk variables can get reordered leaving the socket throttled when it should not.
> > > The tasklet running tcp_wfree() may also not observe the memory updates in time
> > > and will skip flushing any packets throttled by tcp_push(), delaying the sending.
> > > This can pathologically cause 40ms extra latency due to bad interactions with
> > > delayed acks.
> > > 
> > > Adding a memory barrier in tcp_push before the sk_wmem_alloc read removes the
> > > bug, similarly to the previous commit bf06200e732d ("tcp: tsq: fix nonagle
> > > handling"). smp_mb__after_atomic() is used to not incur in unnecessary overhead
> > > on x86 since not affected.
> > > 
> > > 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. Before the patch an additional 40ms latency from P99.99+
> > > values is observed while, with the patch, the extra latency disappears.
> > > 
> > > # No patch and tcp_autocorking=1
> > > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> > >   ...
> > >  50.000%    0.91ms
> > >  75.000%    1.13ms
> > >  90.000%    1.46ms
> > >  99.000%    1.74ms
> > >  99.900%    1.89ms
> > >  99.990%   41.95ms  <<< 40+ ms extra latency
> > >  99.999%   48.32ms
> > > 100.000%   48.96ms
> > > 
> > > # With patch and tcp_autocorking=1
> > > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> > >   ...
> > >  50.000%    0.90ms
> > >  75.000%    1.13ms
> > >  90.000%    1.45ms
> > >  99.000%    1.72ms
> > >  99.900%    1.83ms
> > >  99.990%    2.11ms  <<< no 40+ ms extra latency
> > >  99.999%    2.53ms
> > > 100.000%    2.62ms
> > > 
> > > Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
> > > affected by this issue and the patch doesn't introduce any additional
> > > delay.
> > > 
> > > Fixes: a181ceb501b3 ("tcp: autocork should not hold first packet in write
> > > queue")
> > 
> > Please read carefully the process documentation under
> > Documentation/process/ and specifically the netdev specific bits:
> > 
> > no resubmissions within the 24h grace period.
> > 
> > Please double-check your patch with checkpatch for formal errors: the
> > fixes tag must not be split across multiple lines.
> > 
> > And please do not sent new version in reply to a previous one: it will
> > confuse the bot.
> > 
> > > Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> > > ---
> > >  net/ipv4/tcp.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index ff6838ca2e58..ab9e3922393c 100644
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
> > >               /* It is possible TX completion already happened
> > >                * before we set TSQ_THROTTLED.
> > >                */
> > > +             smp_mb__after_atomic();
> > 
> > Out of sheer ignorance I'm wondering if moving such barrier inside the
> > above 'if' just after 'set_bit' would suffice?
> 
> According to the herd7 modeling tool, the answer is no for weak memory 
> models.  If we put the smp_mb inside the if, it allows the machine to 
> reorder the two reads to sk_wmem_alloc and we can get to the bad state 
> this patch is fixing.  Placing it outside the if ensures 
> the ordering between those two reads as well as ordering the write to the 
> flags variable.

For the records, I asked because reading the documentation  I assumed
that smp_mb__after_atomic() enforces the ordering WRT the previous RMW
operation (in this case 'set_bit'). Is this a wrong interpretation?

Now I wonder if the patch is enough. Specifically, would it work
correctly when the TSQ_THROTTLED bit is already set and there is no RMW
operation in between the two refcount_read()?

Thanks,

Paolo

RE: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Geoff Blake]

On Thu, 18 Jan 2024, Paolo Abeni wrote:

> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Thu, 2024-01-18 at 11:46 -0600, Geoff Blake wrote:
> >
> > On Thu, 18 Jan 2024, Paolo Abeni wrote:
> >
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > >
> > >
> > >
> > > On Wed, 2024-01-17 at 15:16 -0800, Salvatore Dipietro wrote:
> > > > On CPUs with weak memory models, reads and updates performed by tcp_push to the
> > > > sk variables can get reordered leaving the socket throttled when it should not.
> > > > The tasklet running tcp_wfree() may also not observe the memory updates in time
> > > > and will skip flushing any packets throttled by tcp_push(), delaying the sending.
> > > > This can pathologically cause 40ms extra latency due to bad interactions with
> > > > delayed acks.
> > > >
> > > > Adding a memory barrier in tcp_push before the sk_wmem_alloc read removes the
> > > > bug, similarly to the previous commit bf06200e732d ("tcp: tsq: fix nonagle
> > > > handling"). smp_mb__after_atomic() is used to not incur in unnecessary overhead
> > > > on x86 since not affected.
> > > >
> > > > 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. Before the patch an additional 40ms latency from P99.99+
> > > > values is observed while, with the patch, the extra latency disappears.
> > > >
> > > > # No patch and tcp_autocorking=1
> > > > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> > > >   ...
> > > >  50.000%    0.91ms
> > > >  75.000%    1.13ms
> > > >  90.000%    1.46ms
> > > >  99.000%    1.74ms
> > > >  99.900%    1.89ms
> > > >  99.990%   41.95ms  <<< 40+ ms extra latency
> > > >  99.999%   48.32ms
> > > > 100.000%   48.96ms
> > > >
> > > > # With patch and tcp_autocorking=1
> > > > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> > > >   ...
> > > >  50.000%    0.90ms
> > > >  75.000%    1.13ms
> > > >  90.000%    1.45ms
> > > >  99.000%    1.72ms
> > > >  99.900%    1.83ms
> > > >  99.990%    2.11ms  <<< no 40+ ms extra latency
> > > >  99.999%    2.53ms
> > > > 100.000%    2.62ms
> > > >
> > > > Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
> > > > affected by this issue and the patch doesn't introduce any additional
> > > > delay.
> > > >
> > > > Fixes: a181ceb501b3 ("tcp: autocork should not hold first packet in write
> > > > queue")
> > >
> > > Please read carefully the process documentation under
> > > Documentation/process/ and specifically the netdev specific bits:
> > >
> > > no resubmissions within the 24h grace period.
> > >
> > > Please double-check your patch with checkpatch for formal errors: the
> > > fixes tag must not be split across multiple lines.
> > >
> > > And please do not sent new version in reply to a previous one: it will
> > > confuse the bot.
> > >
> > > > Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> > > > ---
> > > >  net/ipv4/tcp.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > index ff6838ca2e58..ab9e3922393c 100644
> > > > --- a/net/ipv4/tcp.c
> > > > +++ b/net/ipv4/tcp.c
> > > > @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
> > > >               /* It is possible TX completion already happened
> > > >                * before we set TSQ_THROTTLED.
> > > >                */
> > > > +             smp_mb__after_atomic();
> > >
> > > Out of sheer ignorance I'm wondering if moving such barrier inside the
> > > above 'if' just after 'set_bit' would suffice?
> >
> > According to the herd7 modeling tool, the answer is no for weak memory
> > models.  If we put the smp_mb inside the if, it allows the machine to
> > reorder the two reads to sk_wmem_alloc and we can get to the bad state
> > this patch is fixing.  Placing it outside the if ensures
> > the ordering between those two reads as well as ordering the write to the
> > flags variable.
> 
> For the records, I asked because reading the documentation  I assumed
> that smp_mb__after_atomic() enforces the ordering WRT the previous RMW
> operation (in this case 'set_bit'). Is this a wrong interpretation?
> 
> Now I wonder if the patch is enough. Specifically, would it work
> correctly when the TSQ_THROTTLED bit is already set and there is no RMW
> operation in between the two refcount_read()?
> 

I fat fingered the model in the previous reply, sorry for the confusion.  
It appears we can put the barrier inside the if check and be fine.  For 
the x86 case reads won't pass other reads according to my understanding 
of their memory model which makes this work as is without the barrier to
begin with for when TSQ_THROTTLED is already 
set or not set.
  
For arm64, we need to generate a dmb.ish instruction after the store.  Can 
run a test to make sure the model is correct and can move the barrier 
to inside the if after the atomic bitset.

Thanks,
Geoff

Re: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Eric Dumazet]

On Thu, Jan 18, 2024 at 8:30 PM Geoff Blake <blakgeof@amazon.com> wrote:
>
>
>
> On Thu, 18 Jan 2024, Paolo Abeni wrote:
>
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >
> >
> >
> > On Thu, 2024-01-18 at 11:46 -0600, Geoff Blake wrote:
> > >
> > > On Thu, 18 Jan 2024, Paolo Abeni wrote:
> > >
> > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > >
> > > >
> > > >
> > > > On Wed, 2024-01-17 at 15:16 -0800, Salvatore Dipietro wrote:
> > > > > On CPUs with weak memory models, reads and updates performed by tcp_push to the
> > > > > sk variables can get reordered leaving the socket throttled when it should not.
> > > > > The tasklet running tcp_wfree() may also not observe the memory updates in time
> > > > > and will skip flushing any packets throttled by tcp_push(), delaying the sending.
> > > > > This can pathologically cause 40ms extra latency due to bad interactions with
> > > > > delayed acks.
> > > > >
> > > > > Adding a memory barrier in tcp_push before the sk_wmem_alloc read removes the
> > > > > bug, similarly to the previous commit bf06200e732d ("tcp: tsq: fix nonagle
> > > > > handling"). smp_mb__after_atomic() is used to not incur in unnecessary overhead
> > > > > on x86 since not affected.
> > > > >
> > > > > 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. Before the patch an additional 40ms latency from P99.99+
> > > > > values is observed while, with the patch, the extra latency disappears.
> > > > >
> > > > > # No patch and tcp_autocorking=1
> > > > > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> > > > >   ...
> > > > >  50.000%    0.91ms
> > > > >  75.000%    1.13ms
> > > > >  90.000%    1.46ms
> > > > >  99.000%    1.74ms
> > > > >  99.900%    1.89ms
> > > > >  99.990%   41.95ms  <<< 40+ ms extra latency
> > > > >  99.999%   48.32ms
> > > > > 100.000%   48.96ms
> > > > >
> > > > > # With patch and tcp_autocorking=1
> > > > > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> > > > >   ...
> > > > >  50.000%    0.90ms
> > > > >  75.000%    1.13ms
> > > > >  90.000%    1.45ms
> > > > >  99.000%    1.72ms
> > > > >  99.900%    1.83ms
> > > > >  99.990%    2.11ms  <<< no 40+ ms extra latency
> > > > >  99.999%    2.53ms
> > > > > 100.000%    2.62ms
> > > > >
> > > > > Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
> > > > > affected by this issue and the patch doesn't introduce any additional
> > > > > delay.
> > > > >
> > > > > Fixes: a181ceb501b3 ("tcp: autocork should not hold first packet in write
> > > > > queue")
> > > >
> > > > Please read carefully the process documentation under
> > > > Documentation/process/ and specifically the netdev specific bits:
> > > >
> > > > no resubmissions within the 24h grace period.
> > > >
> > > > Please double-check your patch with checkpatch for formal errors: the
> > > > fixes tag must not be split across multiple lines.
> > > >
> > > > And please do not sent new version in reply to a previous one: it will
> > > > confuse the bot.
> > > >
> > > > > Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> > > > > ---
> > > > >  net/ipv4/tcp.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > > index ff6838ca2e58..ab9e3922393c 100644
> > > > > --- a/net/ipv4/tcp.c
> > > > > +++ b/net/ipv4/tcp.c
> > > > > @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
> > > > >               /* It is possible TX completion already happened
> > > > >                * before we set TSQ_THROTTLED.
> > > > >                */
> > > > > +             smp_mb__after_atomic();
> > > >
> > > > Out of sheer ignorance I'm wondering if moving such barrier inside the
> > > > above 'if' just after 'set_bit' would suffice?
> > >
> > > According to the herd7 modeling tool, the answer is no for weak memory
> > > models.  If we put the smp_mb inside the if, it allows the machine to
> > > reorder the two reads to sk_wmem_alloc and we can get to the bad state
> > > this patch is fixing.  Placing it outside the if ensures
> > > the ordering between those two reads as well as ordering the write to the
> > > flags variable.
> >
> > For the records, I asked because reading the documentation  I assumed
> > that smp_mb__after_atomic() enforces the ordering WRT the previous RMW
> > operation (in this case 'set_bit'). Is this a wrong interpretation?
> >
> > Now I wonder if the patch is enough. Specifically, would it work
> > correctly when the TSQ_THROTTLED bit is already set and there is no RMW
> > operation in between the two refcount_read()?
> >
>
> I fat fingered the model in the previous reply, sorry for the confusion.
> It appears we can put the barrier inside the if check and be fine.  For
> the x86 case reads won't pass other reads according to my understanding
> of their memory model which makes this work as is without the barrier to
> begin with for when TSQ_THROTTLED is already
> set or not set.
>
> For arm64, we need to generate a dmb.ish instruction after the store.  Can
> run a test to make sure the model is correct and can move the barrier
> to inside the if after the atomic bitset.

Great, Salvatore can you send a v4, with the Fixes: tag properly formatted
and including Paolo suggestion ?

Thanks a lot.

Re: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Eric Dumazet]

On Thu, Jan 18, 2024 at 8:30 PM Geoff Blake <blakgeof@amazon.com> wrote:
>
>
>
> On Thu, 18 Jan 2024, Paolo Abeni wrote:
>
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> >
> >
> >
> > On Thu, 2024-01-18 at 11:46 -0600, Geoff Blake wrote:
> > >
> > > On Thu, 18 Jan 2024, Paolo Abeni wrote:
> > >
> > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > > >
> > > >
> > > >
> > > > On Wed, 2024-01-17 at 15:16 -0800, Salvatore Dipietro wrote:
> > > > > On CPUs with weak memory models, reads and updates performed by tcp_push to the
> > > > > sk variables can get reordered leaving the socket throttled when it should not.
> > > > > The tasklet running tcp_wfree() may also not observe the memory updates in time
> > > > > and will skip flushing any packets throttled by tcp_push(), delaying the sending.
> > > > > This can pathologically cause 40ms extra latency due to bad interactions with
> > > > > delayed acks.
> > > > >
> > > > > Adding a memory barrier in tcp_push before the sk_wmem_alloc read removes the
> > > > > bug, similarly to the previous commit bf06200e732d ("tcp: tsq: fix nonagle
> > > > > handling"). smp_mb__after_atomic() is used to not incur in unnecessary overhead
> > > > > on x86 since not affected.
> > > > >
> > > > > 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. Before the patch an additional 40ms latency from P99.99+
> > > > > values is observed while, with the patch, the extra latency disappears.
> > > > >
> > > > > # No patch and tcp_autocorking=1
> > > > > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> > > > >   ...
> > > > >  50.000%    0.91ms
> > > > >  75.000%    1.13ms
> > > > >  90.000%    1.46ms
> > > > >  99.000%    1.74ms
> > > > >  99.900%    1.89ms
> > > > >  99.990%   41.95ms  <<< 40+ ms extra latency
> > > > >  99.999%   48.32ms
> > > > > 100.000%   48.96ms
> > > > >
> > > > > # With patch and tcp_autocorking=1
> > > > > ./wrk -t32 -c128 -d40s --latency -R10000  http://172.31.60.173:8080/hello/hello
> > > > >   ...
> > > > >  50.000%    0.90ms
> > > > >  75.000%    1.13ms
> > > > >  90.000%    1.45ms
> > > > >  99.000%    1.72ms
> > > > >  99.900%    1.83ms
> > > > >  99.990%    2.11ms  <<< no 40+ ms extra latency
> > > > >  99.999%    2.53ms
> > > > > 100.000%    2.62ms
> > > > >
> > > > > Patch has been also tested on x86 (m7i.2xlarge instance) which it is not
> > > > > affected by this issue and the patch doesn't introduce any additional
> > > > > delay.
> > > > >
> > > > > Fixes: a181ceb501b3 ("tcp: autocork should not hold first packet in write
> > > > > queue")
> > > >
> > > > Please read carefully the process documentation under
> > > > Documentation/process/ and specifically the netdev specific bits:
> > > >
> > > > no resubmissions within the 24h grace period.
> > > >
> > > > Please double-check your patch with checkpatch for formal errors: the
> > > > fixes tag must not be split across multiple lines.
> > > >
> > > > And please do not sent new version in reply to a previous one: it will
> > > > confuse the bot.
> > > >
> > > > > Signed-off-by: Salvatore Dipietro <dipiets@amazon.com>
> > > > > ---
> > > > >  net/ipv4/tcp.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > > index ff6838ca2e58..ab9e3922393c 100644
> > > > > --- a/net/ipv4/tcp.c
> > > > > +++ b/net/ipv4/tcp.c
> > > > > @@ -726,6 +726,7 @@ void tcp_push(struct sock *sk, int flags, int mss_now,
> > > > >               /* It is possible TX completion already happened
> > > > >                * before we set TSQ_THROTTLED.
> > > > >                */
> > > > > +             smp_mb__after_atomic();
> > > >
> > > > Out of sheer ignorance I'm wondering if moving such barrier inside the
> > > > above 'if' just after 'set_bit' would suffice?
> > >
> > > According to the herd7 modeling tool, the answer is no for weak memory
> > > models.  If we put the smp_mb inside the if, it allows the machine to
> > > reorder the two reads to sk_wmem_alloc and we can get to the bad state
> > > this patch is fixing.  Placing it outside the if ensures
> > > the ordering between those two reads as well as ordering the write to the
> > > flags variable.
> >
> > For the records, I asked because reading the documentation  I assumed
> > that smp_mb__after_atomic() enforces the ordering WRT the previous RMW
> > operation (in this case 'set_bit'). Is this a wrong interpretation?
> >
> > Now I wonder if the patch is enough. Specifically, would it work
> > correctly when the TSQ_THROTTLED bit is already set and there is no RMW
> > operation in between the two refcount_read()?
> >
>
> I fat fingered the model in the previous reply, sorry for the confusion.
> It appears we can put the barrier inside the if check and be fine.  For
> the x86 case reads won't pass other reads according to my understanding
> of their memory model which makes this work as is without the barrier to
> begin with for when TSQ_THROTTLED is already
> set or not set.
>
> For arm64, we need to generate a dmb.ish instruction after the store.  Can
> run a test to make sure the model is correct and can move the barrier
> to inside the if after the atomic bitset.

I read again this code (in 'fixed' kernels) and could not convince
myself it is bug free/.

Without RMW with return value or barriers, this all boils to bare reads,
and all these reads could be ordered differently.

I am thinking of making the following change to be on the cautious side.

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index f23b97777ea5e93fc860d3f225e464f9376cfe09..2e768aab39128cc8aebc2dbd65406f3bccf449f7
100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -722,12 +722,9 @@ void tcp_push(struct sock *sk, int flags, int mss_now,

        if (tcp_should_autocork(sk, skb, size_goal)) {

-               /* avoid atomic op if TSQ_THROTTLED bit is already set */
-               if (!test_bit(TSQ_THROTTLED, &sk->sk_tsq_flags)) {
+               if (!test_and_set_bit(TSQ_THROTTLED, &sk->sk_tsq_flags))
                        NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAUTOCORKING);
-                       set_bit(TSQ_THROTTLED, &sk->sk_tsq_flags);
-                       smp_mb__after_atomic();
-               }
+
                /* It is possible TX completion already happened
                 * before we set TSQ_THROTTLED.
                 */

Re: [PATCH v3] tcp: Add memory barrier to tcp_push()

[Salvatore Dipietro]

We have tested the proposed solution and it doesn't introduce any regression in our benchmark. 
Moreover, looking to the assembly code, the `test_and_set_bit` generates an `ldsetal` and `dmb ish` instructions which are correct for the ARM architecture.
We do not see any concern with the proposed patch.

Re: [PATCH] tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 8 Dec 2023 10:20:49 -0800 you 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.
> 
> [...]

Here is the summary with links:
  - tcp: disable tcp_autocorking for socket when TCP_NODELAY flag is set
    https://git.kernel.org/netdev/net/c/f3f32a356c0d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html