| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8903a794-a740-405a-86bd-d97fb611e058@intel.com>
Date: Thu, 22 Aug 2024 18:54:36 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
<horms@...nel.org>, <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
<alexandr.lobakin@...el.com>
Subject: Re: [PATCH iwl-next v10 14/14] iavf: add support for Rx timestamps to
hotpath
From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Wed, 21 Aug 2024 14:15:39 +0200
> From: Jacob Keller <jacob.e.keller@...el.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@...el.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> Reviewed-by: Rahul Rameshbabu <rrameshbabu@...dia.com>
> Reviewed-by: Sunil Goutham <sgoutham@...vell.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.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
Powered by blists - more mailing lists