Re: [PATCH bpf-next v8 09/12] bpf: support SCM_TSTAMP_ACK of SO_TIMESTAMPING

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jason Xing wrote:
> On Wed, Feb 5, 2025 at 11:47 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> 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@gmail.com>
> > > ---
> > >  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