Re: igc: missing HW timestamps at TX

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 6123 bytes --]

On Tue Aug 16 2022, Vinicius Costa Gomes wrote:
> Kurt Kanzenbach <kurt@linutronix.de> writes:
>
>> Hi Vinicius,
>>
>> On Mon Aug 15 2022, Vinicius Costa Gomes wrote:
>>> I think your question is more "why there's that workqueue on igc?"/"why
>>> don't you retrieve the TX timestamp 'inline' with the interrupt?", if I
>>> got that right, then, I don't have a good reason, apart from the feeling
>>> that reading all those (5-6?) registers may take too long for a
>>> interrupt handler. And it's something that's being done the same for
>>> most (all?) Intel drivers.
>>
>> We do have one optimization for igb which attempts to read the Tx
>> timestamp directly from the ISR. If that's not ready *only* then we
>> schedule the worker. I do assume igb and igc have the same logic for
>> retrieving the timestamps here.
>>
>
> That seems a sensible approach. And yes, the timestamping logic is the
> same.
>
>> The problem with workqueues is that under heavy system load, it might be
>> deferred and timestamps will be lost. I guess that workqueue was added
>> because of something like this: 1f6e8178d685 ("igb: Prevent dropped Tx
>> timestamps via work items and interrupts.").
>>
>>>
>>> I have a TODO to experiment with removing the workqueue, and retrieving
>>> the TX timestamp in the same context as the interrupt handler, but other
>>> things always come up.
>>
>> Let me know if you have interest in that igb patch.
>
> That would be great! Thanks.

Sure. See igb patch below.

I'm also wondering whether that delayed work should be replaced
completely by the PTP AUX worker, because that one can be prioritized in
accordance to the use case. And I see Vladimir already suggested this.

From c546621323ba325eccfcf6a9891681929d4b9b4f Mon Sep 17 00:00:00 2001
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Tue, 8 Jun 2021 15:38:43 +0200
Subject: [PATCH] igb: Try to tread the timestamp from the ISR.

Commit
   1f6e8178d6851 ("igb: Prevent dropped Tx timestamps via work items and interrupts.")

moved the read of the timestamp into a workqueue in order to retry the
read process in case the timestamp is not immediatly available after the
interrupt. The workqueue is scheduled immediatelly after sending a skb
on 82576 and for all other card types once the timestamp interrupt
occurs. Once scheduled, the workqueue will busy poll for the timestamp
until the operation times out.

By defferring the read of timestamp into a workqueue, the driver can
drop the timestamp of a following packet if the read does not occur
before the following packet is sent. This can happen if the system is
busy and the workqueue is delayed so that it is scheduled after another
skb.

Attempt the of the timestamp directly in the ISR and schedule a
workqueue in case the timestamp is not yet ready. Guard the read process
with __IGB_PTP_TX_READ_IN_PROGRESS to ensure that only one process
attempts to read the timestamp.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/intel/igb/igb.h      | 2 ++
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 drivers/net/ethernet/intel/igb/igb_ptp.c  | 7 +++++--
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 2d3daf022651..4c49550768ee 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -705,6 +705,7 @@ enum e1000_state_t {
 	__IGB_RESETTING,
 	__IGB_DOWN,
 	__IGB_PTP_TX_IN_PROGRESS,
+	__IGB_PTP_TX_READ_IN_PROGRESS,
 };
 
 enum igb_boards {
@@ -750,6 +751,7 @@ void igb_ptp_tx_hang(struct igb_adapter *adapter);
 void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector, struct sk_buff *skb);
 int igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, void *va,
 			ktime_t *timestamp);
+void igb_ptp_tx_work(struct work_struct *work);
 int igb_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
 int igb_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
 void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b88303351484..b74b305b5730 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6756,7 +6756,7 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
 
 	if (tsicr & E1000_TSICR_TXTS) {
 		/* retrieve hardware timestamp */
-		schedule_work(&adapter->ptp_tx_work);
+		igb_ptp_tx_work(&adapter->ptp_tx_work);
 		ack |= E1000_TSICR_TXTS;
 	}
 
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 0011b15e678c..97b444f84b83 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -680,7 +680,7 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
  * This work function polls the TSYNCTXCTL valid bit to determine when a
  * timestamp has been taken for the current stored skb.
  **/
-static void igb_ptp_tx_work(struct work_struct *work)
+void igb_ptp_tx_work(struct work_struct *work)
 {
 	struct igb_adapter *adapter = container_of(work, struct igb_adapter,
 						   ptp_tx_work);
@@ -705,7 +705,9 @@ static void igb_ptp_tx_work(struct work_struct *work)
 	}
 
 	tsynctxctl = rd32(E1000_TSYNCTXCTL);
-	if (tsynctxctl & E1000_TSYNCTXCTL_VALID)
+	if (tsynctxctl & E1000_TSYNCTXCTL_VALID &&
+	    !test_and_set_bit_lock(__IGB_PTP_TX_READ_IN_PROGRESS,
+				   &adapter->state))
 		igb_ptp_tx_hwtstamp(adapter);
 	else
 		/* reschedule to check later */
@@ -850,6 +852,7 @@ static void igb_ptp_tx_hwtstamp(struct igb_adapter *adapter)
 	 */
 	adapter->ptp_tx_skb = NULL;
 	clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
+	clear_bit_unlock(__IGB_PTP_TX_READ_IN_PROGRESS, &adapter->state);
 
 	/* Notify the stack and free the skb after we've unlocked */
 	skb_tstamp_tx(skb, &shhwtstamps);
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]