[PATCH net-next] net-timestamp: support TCP GSO case for a few missing flags

[PATCH net-next] net-timestamp: support TCP GSO case for a few missing flags

[Jason Xing]

When I read through the TSO codes, I found out that we probably
miss initializing the tx_flags of last seg when TSO is turned
off, which means at the following points no more timestamp
(for this last one) will be generated. There are three flags
to be handled in this patch:
1. SKBTX_HW_TSTAMP
2. SKBTX_HW_TSTAMP_USE_CYCLES
3. SKBTX_BPF

This patch initializes the tx_flags to SKBTX_ANY_TSTAMP like what
the UDP GSO does. But flag like SKBTX_SCHED_TSTAMP is not useful
and will not be used in the remaining path since the skb has already
passed the QDisc layer.

Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
---
 net/ipv4/tcp_offload.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 2308665b51c5..886582002425 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -13,12 +13,15 @@
 #include <net/tcp.h>
 #include <net/protocol.h>
 
-static void tcp_gso_tstamp(struct sk_buff *skb, unsigned int ts_seq,
+static void tcp_gso_tstamp(struct sk_buff *skb, struct sk_buff *gso_skb,
 			   unsigned int seq, unsigned int mss)
 {
+	u32 flags = skb_shinfo(gso_skb)->tx_flags & SKBTX_ANY_TSTAMP;
+	u32 ts_seq = skb_shinfo(gso_skb)->tskey;
+
 	while (skb) {
 		if (before(ts_seq, seq + mss)) {
-			skb_shinfo(skb)->tx_flags |= SKBTX_SW_TSTAMP;
+			skb_shinfo(skb)->tx_flags |= flags;
 			skb_shinfo(skb)->tskey = ts_seq;
 			return;
 		}
@@ -193,8 +196,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
 	th = tcp_hdr(skb);
 	seq = ntohl(th->seq);
 
-	if (unlikely(skb_shinfo(gso_skb)->tx_flags & SKBTX_SW_TSTAMP))
-		tcp_gso_tstamp(segs, skb_shinfo(gso_skb)->tskey, seq, mss);
+	if (unlikely(skb_shinfo(gso_skb)->tx_flags & (SKBTX_ANY_TSTAMP)))
+		tcp_gso_tstamp(segs, gso_skb, seq, mss);
 
 	newcheck = ~csum_fold(csum_add(csum_unfold(th->check), delta));
 
-- 
2.43.5

Re: [PATCH net-next] net-timestamp: support TCP GSO case for a few missing flags

[Willem de Bruijn]

Jason Xing wrote:
> When I read through the TSO codes, I found out that we probably
> miss initializing the tx_flags of last seg when TSO is turned
> off,

When falling back onto TCP GSO. Good catch.

This is a fix, so should target net?

> which means at the following points no more timestamp
> (for this last one) will be generated. There are three flags
> to be handled in this patch:
> 1. SKBTX_HW_TSTAMP
> 2. SKBTX_HW_TSTAMP_USE_CYCLES

Nit: this no longer exists

(But it will affect the upcoming completion timestamp.)

> 3. SKBTX_BPF
> 
> This patch initializes the tx_flags to SKBTX_ANY_TSTAMP like what
> the UDP GSO does. But flag like SKBTX_SCHED_TSTAMP is not useful
> and will not be used in the remaining path since the skb has already
> passed the QDisc layer.

Unless multiple layers of qdiscs (e.g., bonding or tunneling) and
GSO is applied on the first layer, and SKBTX_SW_TSTAMP is not
requested. But SCHED without SW is an unlikely configuration

Probably best to just drop this.

> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>  net/ipv4/tcp_offload.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index 2308665b51c5..886582002425 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -13,12 +13,15 @@
>  #include <net/tcp.h>
>  #include <net/protocol.h>
>  
> -static void tcp_gso_tstamp(struct sk_buff *skb, unsigned int ts_seq,
> +static void tcp_gso_tstamp(struct sk_buff *skb, struct sk_buff *gso_skb,
>  			   unsigned int seq, unsigned int mss)
>  {
> +	u32 flags = skb_shinfo(gso_skb)->tx_flags & SKBTX_ANY_TSTAMP;
> +	u32 ts_seq = skb_shinfo(gso_skb)->tskey;
> +
>  	while (skb) {
>  		if (before(ts_seq, seq + mss)) {
> -			skb_shinfo(skb)->tx_flags |= SKBTX_SW_TSTAMP;
> +			skb_shinfo(skb)->tx_flags |= flags;
>  			skb_shinfo(skb)->tskey = ts_seq;
>  			return;
>  		}
> @@ -193,8 +196,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
>  	th = tcp_hdr(skb);
>  	seq = ntohl(th->seq);
>  
> -	if (unlikely(skb_shinfo(gso_skb)->tx_flags & SKBTX_SW_TSTAMP))
> -		tcp_gso_tstamp(segs, skb_shinfo(gso_skb)->tskey, seq, mss);
> +	if (unlikely(skb_shinfo(gso_skb)->tx_flags & (SKBTX_ANY_TSTAMP)))

no need for the extra parentheses

> +		tcp_gso_tstamp(segs, gso_skb, seq, mss);
>  
>  	newcheck = ~csum_fold(csum_add(csum_unfold(th->check), delta));
>  
> -- 
> 2.43.5
> 

Re: [PATCH net-next] net-timestamp: support TCP GSO case for a few missing flags

[Jason Xing]

On Mon, Mar 3, 2025 at 10:17 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > When I read through the TSO codes, I found out that we probably
> > miss initializing the tx_flags of last seg when TSO is turned
> > off,
>
> When falling back onto TCP GSO. Good catch.
>
> This is a fix, so should target net?

After seeing your comment below, I'm not sure if the next version
targets the net branch because SKBTX_BPF flag is not in the net branch.

If target net-net tree, I would add the following:
Fixes: 6b98ec7e882af ("bpf: Add BPF_SOCK_OPS_TSTAMP_SCHED_CB callback")
Fixes: 4ed2d765dfacc ("net-timestamp: TCP timestamping")

>
> > which means at the following points no more timestamp
> > (for this last one) will be generated. There are three flags
> > to be handled in this patch:
> > 1. SKBTX_HW_TSTAMP
> > 2. SKBTX_HW_TSTAMP_USE_CYCLES
>
> Nit: this no longer exists

Right, I wrote on the old branch, sorry.

>
> (But it will affect the upcoming completion timestamp.)
>
> > 3. SKBTX_BPF
> >
> > This patch initializes the tx_flags to SKBTX_ANY_TSTAMP like what
> > the UDP GSO does. But flag like SKBTX_SCHED_TSTAMP is not useful
> > and will not be used in the remaining path since the skb has already
> > passed the QDisc layer.
>
> Unless multiple layers of qdiscs (e.g., bonding or tunneling) and
> GSO is applied on the first layer, and SKBTX_SW_TSTAMP is not
> requested. But SCHED without SW is an unlikely configuration
> Probably best to just drop this.

Got it. I will remove this description.

>
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> > ---
> >  net/ipv4/tcp_offload.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index 2308665b51c5..886582002425 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -13,12 +13,15 @@
> >  #include <net/tcp.h>
> >  #include <net/protocol.h>
> >
> > -static void tcp_gso_tstamp(struct sk_buff *skb, unsigned int ts_seq,
> > +static void tcp_gso_tstamp(struct sk_buff *skb, struct sk_buff *gso_skb,
> >                          unsigned int seq, unsigned int mss)
> >  {
> > +     u32 flags = skb_shinfo(gso_skb)->tx_flags & SKBTX_ANY_TSTAMP;
> > +     u32 ts_seq = skb_shinfo(gso_skb)->tskey;
> > +
> >       while (skb) {
> >               if (before(ts_seq, seq + mss)) {
> > -                     skb_shinfo(skb)->tx_flags |= SKBTX_SW_TSTAMP;
> > +                     skb_shinfo(skb)->tx_flags |= flags;
> >                       skb_shinfo(skb)->tskey = ts_seq;
> >                       return;
> >               }
> > @@ -193,8 +196,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
> >       th = tcp_hdr(skb);
> >       seq = ntohl(th->seq);
> >
> > -     if (unlikely(skb_shinfo(gso_skb)->tx_flags & SKBTX_SW_TSTAMP))
> > -             tcp_gso_tstamp(segs, skb_shinfo(gso_skb)->tskey, seq, mss);
> > +     if (unlikely(skb_shinfo(gso_skb)->tx_flags & (SKBTX_ANY_TSTAMP)))
>
> no need for the extra parentheses

Will correct it.

Thank for the review,
Jason


>
> > +             tcp_gso_tstamp(segs, gso_skb, seq, mss);
> >
> >       newcheck = ~csum_fold(csum_add(csum_unfold(th->check), delta));
> >
> > --
> > 2.43.5
> >
>
>

Re: [PATCH net-next] net-timestamp: support TCP GSO case for a few missing flags

[Willem de Bruijn]

Jason Xing wrote:
> On Mon, Mar 3, 2025 at 10:17 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > When I read through the TSO codes, I found out that we probably
> > > miss initializing the tx_flags of last seg when TSO is turned
> > > off,
> >
> > When falling back onto TCP GSO. Good catch.
> >
> > This is a fix, so should target net?
> 
> After seeing your comment below, I'm not sure if the next version
> targets the net branch because SKBTX_BPF flag is not in the net branch.

HWTSTAMP is sufficient reason
 
> If target net-net tree, I would add the following:
> Fixes: 6b98ec7e882af ("bpf: Add BPF_SOCK_OPS_TSTAMP_SCHED_CB callback")
> Fixes: 4ed2d765dfacc ("net-timestamp: TCP timestamping")

Please only add one Fixes tag. In this case 4ed2d765dfacc

Re: [PATCH net-next] net-timestamp: support TCP GSO case for a few missing flags

[Jason Xing]

On Mon, Mar 3, 2025 at 9:49 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Mon, Mar 3, 2025 at 10:17 AM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > When I read through the TSO codes, I found out that we probably
> > > > miss initializing the tx_flags of last seg when TSO is turned
> > > > off,
> > >
> > > When falling back onto TCP GSO. Good catch.
> > >
> > > This is a fix, so should target net?
> >
> > After seeing your comment below, I'm not sure if the next version
> > targets the net branch because SKBTX_BPF flag is not in the net branch.
>
> HWTSTAMP is sufficient reason

Got it.

>
> > If target net-net tree, I would add the following:
> > Fixes: 6b98ec7e882af ("bpf: Add BPF_SOCK_OPS_TSTAMP_SCHED_CB callback")
> > Fixes: 4ed2d765dfacc ("net-timestamp: TCP timestamping")
>
> Please only add one Fixes tag. In this case 4ed2d765dfacc

Okay, I will do it as you said.

Sorry to ask one more thing: should I mention SKBTX_BPF in the commit
message like "...SKBTX_BPF for now is only net-next material, but it
can be fixed by this patch as well"? I'm not sure if it's allowed to
say like that since we target the net branch.

Thanks,
Jason

Re: [PATCH net-next] net-timestamp: support TCP GSO case for a few missing flags

[Willem de Bruijn]

Jason Xing wrote:
> On Mon, Mar 3, 2025 at 9:49 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > On Mon, Mar 3, 2025 at 10:17 AM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > Jason Xing wrote:
> > > > > When I read through the TSO codes, I found out that we probably
> > > > > miss initializing the tx_flags of last seg when TSO is turned
> > > > > off,
> > > >
> > > > When falling back onto TCP GSO. Good catch.
> > > >
> > > > This is a fix, so should target net?
> > >
> > > After seeing your comment below, I'm not sure if the next version
> > > targets the net branch because SKBTX_BPF flag is not in the net branch.
> >
> > HWTSTAMP is sufficient reason
> 
> Got it.
> 
> >
> > > If target net-net tree, I would add the following:
> > > Fixes: 6b98ec7e882af ("bpf: Add BPF_SOCK_OPS_TSTAMP_SCHED_CB callback")
> > > Fixes: 4ed2d765dfacc ("net-timestamp: TCP timestamping")
> >
> > Please only add one Fixes tag. In this case 4ed2d765dfacc
> 
> Okay, I will do it as you said.
> 
> Sorry to ask one more thing: should I mention SKBTX_BPF in the commit
> message like "...SKBTX_BPF for now is only net-next material, but it
> can be fixed by this patch as well"? I'm not sure if it's allowed to
> say like that since we target the net branch.

Absolutely fine. That is relevant information.