| Message ID | 20250228164904.47511-1-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net-timestamp: support TCP GSO case for a few missing flags | expand |
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 >
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 > > > >
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
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
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.
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));
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(-)