On Fri, Dec 13, 2024 at 6:36 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 12/7/24 9:37 AM, Jason Xing wrote:
> > From: Jason Xing <kernelxing@xxxxxxxxxxx>
> >
> > Handle the ACK timestamp case. Actually testing SKBTX_BPF flag
> > can work, but we need to Introduce a new txstamp_ack_bpf to avoid
> > cache line misses in tcp_ack_tstamp().
> >
> > Signed-off-by: Jason Xing <kernelxing@xxxxxxxxxxx>
> > ---
> > include/net/tcp.h | 3 ++-
> > include/uapi/linux/bpf.h | 5 +++++
> > net/core/skbuff.c | 9 ++++++---
> > net/ipv4/tcp_input.c | 3 ++-
> > net/ipv4/tcp_output.c | 5 +++++
> > tools/include/uapi/linux/bpf.h | 5 +++++
> > 6 files changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index e9b37b76e894..8e5103d3c6b9 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 */
>
> After quickly peeking at patch 8, I realize that the new txstamp_ack_bpf bit is
> not needed. SKBTX_BPF bit (in skb_shinfo(skb)->tx_flags) and the txstamp_ack_bpf
> are always set together. Then only use the SKBTX_BPF bit should be as good.
Please see the comments below :)
>
> > 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 a6d761f07f67..a0aff1b4eb61 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -7032,6 +7032,11 @@ enum {
> > * feature is on. It indicates the
> > * recorded timestamp.
> > */
> > + BPF_SOCK_OPS_TS_ACK_OPT_CB, /* Called when all the skbs are
> > + * acknowledged when SO_TIMESTAMPING
> > + * feature is on. It indicates the
> > + * recorded timestamp.
> > + */
> > };
> >
> > /* 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 73b15d6277f7..48b0c71e9522 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -5553,6 +5553,9 @@ static void __skb_tstamp_tx_bpf(struct sock *sk, struct sk_buff *skb, int tstype
> > case SCM_TSTAMP_SND:
> > op = BPF_SOCK_OPS_TS_SW_OPT_CB;
> > break;
> > + case SCM_TSTAMP_ACK:
> > + op = BPF_SOCK_OPS_TS_ACK_OPT_CB;
> > + break;
> > default:
> > return;
> > }
> > @@ -5632,9 +5635,9 @@ static bool skb_tstamp_is_set(const struct sk_buff *skb, int tstype, bool bpf_mo
> > return true;
> > return false;
> > case SCM_TSTAMP_ACK:
> > - if (TCP_SKB_CB(skb)->txstamp_ack)
> > - return true;
> > - return false;
> > + flag = bpf_mode ? TCP_SKB_CB(skb)->txstamp_ack_bpf :
> > + TCP_SKB_CB(skb)->txstamp_ack;
> > + return !!flag;
> > }
> >
> > return false;
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 5bdf13ac26ef..82bb26f5b214 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -3321,7 +3321,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 */
Please take a look at the above comment.
> > - if (likely(!TCP_SKB_CB(skb)->txstamp_ack))
> > + if (likely(!TCP_SKB_CB(skb)->txstamp_ack &&
> > + !TCP_SKB_CB(skb)->txstamp_ack_bpf))
>
> Change the test here to:
> if (likely(!TCP_SKB_CB(skb)->txstamp_ack &&
> !(skb_shinfo(skb)->tx_flags & SKBTX_BPF)))
>
> Does it make sense?
It surely works. Talking about the result only, introducing SKBTX_BPF
can work for all the cases. However, in the ACK case, the above code
snippet will access the shinfo->tx_flags, which triggers cache line
misses. I also mentioned this on purpose in the patch [06/11].
