[PATCH net-next 0/3] tcp: add fast path in timer handlers

[PATCH net-next 0/3] tcp: add fast path in timer handlers

[Eric Dumazet]

As mentioned in Netconf 2024:

TCP retransmit and delack timers are not stopped from
inet_csk_clear_xmit_timer() because we do not define
INET_CSK_CLEAR_TIMERS.

Enabling INET_CSK_CLEAR_TIMERS leads to lower performance,
mainly because del_timer() and mod_timer() happen from
different cpus quite often.

What we can do instead is to add fast paths to tcp_write_timer()
and tcp_delack_timer() to avoid socket spinlock acquisition.

Eric Dumazet (3):
  tcp: annotate data-races around icsk->icsk_pending
  tcp: add a fast path in tcp_write_timer()
  tcp: add a fast path in tcp_delack_timer()

 include/net/inet_connection_sock.h |  9 +++++----
 net/ipv4/inet_connection_sock.c    |  6 ++++--
 net/ipv4/inet_diag.c               | 10 ++++++----
 net/ipv4/tcp_ipv4.c                | 10 ++++++----
 net/ipv4/tcp_output.c              |  7 ++++---
 net/ipv4/tcp_timer.c               | 18 ++++++++++++++++--
 net/ipv6/tcp_ipv6.c                | 10 ++++++----
 net/mptcp/protocol.c               |  3 ++-
 8 files changed, 49 insertions(+), 24 deletions(-)

-- 
2.47.0.rc0.187.ge670bccf7e-goog

[PATCH net-next 1/3] tcp: annotate data-races around icsk->icsk_pending

[Eric Dumazet]

icsk->icsk_pending can be read locklessly already.

Following patch in the series will add another lockless read.

