Re: [PATCH net v1 1/3] net: introduce helper to resolve hardware timestamps from skb

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

Kohei Enju wrote:
> Move the logic that resolves a hardware timestamp from an skb, including
> late timestamp resolution via netdev_get_tstamp(), from net/socket.c to
> a common helper.
> 
> Let's allow other networking code to reuse the same resolution path.
> 
> Signed-off-by: Kohei Enju <kohei@enjuk.jp>

Thanks for the fix series.

Fixes require a Fixes tag. See also https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html.

I suggest merging this and the second patch, as this is not a
standalone fix, and the other is a one-line change.

> ---
>  include/linux/skbuff.h | 11 +++++++++++
>  net/core/skbuff.c      | 27 +++++++++++++++++++++++++++
>  net/socket.c           | 27 +++------------------------
>  3 files changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 2bcf78a4de7b..651a5ae8b11c 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -4731,6 +4731,17 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb, const struct sk_buff *ack_skb,
>  void skb_tstamp_tx(struct sk_buff *orig_skb,
>  		   struct skb_shared_hwtstamps *hwtstamps);
>  
> +/**
> + * skb_get_hwtstamp - resolve a hardware timestamp from an skb
> + * @skb:	skb carrying the timestamp
> + * @cycles:	true to request the free-running cycle-based timestamp
> + * @if_index:	optional return pointer for the originating netdev ifindex
> + *
> + * Return: resolved hardware timestamp, or the stored skb hwtstamp when no
> + * device-specific late timestamp resolution is needed.
> + */
> +ktime_t skb_get_hwtstamp(struct sk_buff *skb, bool cycles, int *if_index);
> +
>  /**
>   * skb_tx_timestamp() - Driver hook for transmit timestamping
>   *
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7dad68e3b518..d11f4e2e9391 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5729,6 +5729,33 @@ void skb_tstamp_tx(struct sk_buff *orig_skb,
>  }
>  EXPORT_SYMBOL_GPL(skb_tstamp_tx);
>  
> +ktime_t skb_get_hwtstamp(struct sk_buff *skb, bool cycles, int *if_index)
> +{
> +	struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
> +	struct net_device *orig_dev;
> +	ktime_t hwtstamp;
> +
> +	if (if_index)
> +		*if_index = 0;
> +
> +	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV))
> +		return shhwtstamps->hwtstamp;
> +
> +	rcu_read_lock();
> +	orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> +	if (orig_dev) {
> +		if (if_index)
> +			*if_index = orig_dev->ifindex;
> +		hwtstamp = netdev_get_tstamp(orig_dev, shhwtstamps, cycles);
> +	} else {
> +		hwtstamp = shhwtstamps->hwtstamp;
> +	}
> +	rcu_read_unlock();
> +
> +	return hwtstamp;
> +}
> +EXPORT_SYMBOL_GPL(skb_get_hwtstamp);
> +
>  #ifdef CONFIG_WIRELESS
>  void skb_complete_wifi_ack(struct sk_buff *skb, bool acked)
>  {
> diff --git a/net/socket.c b/net/socket.c
> index 22a412fdec07..95b21b16a0fc 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -876,21 +876,7 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp)
>  static ktime_t get_timestamp(struct sock *sk, struct sk_buff *skb, int *if_index)
>  {
>  	bool cycles = READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_BIND_PHC;
> -	struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
> -	struct net_device *orig_dev;
> -	ktime_t hwtstamp;
> -
> -	rcu_read_lock();
> -	orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> -	if (orig_dev) {
> -		*if_index = orig_dev->ifindex;
> -		hwtstamp = netdev_get_tstamp(orig_dev, shhwtstamps, cycles);
> -	} else {
> -		hwtstamp = shhwtstamps->hwtstamp;
> -	}
> -	rcu_read_unlock();
> -
> -	return hwtstamp;
> +	return skb_get_hwtstamp(skb, cycles, if_index);
>  }

At this point simpler to remove get_timestamp entirely. It's an
unnecessary layer of indirection.

Perhaps pass sk_tsflags rather than bool to skb_get_hwtstamp.
  
>  static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
> @@ -940,7 +926,6 @@ int skb_get_tx_timestamp(struct sk_buff *skb, struct sock *sk,
>  {
>  	u32 tsflags = READ_ONCE(sk->sk_tsflags);
>  	ktime_t hwtstamp;
> -	int if_index = 0;
>  
>  	if ((tsflags & SOF_TIMESTAMPING_SOFTWARE) &&
>  	    ktime_to_timespec64_cond(skb->tstamp, ts))
> @@ -950,10 +935,7 @@ int skb_get_tx_timestamp(struct sk_buff *skb, struct sock *sk,
>  	    skb_is_swtx_tstamp(skb, false))
>  		return -ENOENT;
>  
> -	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)
> -		hwtstamp = get_timestamp(sk, skb, &if_index);
> -	else
> -		hwtstamp = skb_hwtstamps(skb)->hwtstamp;
> +	hwtstamp = get_timestamp(sk, skb, NULL);
>  
>  	if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
>  		hwtstamp = ptp_convert_timestamp(&hwtstamp,
> @@ -1033,10 +1015,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>  	      !(tsflags & SOF_TIMESTAMPING_OPT_RX_FILTER))) &&
>  	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
>  		if_index = 0;
> -		if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_NETDEV)
> -			hwtstamp = get_timestamp(sk, skb, &if_index);
> -		else
> -			hwtstamp = shhwtstamps->hwtstamp;
> +		hwtstamp = get_timestamp(sk, skb, &if_index);
>  
>  		if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
>  			hwtstamp = ptp_convert_timestamp(&hwtstamp,
> -- 
> 2.53.0
>