Date:   Thu, 01 Dec 2022 15:58:52 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
        kuba@...nel.org, edumazet@...gle.com
Cc:     Jacob Keller <jacob.e.keller@...el.com>, netdev@...r.kernel.org,
        richardcochran@...il.com, Gurucharan G <gurucharanx.g@...el.com>
Subject: Re: [PATCH net-next 06/14] ice: handle discarding old Tx requests
 in ice_ptp_tx_tstamp

On Wed, 2022-11-30 at 11:43 -0800, Tony Nguyen wrote:
> From: Jacob Keller <jacob.e.keller@...el.com>
> 
> Currently the driver uses the PTP kthread to process handling and
> discarding of stale Tx timestamp requests. The function
> ice_ptp_tx_tstamp_cleanup is used for this.
> 
> A separate thread creates complications for the driver as we now have both
> the main Tx timestamp processing IRQ checking timestamps as well as the
> kthread.
> 
> Rather than using the kthread to handle this, simply check for stale
> timestamps within the ice_ptp_tx_tstamp function. This function must
> already process the timestamps anyways.
> 
> If a Tx timestamp has been waiting for 2 seconds we simply clear the bit
> and discard the SKB. This avoids the complication of having separate
> threads polling, reducing overall CPU work.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Tested-by: Gurucharan G <gurucharanx.g@...el.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp.c | 106 ++++++++++-------------
>  1 file changed, 45 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 1564c72189bf..58e527f202c0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -626,15 +626,32 @@ static u64 ice_ptp_extend_40b_ts(struct ice_pf *pf, u64 in_tstamp)
>   * Note that we only take the tracking lock when clearing the bit and when
>   * checking if we need to re-queue this task. The only place where bits can be
>   * set is the hard xmit routine where an SKB has a request flag set. The only
> - * places where we clear bits are this work function, or the periodic cleanup
> - * thread. If the cleanup thread clears a bit we're processing we catch it
> - * when we lock to clear the bit and then grab the SKB pointer. If a Tx thread
> - * starts a new timestamp, we might not begin processing it right away but we
> - * will notice it at the end when we re-queue the task. If a Tx thread starts
> - * a new timestamp just after this function exits without re-queuing,
> - * the interrupt when the timestamp finishes should trigger. Avoiding holding
> - * the lock for the entire function is important in order to ensure that Tx
> - * threads do not get blocked while waiting for the lock.
> + * places where we clear bits are this work function, or when flushing the Tx
> + * timestamp tracker.
> + *
> + * If the Tx tracker gets flushed while we're processing a packet, we catch
> + * this because we grab the SKB pointer under lock. If the SKB is NULL we know
> + * that another thread already discarded the SKB and we can avoid passing it
> + * up to the stack.
> + *
> + * If a Tx thread starts a new timestamp, we might not begin processing it
> + * right away but we will notice it at the end when we re-queue the task.
> + *
> + * If a Tx thread starts a new timestamp just after this function exits, the
> + * interrupt for that timestamp should re-trigger this function once
> + * a timestamp is ready.
> + *
> + * Note that minimizing the time we hold the lock is important. If we held the
> + * lock for the entire function we would unnecessarily block the Tx hot path
> + * which needs to set the timestamp index. Limiting how long we hold the lock
> + * ensures we do not block Tx threads.
> + *
> + * If a Tx packet has been waiting for more than 2 seconds, it is not possible
> + * to correctly extend the timestamp using the cached PHC time. It is
> + * extremely unlikely that a packet will ever take this long to timestamp. If
> + * we detect a Tx timestamp request that has waited for this long we assume
> + * the packet will never be sent by hardware and discard it without reading
> + * the timestamp register.
>   */
>  static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
>  {
> @@ -653,9 +670,20 @@ static bool ice_ptp_tx_tstamp(struct ice_ptp_tx *tx)
>  		struct skb_shared_hwtstamps shhwtstamps = {};
>  		u8 phy_idx = idx + tx->offset;
>  		u64 raw_tstamp, tstamp;
> +		bool drop_ts = false;
>  		struct sk_buff *skb;
>  		int err;
>  
> +		/* Drop packets which have waited for more than 2 seconds */
> +		if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
> +			drop_ts = true;
> +
> +			/* Count the number of Tx timestamps that timed out */
> +			pf->ptp.tx_hwtstamp_timeouts++;
> +
> +			goto skip_ts_read;

This skips raw_tstamp initialization and causes later compiler warning
when accessing raw_tstamp.

You probably need to duplicate/factor out a bit of later code to fix
this.

Cheers,

Paolo

Powered by blists - more mailing lists