Add smp_load_acquire() and smp_store_release() annotations
because following patch will add a test in tcp_write_timer(),
and READ_ONCE()/WRITE_ONCE() alone would possibly lead to races.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/inet_connection_sock.h |  4 ++--
 net/ipv4/inet_connection_sock.c    |  6 ++++--
 net/ipv4/inet_diag.c               | 10 ++++++----
 net/ipv4/tcp_ipv4.c                | 10 ++++++----
 net/ipv4/tcp_output.c              |  4 ++--
 net/ipv4/tcp_timer.c               |  4 ++--
 net/ipv6/tcp_ipv6.c                | 10 ++++++----
 7 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index c0deaafebfdc0bc5b7f9e1b2a881f797a1a82ace..914d1977270449241f6fc6da2055f3af02a75f99 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -197,7 +197,7 @@ static inline void inet_csk_clear_xmit_timer(struct sock *sk, const int what)
 	struct inet_connection_sock *icsk = inet_csk(sk);
 
 	if (what == ICSK_TIME_RETRANS || what == ICSK_TIME_PROBE0) {
-		icsk->icsk_pending = 0;
+		smp_store_release(&icsk->icsk_pending, 0);
 #ifdef INET_CSK_CLEAR_TIMERS
 		sk_stop_timer(sk, &icsk->icsk_retransmit_timer);
 #endif
@@ -229,7 +229,7 @@ static inline void inet_csk_reset_xmit_timer(struct sock *sk, const int what,
 
 	if (what == ICSK_TIME_RETRANS || what == ICSK_TIME_PROBE0 ||
 	    what == ICSK_TIME_LOSS_PROBE || what == ICSK_TIME_REO_TIMEOUT) {
-		icsk->icsk_pending = what;
+		smp_store_release(&icsk->icsk_pending, what);
 		icsk->icsk_timeout = jiffies + when;
 		sk_reset_timer(sk, &icsk->icsk_retransmit_timer, icsk->icsk_timeout);
 	} else if (what == ICSK_TIME_DACK) {
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 2c5632d4fddbe8ad96f6c35b9ed770d09126eb5d..8c53385cc808c61097898514fd91a322e3a08d31 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -775,7 +775,8 @@ void inet_csk_clear_xmit_timers(struct sock *sk)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
 
-	icsk->icsk_pending = icsk->icsk_ack.pending = 0;
+	smp_store_release(&icsk->icsk_pending, 0);
+	icsk->icsk_ack.pending = 0;
 
 	sk_stop_timer(sk, &icsk->icsk_retransmit_timer);
 	sk_stop_timer(sk, &icsk->icsk_delack_timer);
@@ -790,7 +791,8 @@ void inet_csk_clear_xmit_timers_sync(struct sock *sk)
 	/* ongoing timer handlers need to acquire socket lock. */
 	sock_not_owned_by_me(sk);
 
-	icsk->icsk_pending = icsk->icsk_ack.pending = 0;
+	smp_store_release(&icsk->icsk_pending, 0);
+	icsk->icsk_ack.pending = 0;
 
 	sk_stop_timer_sync(sk, &icsk->icsk_retransmit_timer);
 	sk_stop_timer_sync(sk, &icsk->icsk_delack_timer);
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 67639309163d05c034fad80fc9a6096c3b79d42f..321acc8abf17e8c7d6a4e3326615123fff19deab 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -247,6 +247,7 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
 	struct nlmsghdr  *nlh;
 	struct nlattr *attr;
 	void *info = NULL;
+	u8 icsk_pending;
 	int protocol;
 
 	cb_data = cb->data;
@@ -307,14 +308,15 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
 		goto out;
 	}
 
-	if (icsk->icsk_pending == ICSK_TIME_RETRANS ||
-	    icsk->icsk_pending == ICSK_TIME_REO_TIMEOUT ||
-	    icsk->icsk_pending == ICSK_TIME_LOSS_PROBE) {
+	icsk_pending = smp_load_acquire(&icsk->icsk_pending);
+	if (icsk_pending == ICSK_TIME_RETRANS ||
+	    icsk_pending == ICSK_TIME_REO_TIMEOUT ||
+	    icsk_pending == ICSK_TIME_LOSS_PROBE) {
 		r->idiag_timer = 1;
 		r->idiag_retrans = icsk->icsk_retransmits;
 		r->idiag_expires =
 			jiffies_delta_to_msecs(icsk->icsk_timeout - jiffies);
-	} else if (icsk->icsk_pending == ICSK_TIME_PROBE0) {
+	} else if (icsk_pending == ICSK_TIME_PROBE0) {
 		r->idiag_timer = 4;
 		r->idiag_retrans = icsk->icsk_probes_out;
 		r->idiag_expires =
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 5afe5e57c89b5c28dfada2bf2e01fa7b3afa61f0..985028434f644c399e51d12ba8d9c2c5740dc6e1 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2900,15 +2900,17 @@ static void get_tcp4_sock(struct sock *sk, struct seq_file *f, int i)
 	__be32 src = inet->inet_rcv_saddr;
 	__u16 destp = ntohs(inet->inet_dport);
 	__u16 srcp = ntohs(inet->inet_sport);
+	u8 icsk_pending;
 	int rx_queue;
 	int state;
 
-	if (icsk->icsk_pending == ICSK_TIME_RETRANS ||
-	    icsk->icsk_pending == ICSK_TIME_REO_TIMEOUT ||
-	    icsk->icsk_pending == ICSK_TIME_LOSS_PROBE) {
+	icsk_pending = smp_load_acquire(&icsk->icsk_pending);
+	if (icsk_pending == ICSK_TIME_RETRANS ||
+	    icsk_pending == ICSK_TIME_REO_TIMEOUT ||
+	    icsk_pending == ICSK_TIME_LOSS_PROBE) {
 		timer_active	= 1;
 		timer_expires	= icsk->icsk_timeout;
-	} else if (icsk->icsk_pending == ICSK_TIME_PROBE0) {
+	} else if (icsk_pending == ICSK_TIME_PROBE0) {
 		timer_active	= 4;
 		timer_expires	= icsk->icsk_timeout;
 	} else if (timer_pending(&sk->sk_timer)) {
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 4fd746bd4d54f621601b20c3821e71370a4a615a..4d04073016035dcf62ba5e0ad23aac86e54e65c7 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2960,7 +2960,7 @@ void tcp_send_loss_probe(struct sock *sk)
 		WARN_ONCE(tp->packets_out,
 			  "invalid inflight: %u state %u cwnd %u mss %d\n",
 			  tp->packets_out, sk->sk_state, tcp_snd_cwnd(tp), mss);
-		inet_csk(sk)->icsk_pending = 0;
+		smp_store_release(&inet_csk(sk)->icsk_pending, 0);
 		return;
 	}
 
@@ -2993,7 +2993,7 @@ void tcp_send_loss_probe(struct sock *sk)
 
 	NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPLOSSPROBES);
 	/* Reset s.t. tcp_rearm_rto will restart timer from now */
-	inet_csk(sk)->icsk_pending = 0;
+	smp_store_release(&inet_csk(sk)->icsk_pending, 0);
 rearm_timer:
 	tcp_rearm_rto(sk);
 }
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 79064580c8c0d55daa2ab4dc6d27a5ab8802e599..56c597e763ac7a8cebeba324f84e57b1eeeae977 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -701,11 +701,11 @@ void tcp_write_timer_handler(struct sock *sk)
 		tcp_send_loss_probe(sk);
 		break;
 	case ICSK_TIME_RETRANS:
-		icsk->icsk_pending = 0;
+		smp_store_release(&icsk->icsk_pending, 0);
 		tcp_retransmit_timer(sk);
 		break;
 	case ICSK_TIME_PROBE0:
-		icsk->icsk_pending = 0;
+		smp_store_release(&icsk->icsk_pending, 0);
 		tcp_probe_timer(sk);
 		break;
 	}
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index d71ab4e1efe1c6598cf3d3e4334adf0881064ce9..7634c0be6acbdb67bb378cc81bdbf184552d2afc 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -2177,6 +2177,7 @@ static void get_tcp6_sock(struct seq_file *seq, struct sock *sp, int i)
 	const struct tcp_sock *tp = tcp_sk(sp);
 	const struct inet_connection_sock *icsk = inet_csk(sp);
 	const struct fastopen_queue *fastopenq = &icsk->icsk_accept_queue.fastopenq;
+	u8 icsk_pending;
 	int rx_queue;
 	int state;
 
@@ -2185,12 +2186,13 @@ static void get_tcp6_sock(struct seq_file *seq, struct sock *sp, int i)
 	destp = ntohs(inet->inet_dport);
 	srcp  = ntohs(inet->inet_sport);
 
-	if (icsk->icsk_pending == ICSK_TIME_RETRANS ||
-	    icsk->icsk_pending == ICSK_TIME_REO_TIMEOUT ||
-	    icsk->icsk_pending == ICSK_TIME_LOSS_PROBE) {
+	icsk_pending = smp_load_acquire(&icsk->icsk_pending);
+	if (icsk_pending == ICSK_TIME_RETRANS ||
+	    icsk_pending == ICSK_TIME_REO_TIMEOUT ||
+	    icsk_pending == ICSK_TIME_LOSS_PROBE) {
 		timer_active	= 1;
 		timer_expires	= icsk->icsk_timeout;
-	} else if (icsk->icsk_pending == ICSK_TIME_PROBE0) {
+	} else if (icsk_pending == ICSK_TIME_PROBE0) {
 		timer_active	= 4;
 		timer_expires	= icsk->icsk_timeout;
 	} else if (timer_pending(&sp->sk_timer)) {
-- 
2.47.0.rc0.187.ge670bccf7e-goog

[PATCH net-next 2/3] tcp: add a fast path in tcp_write_timer()

[Eric Dumazet]

retransmit timer is not stopped from inet_csk_clear_xmit_timer()
because we do not define INET_CSK_CLEAR_TIMERS.

This is a conscious choice : for active TCP flows, it is better
to only call mod_timer(), because there is more chances of
keeping the timer unchanged. Also inet_csk_clear_xmit_timer()
is often called from another cpu, and calling del_timer()
would cause false sharing and lock contention.

This means that very often, tcp_write_timer() is called
at the timer expiration, while there is nothing to retransmit.

This can be detected very early, avoiding the socket spinlock.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_timer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 56c597e763ac7a8cebeba324f84e57b1eeeae977..b7266b9101ce5933776bd38d086287667e3a7f18 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -717,6 +717,10 @@ static void tcp_write_timer(struct timer_list *t)
 			from_timer(icsk, t, icsk_retransmit_timer);
 	struct sock *sk = &icsk->icsk_inet.sk;
 
+	/* Avoid locking the socket when there is no pending event. */
+	if (!smp_load_acquire(&icsk->icsk_pending))
+		goto out;
+
 	bh_lock_sock(sk);
 	if (!sock_owned_by_user(sk)) {
 		tcp_write_timer_handler(sk);
@@ -726,6 +730,7 @@ static void tcp_write_timer(struct timer_list *t)
 			sock_hold(sk);
 	}
 	bh_unlock_sock(sk);
+out:
 	sock_put(sk);
 }
 
-- 
2.47.0.rc0.187.ge670bccf7e-goog

[PATCH net-next 3/3] tcp: add a fast path in tcp_delack_timer()

[Eric Dumazet]

delack timer is not stopped from inet_csk_clear_xmit_timer()
because we do not define INET_CSK_CLEAR_TIMERS.

This is a conscious choice : inet_csk_clear_xmit_timer()
is often called from another cpu. Calling del_timer()
would cause false sharing and lock contention.

This means that very often, tcp_delack_timer() is called
at the timer expiration, while there is no ACK to transmit.

This can be detected very early, avoiding the socket spinlock.

Notes:
- test about tp->compressed_ack is racy,
  but in the unlikely case there is a race, the dedicated
  compressed_ack_timer hrtimer would close it.

- Even if the fast path is not taken, reading
  icsk->icsk_ack.pending and tp->compressed_ack
  before acquiring the socket spinlock reduces
  acquisition time and chances of contention.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/inet_connection_sock.h | 5 +++--
 net/ipv4/inet_connection_sock.c    | 4 ++--
 net/ipv4/tcp_output.c              | 3 ++-
 net/ipv4/tcp_timer.c               | 9 +++++++++
 net/mptcp/protocol.c               | 3 ++-
 5 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 914d1977270449241f6fc6da2055f3af02a75f99..3c82fad904d4c6c51069e2e703673d667bb36d06 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -202,7 +202,7 @@ static inline void inet_csk_clear_xmit_timer(struct sock *sk, const int what)
 		sk_stop_timer(sk, &icsk->icsk_retransmit_timer);
 #endif
 	} else if (what == ICSK_TIME_DACK) {
-		icsk->icsk_ack.pending = 0;
+		smp_store_release(&icsk->icsk_ack.pending, 0);
 		icsk->icsk_ack.retry = 0;
 #ifdef INET_CSK_CLEAR_TIMERS
 		sk_stop_timer(sk, &icsk->icsk_delack_timer);
@@ -233,7 +233,8 @@ static inline void inet_csk_reset_xmit_timer(struct sock *sk, const int what,
 		icsk->icsk_timeout = jiffies + when;
 		sk_reset_timer(sk, &icsk->icsk_retransmit_timer, icsk->icsk_timeout);
 	} else if (what == ICSK_TIME_DACK) {
-		icsk->icsk_ack.pending |= ICSK_ACK_TIMER;
+		smp_store_release(&icsk->icsk_ack.pending,
+				  icsk->icsk_ack.pending | ICSK_ACK_TIMER);
 		icsk->icsk_ack.timeout = jiffies + when;
 		sk_reset_timer(sk, &icsk->icsk_delack_timer, icsk->icsk_ack.timeout);
 	} else {
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 8c53385cc808c61097898514fd91a322e3a08d31..12e975ed4910d8c7cca79b1812f365589a5d469a 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -776,7 +776,7 @@ void inet_csk_clear_xmit_timers(struct sock *sk)
 	struct inet_connection_sock *icsk = inet_csk(sk);
 
 	smp_store_release(&icsk->icsk_pending, 0);
-	icsk->icsk_ack.pending = 0;
+	smp_store_release(&icsk->icsk_ack.pending, 0);
 
 	sk_stop_timer(sk, &icsk->icsk_retransmit_timer);
 	sk_stop_timer(sk, &icsk->icsk_delack_timer);
@@ -792,7 +792,7 @@ void inet_csk_clear_xmit_timers_sync(struct sock *sk)
 	sock_not_owned_by_me(sk);
 
 	smp_store_release(&icsk->icsk_pending, 0);
-	icsk->icsk_ack.pending = 0;
+	smp_store_release(&icsk->icsk_ack.pending, 0);
 
 	sk_stop_timer_sync(sk, &icsk->icsk_retransmit_timer);
 	sk_stop_timer_sync(sk, &icsk->icsk_delack_timer);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 4d04073016035dcf62ba5e0ad23aac86e54e65c7..08772395690d13a0c3309a273543a51aa0dd3fdc 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -4224,7 +4224,8 @@ void tcp_send_delayed_ack(struct sock *sk)
 		if (!time_before(timeout, icsk->icsk_ack.timeout))
 			timeout = icsk->icsk_ack.timeout;
 	}
-	icsk->icsk_ack.pending |= ICSK_ACK_SCHED | ICSK_ACK_TIMER;
+	smp_store_release(&icsk->icsk_ack.pending,
+			  icsk->icsk_ack.pending | ICSK_ACK_SCHED | ICSK_ACK_TIMER);
 	icsk->icsk_ack.timeout = timeout;
 	sk_reset_timer(sk, &icsk->icsk_delack_timer, timeout);
 }
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index b7266b9101ce5933776bd38d086287667e3a7f18..c3a7442332d4926a6089812f789e89ee23081306 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -361,6 +361,14 @@ static void tcp_delack_timer(struct timer_list *t)
 			from_timer(icsk, t, icsk_delack_timer);
 	struct sock *sk = &icsk->icsk_inet.sk;
 
+	/* Avoid taking socket spinlock if there is no ACK to send.
+	 * The compressed_ack check is racy, but a separate hrtimer
+	 * will take care of it eventually.
+	 */
+	if (!(smp_load_acquire(&icsk->icsk_ack.pending) & ICSK_ACK_TIMER) &&
+	    !READ_ONCE(tcp_sk(sk)->compressed_ack))
+		goto out;
+
 	bh_lock_sock(sk);
 	if (!sock_owned_by_user(sk)) {
 		tcp_delack_timer_handler(sk);
@@ -371,6 +379,7 @@ static void tcp_delack_timer(struct timer_list *t)
 			sock_hold(sk);
 	}
 	bh_unlock_sock(sk);
+out:
 	sock_put(sk);
 }
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c2317919fc148a67a81ded795359bd613c9b0dff..e85862352084907582ec884dcb96832356419fa5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3504,7 +3504,8 @@ static void schedule_3rdack_retransmission(struct sock *ssk)
 	timeout += jiffies;
 
 	WARN_ON_ONCE(icsk->icsk_ack.pending & ICSK_ACK_TIMER);
-	icsk->icsk_ack.pending |= ICSK_ACK_SCHED | ICSK_ACK_TIMER;
+	smp_store_release(&icsk->icsk_ack.pending,
+			  icsk->icsk_ack.pending | ICSK_ACK_SCHED | ICSK_ACK_TIMER);
 	icsk->icsk_ack.timeout = timeout;
 	sk_reset_timer(ssk, &icsk->icsk_delack_timer, timeout);
 }
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH net-next 3/3] tcp: add a fast path in tcp_delack_timer()

