Re: [PATCH bpf-next v10 08/12] bpf: add BPF_SOCK_OPS_TS_HW_OPT_CB callback

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

On 2/11/25 10:18 PM, Jason Xing wrote:
> Support hw SCM_TSTAMP_SND case for bpf timestamping.
> 
> Add a new sock_ops callback, BPF_SOCK_OPS_TS_HW_OPT_CB. This
> callback will occur at the same timestamping point as the user
> space's hardware SCM_TSTAMP_SND. The BPF program can use it to
> get the same SCM_TSTAMP_SND timestamp without modifying the
> user-space application.
> 
> To avoid increase the code complexity, replace SKBTX_HW_TSTAMP
> with SKBTX_HW_TSTAMP_NOBPF instead of changing numerous callers
> from driver side using SKBTX_HW_TSTAMP. The new definition of
> SKBTX_HW_TSTAMP means the combination tests of socket timestamping
> and bpf timestamping. After this patch, drivers can work under the
> bpf timestamping.
> 
> Considering some drivers doesn't assign the skb with hardware
> timestamp, this patch do the assignment and then BPF program
> can acquire the hwstamp from skb directly.
> 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>   include/linux/skbuff.h         | 4 +++-
>   include/uapi/linux/bpf.h       | 4 ++++
>   net/core/skbuff.c              | 6 +++---
>   tools/include/uapi/linux/bpf.h | 4 ++++
>   4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 76582500c5ea..0b4f1889500d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -470,7 +470,7 @@ struct skb_shared_hwtstamps {
>   /* Definitions for tx_flags in struct skb_shared_info */
>   enum {
>   	/* generate hardware time stamp */
> -	SKBTX_HW_TSTAMP = 1 << 0,
> +	SKBTX_HW_TSTAMP_NOBPF = 1 << 0,
>   
>   	/* generate software time stamp when queueing packet to NIC */
>   	SKBTX_SW_TSTAMP = 1 << 1,
> @@ -494,6 +494,8 @@ enum {
>   	SKBTX_BPF = 1 << 7,
>   };
>   
> +#define SKBTX_HW_TSTAMP		(SKBTX_HW_TSTAMP_NOBPF | SKBTX_BPF)
> +
>   #define SKBTX_ANY_SW_TSTAMP	(SKBTX_SW_TSTAMP    | \
>   				 SKBTX_SCHED_TSTAMP | \
>   				 SKBTX_BPF)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index b3bd92281084..f70edd067edf 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7043,6 +7043,10 @@ enum {
>   					 * to the nic when SK_BPF_CB_TX_TIMESTAMPING
>   					 * feature is on.
>   					 */
> +	BPF_SOCK_OPS_TS_HW_OPT_CB,	/* Called in hardware phase 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 d80d2137692f..4930c43ee77b 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5547,7 +5547,7 @@ static bool skb_tstamp_tx_report_so_timestamping(struct sk_buff *skb,
>   	case SCM_TSTAMP_SCHED:
>   		return skb_shinfo(skb)->tx_flags & SKBTX_SCHED_TSTAMP;
>   	case SCM_TSTAMP_SND:
> -		return skb_shinfo(skb)->tx_flags & (hwts ? SKBTX_HW_TSTAMP :
> +		return skb_shinfo(skb)->tx_flags & (hwts ? SKBTX_HW_TSTAMP_NOBPF :
>   						    SKBTX_SW_TSTAMP);
>   	case SCM_TSTAMP_ACK:
>   		return TCP_SKB_CB(skb)->txstamp_ack;
> @@ -5568,9 +5568,9 @@ static void skb_tstamp_tx_report_bpf_timestamping(struct sk_buff *skb,
>   		op = BPF_SOCK_OPS_TS_SCHED_OPT_CB;
>   		break;
>   	case SCM_TSTAMP_SND:
> +		op = hwts ? BPF_SOCK_OPS_TS_HW_OPT_CB : BPF_SOCK_OPS_TS_SW_OPT_CB;

Remove this "hwts" test.

>   		if (hwts)

Reuse this and do everything in this "if else" statement.

> -			return;
> -		op = BPF_SOCK_OPS_TS_SW_OPT_CB;
> +			*skb_hwtstamps(skb) = *hwts;
>   		break;
>   	default:
>   		return;
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 9bd1c7c77b17..7b9652ce7e3c 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -7033,6 +7033,10 @@ enum {
>   					 * to the nic when SK_BPF_CB_TX_TIMESTAMPING
>   					 * feature is on.
>   					 */
> +	BPF_SOCK_OPS_TS_HW_OPT_CB,	/* Called in hardware phase 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