Jason Xing wrote:
> 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?
Sounds good
