Re: [PATCH net-next v4 06/11] net-timestamp: support SCM_TSTAMP_ACK for bpf extension

[Martin KaFai Lau <martin.lau@linux.dev>]

On 12/7/24 9:37 AM, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> 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@tencent.com>
> ---
>   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.

>   			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 */
> -	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?