On Wed, Feb 5, 2025 at 11:47 PM Willem de Bruijn
<willemdebruijn.kernel@xxxxxxxxx> wrote:
>
> Jason Xing wrote:
> > Handle the ACK timestamp case. Actually testing SKBTX_BPF flag
> > can work, but Introducing a new txstamp_ack_bpf to avoid cache
>
> repeat comment: s/Introducing/introduce
>
> > line misses in tcp_ack_tstamp() is needed. To be more specific,
> > in most cases, normal flows would not access skb_shinfo as
> > txstamp_ack is zero, so that this function won't appear in the
> > hot spot lists. Introducing a new member txstamp_ack_bpf works
> > similarly.
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@xxxxxxxxx>
> > ---
> > include/net/tcp.h | 3 ++-
> > include/uapi/linux/bpf.h | 5 +++++
> > net/core/skbuff.c | 3 +++
> > net/ipv4/tcp_input.c | 3 ++-
> > net/ipv4/tcp_output.c | 5 +++++
> > tools/include/uapi/linux/bpf.h | 5 +++++
> > 6 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index 293047694710..88429e422301 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -959,9 +959,10 @@ struct tcp_skb_cb {
> > __u8 sacked; /* State flags for SACK. */
> > __u8 ip_dsfield; /* IPv4 tos or IPv6 dsfield */
> > __u8 txstamp_ack:1, /* Record TX timestamp for ack? */
> > + txstamp_ack_bpf:1, /* ack timestamp for bpf use */
> > eor:1, /* Is skb MSG_EOR marked? */
> > has_rxtstamp:1, /* SKB has a RX timestamp */
> > - unused:5;
> > + unused:4;
> > __u32 ack_seq; /* Sequence number ACK'd */
> > union {
> > struct {
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 4c3566f623c2..800122a8abe5 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -7047,6 +7047,11 @@ enum {
> > * timestamp that hardware just
> > * generates.
> > */
> > + BPF_SOCK_OPS_TS_ACK_OPT_CB, /* Called when all the skbs in the
> > + * same sendmsg call are acked
> > + * when SK_BPF_CB_TX_TIMESTAMPING
> > + * feature is on.
> > + */
> > };
> >
> > /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 264435f989ad..a8463fef574a 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -5579,6 +5579,9 @@ static void skb_tstamp_tx_bpf(struct sk_buff *skb, struct sock *sk,
> > if (!sw && hwtstamps)
> > *skb_hwtstamps(skb) = *hwtstamps;
> > break;
> > + case SCM_TSTAMP_ACK:
> > + op = BPF_SOCK_OPS_TS_ACK_OPT_CB;
> > + break;
> > default:
> > return;
> > }
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 62252702929d..c8945f5be31b 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -3323,7 +3323,8 @@ static void tcp_ack_tstamp(struct sock *sk, struct sk_buff *skb,
> > const struct skb_shared_info *shinfo;
> >
> > /* Avoid cache line misses to get skb_shinfo() and shinfo->tx_flags */
> > - if (likely(!TCP_SKB_CB(skb)->txstamp_ack))
> > + if (likely(!TCP_SKB_CB(skb)->txstamp_ack &&
> > + !TCP_SKB_CB(skb)->txstamp_ack_bpf))
>
> Here and elsewhere: instead of requiring multiple tests, how about
> extending txstamp_ack to a two-bit field, so that a single branch
> suffices.
It should work. Let me assume 1 stands for so_timestamping, 2 bpf extension?
>
> > return;
> >
> > shinfo = skb_shinfo(skb);
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > index 695749807c09..fc84ca669b76 100644
> > --- a/net/ipv4/tcp_output.c
> > +++ b/net/ipv4/tcp_output.c
> > @@ -1556,6 +1556,7 @@ static void tcp_adjust_pcount(struct sock *sk, const struct sk_buff *skb, int de
> > static bool tcp_has_tx_tstamp(const struct sk_buff *skb)
> > {
> > return TCP_SKB_CB(skb)->txstamp_ack ||
> > + TCP_SKB_CB(skb)->txstamp_ack_bpf ||
> > (skb_shinfo(skb)->tx_flags & SKBTX_ANY_TSTAMP);
> > }
> >
> > @@ -1572,7 +1573,9 @@ static void tcp_fragment_tstamp(struct sk_buff *skb, struct sk_buff *skb2)
> > shinfo2->tx_flags |= tsflags;
> > swap(shinfo->tskey, shinfo2->tskey);
> > TCP_SKB_CB(skb2)->txstamp_ack = TCP_SKB_CB(skb)->txstamp_ack;
> > + TCP_SKB_CB(skb2)->txstamp_ack_bpf = TCP_SKB_CB(skb)->txstamp_ack_bpf;
> > TCP_SKB_CB(skb)->txstamp_ack = 0;
> > + TCP_SKB_CB(skb)->txstamp_ack_bpf = 0;
> > }
> > }
> >
> > @@ -3213,6 +3216,8 @@ void tcp_skb_collapse_tstamp(struct sk_buff *skb,
> > shinfo->tskey = next_shinfo->tskey;
> > TCP_SKB_CB(skb)->txstamp_ack |=
> > TCP_SKB_CB(next_skb)->txstamp_ack;
> > + TCP_SKB_CB(skb)->txstamp_ack_bpf |=
> > + TCP_SKB_CB(next_skb)->txstamp_ack_bpf;
> > }
> > }
> >
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 974b7f61d11f..06e68d772989 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -7040,6 +7040,11 @@ enum {
> > * timestamp that hardware just
> > * generates.
> > */
> > + BPF_SOCK_OPS_TS_ACK_OPT_CB, /* Called when all the skbs in the
> > + * same sendmsg call are acked
> > + * when SK_BPF_CB_TX_TIMESTAMPING
> > + * feature is on.
> > + */
> > };
> >
> > /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> > --
> > 2.43.5
> >
>
>