[Jason Xing]

Hello Eric,

On Thu, Oct 3, 2024 at 2:31 AM Eric Dumazet <edumazet@google.com> wrote:
>
> delack timer is not stopped from inet_csk_clear_xmit_timer()
> because we do not define INET_CSK_CLEAR_TIMERS.
>
> This is a conscious choice : inet_csk_clear_xmit_timer()
> is often called from another cpu. Calling del_timer()
> would cause false sharing and lock contention.
>
> This means that very often, tcp_delack_timer() is called
> at the timer expiration, while there is no ACK to transmit.
>
> This can be detected very early, avoiding the socket spinlock.
>
> Notes:
> - test about tp->compressed_ack is racy,
>   but in the unlikely case there is a race, the dedicated
>   compressed_ack_timer hrtimer would close it.
>
> - Even if the fast path is not taken, reading
>   icsk->icsk_ack.pending and tp->compressed_ack
>   before acquiring the socket spinlock reduces
>   acquisition time and chances of contention.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/net/inet_connection_sock.h | 5 +++--
>  net/ipv4/inet_connection_sock.c    | 4 ++--
>  net/ipv4/tcp_output.c              | 3 ++-
>  net/ipv4/tcp_timer.c               | 9 +++++++++
>  net/mptcp/protocol.c               | 3 ++-
>  5 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
> index 914d1977270449241f6fc6da2055f3af02a75f99..3c82fad904d4c6c51069e2e703673d667bb36d06 100644
> --- a/include/net/inet_connection_sock.h
> +++ b/include/net/inet_connection_sock.h
> @@ -202,7 +202,7 @@ static inline void inet_csk_clear_xmit_timer(struct sock *sk, const int what)
>                 sk_stop_timer(sk, &icsk->icsk_retransmit_timer);
>  #endif
>         } else if (what == ICSK_TIME_DACK) {
> -               icsk->icsk_ack.pending = 0;
> +               smp_store_release(&icsk->icsk_ack.pending, 0);
>                 icsk->icsk_ack.retry = 0;
>  #ifdef INET_CSK_CLEAR_TIMERS
>                 sk_stop_timer(sk, &icsk->icsk_delack_timer);
> @@ -233,7 +233,8 @@ static inline void inet_csk_reset_xmit_timer(struct sock *sk, const int what,
>                 icsk->icsk_timeout = jiffies + when;
>                 sk_reset_timer(sk, &icsk->icsk_retransmit_timer, icsk->icsk_timeout);
>         } else if (what == ICSK_TIME_DACK) {
> -               icsk->icsk_ack.pending |= ICSK_ACK_TIMER;
> +               smp_store_release(&icsk->icsk_ack.pending,
> +                                 icsk->icsk_ack.pending | ICSK_ACK_TIMER);
>                 icsk->icsk_ack.timeout = jiffies + when;
>                 sk_reset_timer(sk, &icsk->icsk_delack_timer, icsk->icsk_ack.timeout);
>         } else {
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 8c53385cc808c61097898514fd91a322e3a08d31..12e975ed4910d8c7cca79b1812f365589a5d469a 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -776,7 +776,7 @@ void inet_csk_clear_xmit_timers(struct sock *sk)
>         struct inet_connection_sock *icsk = inet_csk(sk);
>
>         smp_store_release(&icsk->icsk_pending, 0);
> -       icsk->icsk_ack.pending = 0;
> +       smp_store_release(&icsk->icsk_ack.pending, 0);
>
>         sk_stop_timer(sk, &icsk->icsk_retransmit_timer);
>         sk_stop_timer(sk, &icsk->icsk_delack_timer);
> @@ -792,7 +792,7 @@ void inet_csk_clear_xmit_timers_sync(struct sock *sk)
>         sock_not_owned_by_me(sk);
>
>         smp_store_release(&icsk->icsk_pending, 0);
> -       icsk->icsk_ack.pending = 0;
> +       smp_store_release(&icsk->icsk_ack.pending, 0);
>
>         sk_stop_timer_sync(sk, &icsk->icsk_retransmit_timer);
>         sk_stop_timer_sync(sk, &icsk->icsk_delack_timer);
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 4d04073016035dcf62ba5e0ad23aac86e54e65c7..08772395690d13a0c3309a273543a51aa0dd3fdc 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -4224,7 +4224,8 @@ void tcp_send_delayed_ack(struct sock *sk)
>                 if (!time_before(timeout, icsk->icsk_ack.timeout))
>                         timeout = icsk->icsk_ack.timeout;
>         }
> -       icsk->icsk_ack.pending |= ICSK_ACK_SCHED | ICSK_ACK_TIMER;
> +       smp_store_release(&icsk->icsk_ack.pending,
> +                         icsk->icsk_ack.pending | ICSK_ACK_SCHED | ICSK_ACK_TIMER);
>         icsk->icsk_ack.timeout = timeout;
>         sk_reset_timer(sk, &icsk->icsk_delack_timer, timeout);
>  }
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index b7266b9101ce5933776bd38d086287667e3a7f18..c3a7442332d4926a6089812f789e89ee23081306 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -361,6 +361,14 @@ static void tcp_delack_timer(struct timer_list *t)
>                         from_timer(icsk, t, icsk_delack_timer);
>         struct sock *sk = &icsk->icsk_inet.sk;
>
> +       /* Avoid taking socket spinlock if there is no ACK to send.
> +        * The compressed_ack check is racy, but a separate hrtimer
> +        * will take care of it eventually.
> +        */
> +       if (!(smp_load_acquire(&icsk->icsk_ack.pending) & ICSK_ACK_TIMER) &&
> +           !READ_ONCE(tcp_sk(sk)->compressed_ack))

