[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <IA3PR11MB898642CE8EA4BBA755E76812E53EA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Mon, 25 Aug 2025 10:58:40 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Kurt Kanzenbach <kurt@...utronix.de>, "Nguyen, Anthony L"
<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>
CC: Paul Menzel <pmenzel@...gen.mpg.de>, Vadim Fedorenko
<vadim.fedorenko@...ux.dev>, "Gomes, Vinicius" <vinicius.gomes@...el.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>, Richard Cochran
<richardcochran@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>, Eric Dumazet
<edumazet@...gle.com>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>, "Keller, Jacob E"
<jacob.e.keller@...el.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>, "Sebastian
Andrzej Siewior" <bigeasy@...utronix.de>
Subject: RE: [Intel-wired-lan] [PATCH iwl-next v2] igb: Convert Tx
timestamping to PTP aux worker
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf
> Of Kurt Kanzenbach
> Sent: Friday, August 22, 2025 9:28 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@...el.com>
> Cc: Paul Menzel <pmenzel@...gen.mpg.de>; Vadim Fedorenko
> <vadim.fedorenko@...ux.dev>; Gomes, Vinicius
> <vinicius.gomes@...el.com>; netdev@...r.kernel.org; Richard Cochran
> <richardcochran@...il.com>; Kurt Kanzenbach <kurt@...utronix.de>;
> Andrew Lunn <andrew+netdev@...n.ch>; Eric Dumazet
> <edumazet@...gle.com>; intel-wired-lan@...ts.osuosl.org; Keller, Jacob
> E <jacob.e.keller@...el.com>; Jakub Kicinski <kuba@...nel.org>; Paolo
> Abeni <pabeni@...hat.com>; David S. Miller <davem@...emloft.net>;
> Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2] igb: Convert Tx
> timestamping to PTP aux worker
>
> The current implementation uses schedule_work() which is executed by
> the system work queue to retrieve Tx timestamps. This increases
> latency and can lead to timeouts in case of heavy system load.
>
> Therefore, switch to the PTP aux worker which can be prioritized and
> pinned according to use case. Tested on Intel i210.
>
> Signed-off-by: Kurt Kanzenbach <kurt@...utronix.de>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> ---
> Changes in v2:
> - Switch from IRQ to PTP aux worker due to NTP performance regression
> (Miroslav)
> - Link to v1: https://lore.kernel.org/r/20250815-igb_irq_ts-v1-1-
> 8c6fc0353422@...utronix.de
> ---
> drivers/net/ethernet/intel/igb/igb.h | 1 -
> drivers/net/ethernet/intel/igb/igb_main.c | 6 +++---
> drivers/net/ethernet/intel/igb/igb_ptp.c | 28 +++++++++++++++--------
> -----
> 3 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb.h
> b/drivers/net/ethernet/intel/igb/igb.h
> index
> c3f4f7cd264e9b2ff70f03b580f95b15b528028c..f285def61f7a778f66944d6c52bb
> 31f11ff803cf 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -624,7 +624,6 @@ struct igb_adapter {
> struct ptp_clock *ptp_clock;
> struct ptp_clock_info ptp_caps;
> struct delayed_work ptp_overflow_work;
> - struct work_struct ptp_tx_work;
> struct sk_buff *ptp_tx_skb;
> struct kernel_hwtstamp_config tstamp_config;
> unsigned long ptp_tx_start;
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index
> a9a7a94ae61e93aa737b0103e00580e73601d62b..76467f0e53305188fcbbff27e21e
> 478e764ca552 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6576,7 +6576,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);
> + ptp_schedule_worker(adapter->ptp_clock, 0);
> } else {
> adapter->tx_hwtstamp_skipped++;
> }
> @@ -6612,7 +6612,7 @@ netdev_tx_t igb_xmit_frame_ring(struct sk_buff
> *skb,
> dev_kfree_skb_any(adapter->ptp_tx_skb);
> adapter->ptp_tx_skb = NULL;
> if (adapter->hw.mac.type == e1000_82576)
> - cancel_work_sync(&adapter->ptp_tx_work);
> + ptp_cancel_worker_sync(adapter->ptp_clock);
> clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter-
> >state);
> }
>
> @@ -7080,7 +7080,7 @@ static void igb_tsync_interrupt(struct
> igb_adapter *adapter)
>
> if (tsicr & E1000_TSICR_TXTS) {
> /* retrieve hardware timestamp */
> - schedule_work(&adapter->ptp_tx_work);
> + ptp_schedule_worker(adapter->ptp_clock, 0);
> }
>
> 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
> a7876882aeaf2b2a7fb9ec6ff5c83d8a1b06008a..8dabde01d09dcacc13e19fa4ce7a
> d0327077190a 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -798,20 +798,20 @@ static int igb_ptp_verify_pin(struct
> ptp_clock_info *ptp, unsigned int pin,
>
> /**
> * igb_ptp_tx_work
> - * @work: pointer to work struct
> + * @ptp: pointer to ptp clock information
> *
> * 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)
> +static long igb_ptp_tx_work(struct ptp_clock_info *ptp)
> {
> - struct igb_adapter *adapter = container_of(work, struct
> igb_adapter,
> - ptp_tx_work);
> + struct igb_adapter *adapter = container_of(ptp, struct
> igb_adapter,
> + ptp_caps);
> struct e1000_hw *hw = &adapter->hw;
> u32 tsynctxctl;
>
> if (!adapter->ptp_tx_skb)
> - return;
> + return -1;
>
> if (time_is_before_jiffies(adapter->ptp_tx_start +
> IGB_PTP_TX_TIMEOUT)) {
> @@ -824,15 +824,17 @@ static void igb_ptp_tx_work(struct work_struct
> *work)
> */
> rd32(E1000_TXSTMPH);
> dev_warn(&adapter->pdev->dev, "clearing Tx timestamp
> hang\n");
> - return;
> + return -1;
> }
>
> tsynctxctl = rd32(E1000_TSYNCTXCTL);
> - if (tsynctxctl & E1000_TSYNCTXCTL_VALID)
> + if (tsynctxctl & E1000_TSYNCTXCTL_VALID) {
> igb_ptp_tx_hwtstamp(adapter);
> - else
> - /* reschedule to check later */
> - schedule_work(&adapter->ptp_tx_work);
> + return -1;
> + }
> +
> + /* reschedule to check later */
> + return 1;
> }
>
> static void igb_ptp_overflow_check(struct work_struct *work) @@ -
> 915,7 +917,7 @@ void igb_ptp_tx_hang(struct igb_adapter *adapter)
> * timestamp bit when this occurs.
> */
> if (timeout) {
> - cancel_work_sync(&adapter->ptp_tx_work);
> + ptp_cancel_worker_sync(adapter->ptp_clock);
> dev_kfree_skb_any(adapter->ptp_tx_skb);
> adapter->ptp_tx_skb = NULL;
> clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter-
> >state); @@ -1381,6 +1383,7 @@ void igb_ptp_init(struct igb_adapter
> *adapter)
> return;
> }
>
> + adapter->ptp_caps.do_aux_work = igb_ptp_tx_work;
> adapter->ptp_clock = ptp_clock_register(&adapter->ptp_caps,
> &adapter->pdev->dev);
> if (IS_ERR(adapter->ptp_clock)) {
> @@ -1392,7 +1395,6 @@ void igb_ptp_init(struct igb_adapter *adapter)
> adapter->ptp_flags |= IGB_PTP_ENABLED;
>
> spin_lock_init(&adapter->tmreg_lock);
> - INIT_WORK(&adapter->ptp_tx_work, igb_ptp_tx_work);
>
> if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
> INIT_DELAYED_WORK(&adapter->ptp_overflow_work,
> @@ -1437,7 +1439,7 @@ void igb_ptp_suspend(struct igb_adapter
> *adapter)
> if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
> cancel_delayed_work_sync(&adapter->ptp_overflow_work);
>
> - cancel_work_sync(&adapter->ptp_tx_work);
> + ptp_cancel_worker_sync(adapter->ptp_clock);
> if (adapter->ptp_tx_skb) {
> dev_kfree_skb_any(adapter->ptp_tx_skb);
> adapter->ptp_tx_skb = NULL;
>
> ---
> base-commit: a7bd72158063740212344fad5d99dcef45bc70d6
> change-id: 20250813-igb_irq_ts-1aa77cc7b4cb
>
> Best regards,
> --
> Kurt Kanzenbach <kurt@...utronix.de>
Powered by blists - more mailing lists