Re: [Intel-wired-lan] [PATCH iwl-next v10 14/14] iavf: add support for Rx timestamps to hotpath

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Wed, 21 Aug 2024 14:15:39 +0200

> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Add support for receive timestamps to the Rx hotpath. This support only
> works when using the flexible descriptor format, so make sure that we
> request this format by default if we have receive timestamp support
> available in the PTP capabilities.
> 
> In order to report the timestamps to userspace, we need to perform
> timestamp extension. The Rx descriptor does actually contain the "40
> bit" timestamp. However, upper 32 bits which contain nanoseconds are
> conveniently stored separately in the descriptor. We could extract the
> 32bits and lower 8 bits, then perform a bitwise OR to calculate the
> 40bit value. This makes no sense, because the timestamp extension
> algorithm would simply discard the lower 8 bits anyways.
> 
> Thus, implement timestamp extension as iavf_ptp_extend_32b_timestamp(),
> and extract and forward only the 32bits of nominal nanoseconds.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
> Reviewed-by: Sunil Goutham <sgoutham@marvell.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 11 +++-
>  drivers/net/ethernet/intel/iavf/iavf_ptp.c  | 61 +++++++++++++++++++++
>  drivers/net/ethernet/intel/iavf/iavf_ptp.h  |  4 ++
>  drivers/net/ethernet/intel/iavf/iavf_txrx.c | 45 +++++++++++++++
>  drivers/net/ethernet/intel/iavf/iavf_type.h |  1 +
>  5 files changed, 121 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 1458410ca560..ebc01a8d1ac6 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -730,6 +730,15 @@ static u8 iavf_select_rx_desc_format(const struct iavf_adapter *adapter)
>  	if (!IAVF_RXDID_ALLOWED(adapter))
>  		return VIRTCHNL_RXDID_1_32B_BASE;
>  
> +	/* Rx timestamping requires the use of flexible NIC descriptors */
> +	if (iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_RX_TSTAMP)) {
> +		if (rxdids & BIT(VIRTCHNL_RXDID_2_FLEX_SQ_NIC))
> +			return VIRTCHNL_RXDID_2_FLEX_SQ_NIC;
> +
> +		pci_warn(adapter->pdev,
> +			 "Unable to negotiate flexible descriptor format\n");
> +	}
> +
>  	/* Warn if the PF does not list support for the default legacy
>  	 * descriptor format. This shouldn't happen, as this is the format
>  	 * used if VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC is not supported. It is
> @@ -737,7 +746,7 @@ static u8 iavf_select_rx_desc_format(const struct iavf_adapter *adapter)
>  	 * support for the format.
>  	 */
>  	if (!(rxdids & VIRTCHNL_RXDID_1_32B_BASE_M))
> -		dev_warn(&adapter->pdev->dev, "PF does not list support for default Rx descriptor format\n");
> +		pci_warn(adapter->pdev, "PF does not list support for default Rx descriptor format\n");

Unrelated (addresses my earlier comments, but not related to this patch).

[...]

> @@ -1085,6 +1086,49 @@ static void iavf_flex_rx_hash(const struct iavf_ring *ring,
>  	}
>  }
>  
> +/**
> + * iavf_flex_rx_tstamp - Capture Rx timestamp from the descriptor
> + * @rx_ring: descriptor ring
> + * @rx_desc: specific descriptor
> + * @skb: skb currently being received
> + *
> + * Read the Rx timestamp value from the descriptor and pass it to the stack.
> + *
> + * This function only operates on the VIRTCHNL_RXDID_2_FLEX_SQ_NIC flexible
> + * descriptor writeback format.
> + */
> +static void iavf_flex_rx_tstamp(const struct iavf_ring *rx_ring,
> +				const struct iavf_rx_desc *rx_desc,
> +				struct sk_buff *skb)
> +{
> +	struct iavf_adapter *adapter;
> +	__le64 qw2 = rx_desc->qw2;
> +	__le64 qw3 = rx_desc->qw3;

You only need both @qw2 and @qw3 once, so I'd avoid creating these
variables.
But given that these qwords are used in other functions, I'd just pass
both qwords as function arguments instead of @rx_desc.

> +	u32 tstamp;
> +	u64 ns;
> +
> +	/* Skip processing if timestamps aren't enabled */
> +	if (!(rx_ring->flags & IAVF_TXRX_FLAGS_HW_TSTAMP))
> +		return;
> +
> +	/* Check if this Rx descriptor has a valid timestamp */
> +	if (!le64_get_bits(qw2, IAVF_PTP_40B_TSTAMP_VALID))
> +		return;
> +
> +	/* the ts_low field only contains the valid bit and sub-nanosecond
> +	 * precision, so we don't need to extract it.
> +	 */
> +	tstamp = le64_get_bits(qw3, IAVF_RXD_FLEX_QW3_TSTAMP_HIGH_M);
> +
> +	adapter = netdev_priv(rx_ring->netdev);
> +	ns = iavf_ptp_extend_32b_timestamp(adapter->ptp.cached_phc_time,
> +					   tstamp);

Hmm, this netdev_priv() + adapter access etc. doesn't look good on
hotpath. I think it would be better to cache a pointer to adapter->ptp
in each ring structure and update them when it gets changed.

> +
> +	*skb_hwtstamps(skb) = (struct skb_shared_hwtstamps) {
> +		.hwtstamp = ns_to_ktime(ns),
> +	};

Thanks,
Olek