I wonder what the use of single READ_ONCE() is here without a
WRITE_ONCE() pair? It cannot guarantee that the result of reading
compressed_ack is accurate. What if we use without this READ_ONCE()
here?

Thanks,
Jason

Re: [PATCH net-next 3/3] tcp: add a fast path in tcp_delack_timer()

[Eric Dumazet]

On Thu, Oct 3, 2024 at 1:19 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> Hello Eric,
>
        !READ_ONCE(tcp_sk(sk)->compressed_ack))
>
> I wonder what the use of single READ_ONCE() is here without a
> WRITE_ONCE() pair? It cannot guarantee that the result of reading
> compressed_ack is accurate. What if we use without this READ_ONCE()
> here?

Have you read the changelog and comments about this 'accuracy' thing ?

If you do not use the READ_ONCE() here, only concern is KCSAN might
trigger a splat.

The WRITE_ONCE() for a single byte is not needed, no tearing is possible.

Re: [PATCH net-next 3/3] tcp: add a fast path in tcp_delack_timer()

[Jason Xing]

On Thu, Oct 3, 2024 at 6:11 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Oct 3, 2024 at 1:19 AM Jason Xing <kerneljasonxing@gmail.com> wrote:
> >
> > Hello Eric,
> >
>         !READ_ONCE(tcp_sk(sk)->compressed_ack))
> >
> > I wonder what the use of single READ_ONCE() is here without a
> > WRITE_ONCE() pair? It cannot guarantee that the result of reading
> > compressed_ack is accurate. What if we use without this READ_ONCE()
> > here?
>
> Have you read the changelog and comments about this 'accuracy' thing ?

