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:
> 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.

>  		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
>