Re: [PATCH net-next 06/14] ice: handle discarding old Tx requests in ice_ptp_tx_tstamp

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2022-12-01 at 15:58 +0100, Paolo Abeni wrote:
> On Wed, 2022-11-30 at 11:43 -0800, Tony Nguyen wrote:
> > From: Jacob Keller <jacob.e.keller@intel.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@intel.com>
> > Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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.

Ah, I see the warning is resolved in the next patch. Perhaps it's
worthy to move the relevant chunk here?

Thanks,

Paolo