Dear Kurt,
Thank you for the patch.
Am 03.03.26 um 12:48 schrieb Kurt Kanzenbach:
Retrieve Tx timestamp from system BH instead of regular system workqueue.
The current implementation uses schedule_work() which is executed by the
system work queue and kworkers to retrieve Tx timestamps. This increases
latency and can lead to timeouts in case of heavy system load. i210 is
often used in industrial systems, where timestamp timeouts can be fatal.
Therefore, switch to the system BH workqueues which are executed directly
at the end of the IRQ handler.
Thank you for implementing this.
Tested on Intel i210 and i350 with ptp4l.
It would be great, if you shared the numbers. Did Miroslav already test
this?
Also, if you could add a comment/summary, that doing this in the IRQ
path with your theory on why, that’d be great.
Signed-off-by: Kurt Kanzenbach <[email protected]>
---
Changes in v4:
- Use BH workqueue (tasklet) instead of doing timestamping in IRQ path (Jakub
Kicinski)
- Link to v3:
https://patch.msgid.link/[email protected]
Changes in v3:
- Switch back to IRQ, but for i210 only
- Keep kworker for all other NICs like i350 (Miroslav)
- Link to v2:
https://lore.kernel.org/r/[email protected]
Changes in v2:
- Switch from IRQ to PTP aux worker due to NTP performance regression (Miroslav)
- Link to v1:
https://lore.kernel.org/r/[email protected]
---
drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
drivers/net/ethernet/intel/igb/igb_ptp.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
b/drivers/net/ethernet/intel/igb/igb_main.c
index ee99fd8fd513..9fd29fedb9f5 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6572,7 +6572,7 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff *skb,
adapter->ptp_tx_skb = skb_get(skb);
adapter->ptp_tx_start = jiffies;
if (adapter->hw.mac.type == e1000_82576)
- schedule_work(&adapter->ptp_tx_work);
+ queue_work(system_bh_wq, &adapter->ptp_tx_work);
} else {
adapter->tx_hwtstamp_skipped++;
}
@@ -7076,7 +7076,7 @@ static void igb_tsync_interrupt(struct igb_adapter
*adapter)
if (tsicr & E1000_TSICR_TXTS) {
/* retrieve hardware timestamp */
- schedule_work(&adapter->ptp_tx_work);
+ queue_work(system_bh_wq, &adapter->ptp_tx_work);
}
if (tsicr & TSINTR_TT0)
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c
b/drivers/net/ethernet/intel/igb/igb_ptp.c
index bd85d02ecadd..7b44f9090631 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -832,7 +832,7 @@ static void igb_ptp_tx_work(struct work_struct *work)
igb_ptp_tx_hwtstamp(adapter);
else
/* reschedule to check later */
- schedule_work(&adapter->ptp_tx_work);
+ queue_work(system_bh_wq, &adapter->ptp_tx_work);
}
static void igb_ptp_overflow_check(struct work_struct *work)
The diff looks fine.
Reviewed-by: Paul Menzel <[email protected]>
Kind regards,
Paul
