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

[Jacob Keller <jacob.e.keller@intel.com>]

On 12/1/2022 7:04 AM, Paolo Abeni wrote:
> 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
> 

Ah. Its probably a failed rebase. I re-ordered patches a bunch while 
trying to decide what order made the most sense. Yea we should move the 
hunk from the later patch into this one.

Thanks,
Jake