Re: [PATCH bpf-next v8 08/12] bpf: support hw SCM_TSTAMP_SND of SO_TIMESTAMPING

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

Jason Xing wrote:
> Patch finishes the hardware part.

For consistency with previous patches, and to make sense of this
commit message on its own, when stumbling on it, e.g., through
git blame, replace the above with

Support hw SCM_TSTAMP_SND case. 

> Then bpf program can fetch the
> hwstamp from skb directly.
> 
> To avoid changing so many callers using SKBTX_HW_TSTAMP from drivers,
> use this simple modification like this patch does to support printing
> hardware timestamp.

Which simple modification? Please state explicitly.
 
> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com>
> ---
>  include/linux/skbuff.h         |  4 +++-
>  include/uapi/linux/bpf.h       |  7 +++++++
>  net/core/skbuff.c              | 13 +++++++------
>  net/dsa/user.c                 |  2 +-
>  net/socket.c                   |  2 +-
>  tools/include/uapi/linux/bpf.h |  7 +++++++
>  6 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index de8d3bd311f5..df2d790ae36b 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -471,7 +471,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 = 1 << 0,

Perhaps we can have a more descriptive name. SKBTX_HW_TSTAMP_NOBPF?

>  
>  	/* generate software time stamp when queueing packet to NIC */
>  	SKBTX_SW_TSTAMP = 1 << 1,
> @@ -495,6 +495,8 @@ enum {
>  	SKBTX_BPF = 1 << 7,
>  };
>  
> +#define SKBTX_HW_TSTAMP		(__SKBTX_HW_TSTAMP | 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 6a1083bcf779..4c3566f623c2 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7040,6 +7040,13 @@ 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. At the same time, hwtstamps
> +					 * of skb is initialized as the
> +					 * timestamp that hardware just
> +					 * generates.

"hwtstamp of skb is initialized" is this something new? Or are you
just conveying that when this callback is called, skb_hwtstamps(skb)
is non-zero? If the latter, drop, because the wording is confusing.

> +					 */
>  };
>  
>  /* 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 b22d079e7143..264435f989ad 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5548,7 +5548,7 @@ static bool skb_enable_app_tstamp(struct sk_buff *skb, int tstype, bool sw)
>  		flag = SKBTX_SCHED_TSTAMP;
>  		break;
>  	case SCM_TSTAMP_SND:
> -		flag = sw ? SKBTX_SW_TSTAMP : SKBTX_HW_TSTAMP;
> +		flag = sw ? SKBTX_SW_TSTAMP : __SKBTX_HW_TSTAMP;
>  		break;
>  	case SCM_TSTAMP_ACK:
>  		if (TCP_SKB_CB(skb)->txstamp_ack)
> @@ -5565,7 +5565,8 @@ static bool skb_enable_app_tstamp(struct sk_buff *skb, int tstype, bool sw)
>  }
>  
>  static void skb_tstamp_tx_bpf(struct sk_buff *skb, struct sock *sk,
> -			      int tstype, bool sw)
> +			      int tstype, bool sw,
> +			      struct skb_shared_hwtstamps *hwtstamps)
>  {
>  	int op;
>  
> @@ -5574,9 +5575,9 @@ static void skb_tstamp_tx_bpf(struct sk_buff *skb, struct sock *sk,
>  		op = BPF_SOCK_OPS_TS_SCHED_OPT_CB;
>  		break;
>  	case SCM_TSTAMP_SND:
> -		if (!sw)
> -			return;
> -		op = BPF_SOCK_OPS_TS_SW_OPT_CB;
> +		op = sw ? BPF_SOCK_OPS_TS_SW_OPT_CB : BPF_SOCK_OPS_TS_HW_OPT_CB;
> +		if (!sw && hwtstamps)
> +			*skb_hwtstamps(skb) = *hwtstamps;

Isn't this called by drivers that have actually set skb_hwtstamps?
>  		break;
>  	default:
>  		return;
> @@ -5599,7 +5600,7 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>  
>  	/* bpf extension feature entry */
>  	if (skb_shinfo(orig_skb)->tx_flags & SKBTX_BPF)
> -		skb_tstamp_tx_bpf(orig_skb, sk, tstype, sw);
> +		skb_tstamp_tx_bpf(orig_skb, sk, tstype, sw, hwtstamps);
>  
>  	/* application feature entry */
>  	if (!skb_enable_app_tstamp(orig_skb, tstype, sw))
> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index 291ab1b4acc4..ae715bf0ae75 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -897,7 +897,7 @@ static void dsa_skb_tx_timestamp(struct dsa_user_priv *p,
>  {
>  	struct dsa_switch *ds = p->dp->ds;
>  
> -	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> +	if (!(skb_shinfo(skb)->tx_flags & __SKBTX_HW_TSTAMP))
>  		return;
>  
>  	if (!ds->ops->port_txtstamp)
> diff --git a/net/socket.c b/net/socket.c
> index 262a28b59c7f..70eabb510ce6 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -676,7 +676,7 @@ void __sock_tx_timestamp(__u32 tsflags, __u8 *tx_flags)
>  	u8 flags = *tx_flags;
>  
>  	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
> -		flags |= SKBTX_HW_TSTAMP;
> +		flags |= __SKBTX_HW_TSTAMP;
>  
>  		/* PTP hardware clocks can provide a free running cycle counter
>  		 * as a time base for virtual clocks. Tell driver to use the
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 9bd1c7c77b17..974b7f61d11f 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -7033,6 +7033,13 @@ 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. At the same time, hwtstamps
> +					 * of skb is initialized as the
> +					 * timestamp that hardware just
> +					 * generates.
> +					 */
>  };
>  
>  /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
> -- 
> 2.43.5
>