My initial question was about how only READ_ONCE works without adding
corresponding WRITE_ONCE here. Sorry that I didn't totally understand
it.

Sure, I did read them. But I did miss to understand one line "before
acquiring the socket spinlock reduces acquisition time and chances of
contention", which is perhaps the reason.

>
> If you do not use the READ_ONCE() here, only concern is KCSAN might
> trigger a splat.
>
> The WRITE_ONCE() for a single byte is not needed, no tearing is possible.

Yes, I got it. The writers of this field before this patch are all
protected under the socket lock.

Thanks for your explanation.

Thanks,
Jason

Re: [PATCH net-next 0/3] tcp: add fast path in timer handlers

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  2 Oct 2024 17:30:39 +0000 you wrote:
> As mentioned in Netconf 2024:
> 
> TCP retransmit and delack timers are not stopped from
> inet_csk_clear_xmit_timer() because we do not define
> INET_CSK_CLEAR_TIMERS.
> 
> Enabling INET_CSK_CLEAR_TIMERS leads to lower performance,
> mainly because del_timer() and mod_timer() happen from
> different cpus quite often.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] tcp: annotate data-races around icsk->icsk_pending
    https://git.kernel.org/netdev/net-next/c/5a9071a760a6
  - [net-next,2/3] tcp: add a fast path in tcp_write_timer()
    https://git.kernel.org/netdev/net-next/c/3b7842930162
  - [net-next,3/3] tcp: add a fast path in tcp_delack_timer()
    https://git.kernel.org/netdev/net-next/c/81df4fa94ee8

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