[PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Kurt Kanzenbach]

Retrieve Tx timestamp directly from interrupt handler.

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, fetch the timestamp directly from the interrupt handler.

The work queue code stays for the Intel 82576. Tested on Intel i210.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 drivers/net/ethernet/intel/igb/igb.h      |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c |  2 +-
 drivers/net/ethernet/intel/igb/igb_ptp.c  | 22 ++++++++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index c3f4f7cd264e9b2ff70f03b580f95b15b528028c..102ca32e8979fa3203fc2ea36eac456f1943cfca 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -776,6 +776,7 @@ int igb_ptp_hwtstamp_get(struct net_device *netdev,
 int igb_ptp_hwtstamp_set(struct net_device *netdev,
 			 struct kernel_hwtstamp_config *config,
 			 struct netlink_ext_ack *extack);
+void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter);
 void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
 unsigned int igb_get_max_rss_queues(struct igb_adapter *);
 #ifdef CONFIG_IGB_HWMON
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index a9a7a94ae61e93aa737b0103e00580e73601d62b..8ab6e52cb839bbb698007a74462798faaaab0071 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -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);
+		igb_ptp_tx_tstamp_event(adapter);
 	}
 
 	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..20ecafecc60557353f8cc5ab505030246687c8e4 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -796,6 +796,28 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
 	return 0;
 }
 
+/**
+ * igb_ptp_tx_tstamp_event
+ * @adapter: pointer to igb adapter
+ *
+ * This function checks the TSYNCTXCTL valid bit and stores the Tx hardware
+ * timestamp at the current skb.
+ **/
+void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u32 tsynctxctl;
+
+	if (!adapter->ptp_tx_skb)
+		return;
+
+	tsynctxctl = rd32(E1000_TSYNCTXCTL);
+	if (WARN_ON_ONCE(!(tsynctxctl & E1000_TSYNCTXCTL_VALID)))
+		return;
+
+	igb_ptp_tx_hwtstamp(adapter);
+}
+
 /**
  * igb_ptp_tx_work
  * @work: pointer to work struct

---
base-commit: 88250d40ed59d2b3c2dff788e9065caa7eb4dba0
change-id: 20250813-igb_irq_ts-1aa77cc7b4cb

Best regards,
-- 
Kurt Kanzenbach <kurt@linutronix.de>

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Paul Menzel]

Dear Kurt,


Thank you for your patch.

Am 15.08.25 um 08:50 schrieb Kurt Kanzenbach:
> Retrieve Tx timestamp directly from interrupt handler.
> 
> 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, fetch the timestamp directly from the interrupt handler.
> 
> The work queue code stays for the Intel 82576. Tested on Intel i210.

Excuse my ignorance, I do not understand the first sentence in the last 
line. Is it because the driver support different models? Why not change 
it for Intel 82576 too?

Do you have a reproducer for the issue, so others can test.

> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   drivers/net/ethernet/intel/igb/igb.h      |  1 +
>   drivers/net/ethernet/intel/igb/igb_main.c |  2 +-
>   drivers/net/ethernet/intel/igb/igb_ptp.c  | 22 ++++++++++++++++++++++
>   3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index c3f4f7cd264e9b2ff70f03b580f95b15b528028c..102ca32e8979fa3203fc2ea36eac456f1943cfca 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -776,6 +776,7 @@ int igb_ptp_hwtstamp_get(struct net_device *netdev,
>   int igb_ptp_hwtstamp_set(struct net_device *netdev,
>   			 struct kernel_hwtstamp_config *config,
>   			 struct netlink_ext_ack *extack);
> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter);
>   void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
>   unsigned int igb_get_max_rss_queues(struct igb_adapter *);
>   #ifdef CONFIG_IGB_HWMON
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index a9a7a94ae61e93aa737b0103e00580e73601d62b..8ab6e52cb839bbb698007a74462798faaaab0071 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -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);
> +		igb_ptp_tx_tstamp_event(adapter);
>   	}
>   
>   	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..20ecafecc60557353f8cc5ab505030246687c8e4 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -796,6 +796,28 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
>   	return 0;
>   }
>   
> +/**
> + * igb_ptp_tx_tstamp_event
> + * @adapter: pointer to igb adapter
> + *
> + * This function checks the TSYNCTXCTL valid bit and stores the Tx hardware
> + * timestamp at the current skb.
> + **/
> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter)
> +{
> +	struct e1000_hw *hw = &adapter->hw;
> +	u32 tsynctxctl;
> +
> +	if (!adapter->ptp_tx_skb)
> +		return;
> +
> +	tsynctxctl = rd32(E1000_TSYNCTXCTL);
> +	if (WARN_ON_ONCE(!(tsynctxctl & E1000_TSYNCTXCTL_VALID)))
> +		return;
> +
> +	igb_ptp_tx_hwtstamp(adapter);
> +}
> +
>   /**
>    * igb_ptp_tx_work
>    * @work: pointer to work struct

The diff looks fine.

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Sebastian Andrzej Siewior]

On 2025-08-15 09:55:00 [+0200], Paul Menzel wrote:
> > Therefore, fetch the timestamp directly from the interrupt handler.
> > 
> > The work queue code stays for the Intel 82576. Tested on Intel i210.
> 
> Excuse my ignorance, I do not understand the first sentence in the last
> line. Is it because the driver support different models? Why not change it
> for Intel 82576 too?

The 82576 does not have an interrupt event for this as far as I
remember.

> Do you have a reproducer for the issue, so others can test.

The issue is that the workqueue can be delayed and ptp starts
complaining. If the timestamp can be retrieved directly, there is no
reason for the delay.

Sebastian

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Kurt Kanzenbach]

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

Hi Paul,

On Fri Aug 15 2025, Paul Menzel wrote:
> Dear Kurt,
>
>
> Thank you for your patch.
>
> Am 15.08.25 um 08:50 schrieb Kurt Kanzenbach:
>> Retrieve Tx timestamp directly from interrupt handler.
>> 
>> 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, fetch the timestamp directly from the interrupt handler.
>> 
>> The work queue code stays for the Intel 82576. Tested on Intel i210.
>
> Excuse my ignorance, I do not understand the first sentence in the last 
> line. Is it because the driver support different models? Why not change 
> it for Intel 82576 too?

Yes, the driver supports lots of different NIC(s). AFAICS Intel 82576 is
the only one which does not use time sync interrupts. Probably it does
not have this feature. Therefore, the 82576 needs to schedule a work
queue item.

>
> Do you have a reproducer for the issue, so others can test.

Yeah, I do have a reproducer:

 - Run ptp4l with 40ms tx timeout (--tx_timestamp_timeout)
 - Run periodic RT tasks (e.g. with SCHED_FIFO 1) with run time of
   50-100ms per CPU core

This leads to sporadic error messages from ptp4l such as "increasing
tx_timestamp_timeout or increasing kworker priority may correct this
issue, but a driver bug likely causes it"

However, increasing the kworker priority is not an option, simply
because this kworker is doing non-related PTP work items as well.

As the time sync interrupt already signals that the Tx timestamp is
available, there's no need to schedule a work item in this case. I might
have missed something though. But my testing looked good. The warn_on
never triggered.

>
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>>   drivers/net/ethernet/intel/igb/igb.h      |  1 +
>>   drivers/net/ethernet/intel/igb/igb_main.c |  2 +-
>>   drivers/net/ethernet/intel/igb/igb_ptp.c  | 22 ++++++++++++++++++++++
>>   3 files changed, 24 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
>> index c3f4f7cd264e9b2ff70f03b580f95b15b528028c..102ca32e8979fa3203fc2ea36eac456f1943cfca 100644
>> --- a/drivers/net/ethernet/intel/igb/igb.h
>> +++ b/drivers/net/ethernet/intel/igb/igb.h
>> @@ -776,6 +776,7 @@ int igb_ptp_hwtstamp_get(struct net_device *netdev,
>>   int igb_ptp_hwtstamp_set(struct net_device *netdev,
>>   			 struct kernel_hwtstamp_config *config,
>>   			 struct netlink_ext_ack *extack);
>> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter);
>>   void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
>>   unsigned int igb_get_max_rss_queues(struct igb_adapter *);
>>   #ifdef CONFIG_IGB_HWMON
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index a9a7a94ae61e93aa737b0103e00580e73601d62b..8ab6e52cb839bbb698007a74462798faaaab0071 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -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);
>> +		igb_ptp_tx_tstamp_event(adapter);
>>   	}
>>   
>>   	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..20ecafecc60557353f8cc5ab505030246687c8e4 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
>> @@ -796,6 +796,28 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
>>   	return 0;
>>   }
>>   
>> +/**
>> + * igb_ptp_tx_tstamp_event
>> + * @adapter: pointer to igb adapter
>> + *
>> + * This function checks the TSYNCTXCTL valid bit and stores the Tx hardware
>> + * timestamp at the current skb.
>> + **/
>> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter)
>> +{
>> +	struct e1000_hw *hw = &adapter->hw;
>> +	u32 tsynctxctl;
>> +
>> +	if (!adapter->ptp_tx_skb)
>> +		return;
>> +
>> +	tsynctxctl = rd32(E1000_TSYNCTXCTL);
>> +	if (WARN_ON_ONCE(!(tsynctxctl & E1000_TSYNCTXCTL_VALID)))
>> +		return;
>> +
>> +	igb_ptp_tx_hwtstamp(adapter);
>> +}
>> +
>>   /**
>>    * igb_ptp_tx_work
>>    * @work: pointer to work struct
>
> The diff looks fine.
>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Thanks!

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

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Paul Menzel]

Dear Kurt, dear Sebastian,


Thank you for your replies.


Am 15.08.25 um 10:17 schrieb Kurt Kanzenbach:

> On Fri Aug 15 2025, Paul Menzel wrote:

>> Am 15.08.25 um 08:50 schrieb Kurt Kanzenbach:
>>> Retrieve Tx timestamp directly from interrupt handler.
>>>
>>> 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, fetch the timestamp directly from the interrupt handler.
>>>
>>> The work queue code stays for the Intel 82576. Tested on Intel i210.
>>
>> Excuse my ignorance, I do not understand the first sentence in the last
>> line. Is it because the driver support different models? Why not change
>> it for Intel 82576 too?
> 
> Yes, the driver supports lots of different NIC(s). AFAICS Intel 82576 is
> the only one which does not use time sync interrupts. Probably it does
> not have this feature. Therefore, the 82576 needs to schedule a work
> queue item.

Should you resend, it’d be great if you mentioned that. (Sebastian 
confirmed it.)

>> Do you have a reproducer for the issue, so others can test.
> 
> Yeah, I do have a reproducer:
> 
>   - Run ptp4l with 40ms tx timeout (--tx_timestamp_timeout)
>   - Run periodic RT tasks (e.g. with SCHED_FIFO 1) with run time of
>     50-100ms per CPU core
> 
> This leads to sporadic error messages from ptp4l such as "increasing
> tx_timestamp_timeout or increasing kworker priority may correct this
> issue, but a driver bug likely causes it"
> 
> However, increasing the kworker priority is not an option, simply
> because this kworker is doing non-related PTP work items as well.
> 
> As the time sync interrupt already signals that the Tx timestamp is
> available, there's no need to schedule a work item in this case. I might
> have missed something though. But my testing looked good. The warn_on
> never triggered.

Great. Maybe add that too, as, at least for me, realtime stuff is 
something I do not do, so pointers would help me.

>>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>>> ---
>>>    drivers/net/ethernet/intel/igb/igb.h      |  1 +
>>>    drivers/net/ethernet/intel/igb/igb_main.c |  2 +-
>>>    drivers/net/ethernet/intel/igb/igb_ptp.c  | 22 ++++++++++++++++++++++
>>>    3 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
>>> index c3f4f7cd264e9b2ff70f03b580f95b15b528028c..102ca32e8979fa3203fc2ea36eac456f1943cfca 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb.h
>>> +++ b/drivers/net/ethernet/intel/igb/igb.h
>>> @@ -776,6 +776,7 @@ int igb_ptp_hwtstamp_get(struct net_device *netdev,
>>>    int igb_ptp_hwtstamp_set(struct net_device *netdev,
>>>    			 struct kernel_hwtstamp_config *config,
>>>    			 struct netlink_ext_ack *extack);
>>> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter);
>>>    void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
>>>    unsigned int igb_get_max_rss_queues(struct igb_adapter *);
>>>    #ifdef CONFIG_IGB_HWMON
>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>>> index a9a7a94ae61e93aa737b0103e00580e73601d62b..8ab6e52cb839bbb698007a74462798faaaab0071 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>> @@ -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);
>>> +		igb_ptp_tx_tstamp_event(adapter);
>>>    	}
>>>    
>>>    	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..20ecafecc60557353f8cc5ab505030246687c8e4 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
>>> @@ -796,6 +796,28 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
>>>    	return 0;
>>>    }
>>>    
>>> +/**
>>> + * igb_ptp_tx_tstamp_event
>>> + * @adapter: pointer to igb adapter
>>> + *
>>> + * This function checks the TSYNCTXCTL valid bit and stores the Tx hardware
>>> + * timestamp at the current skb.
>>> + **/
>>> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter)
>>> +{
>>> +	struct e1000_hw *hw = &adapter->hw;
>>> +	u32 tsynctxctl;
>>> +
>>> +	if (!adapter->ptp_tx_skb)
>>> +		return;
>>> +
>>> +	tsynctxctl = rd32(E1000_TSYNCTXCTL);
>>> +	if (WARN_ON_ONCE(!(tsynctxctl & E1000_TSYNCTXCTL_VALID)))
>>> +		return;
>>> +
>>> +	igb_ptp_tx_hwtstamp(adapter);
>>> +}
>>> +
>>>    /**
>>>     * igb_ptp_tx_work
>>>     * @work: pointer to work struct
>>
>> The diff looks fine.
>>
>> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Vadim Fedorenko]

On 15/08/2025 09:17, Kurt Kanzenbach wrote:
> Hi Paul,
> 
> On Fri Aug 15 2025, Paul Menzel wrote:
>> Dear Kurt,
>>
>>
>> Thank you for your patch.
>>
>> Am 15.08.25 um 08:50 schrieb Kurt Kanzenbach:
>>> Retrieve Tx timestamp directly from interrupt handler.
>>>
>>> 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, fetch the timestamp directly from the interrupt handler.
>>>
>>> The work queue code stays for the Intel 82576. Tested on Intel i210.
>>
>> Excuse my ignorance, I do not understand the first sentence in the last
>> line. Is it because the driver support different models? Why not change
>> it for Intel 82576 too?
> 
> Yes, the driver supports lots of different NIC(s). AFAICS Intel 82576 is
> the only one which does not use time sync interrupts. Probably it does
> not have this feature. Therefore, the 82576 needs to schedule a work
> queue item.
> 
>>
>> Do you have a reproducer for the issue, so others can test.
> 
> Yeah, I do have a reproducer:
> 
>   - Run ptp4l with 40ms tx timeout (--tx_timestamp_timeout)
>   - Run periodic RT tasks (e.g. with SCHED_FIFO 1) with run time of
>     50-100ms per CPU core
> 
> This leads to sporadic error messages from ptp4l such as "increasing
> tx_timestamp_timeout or increasing kworker priority may correct this
> issue, but a driver bug likely causes it"
> 
> However, increasing the kworker priority is not an option, simply
> because this kworker is doing non-related PTP work items as well.

Well, in this case, as it pointed out for other drivers, the best
practice would be to use a dedicated PTP worker which does only PTP
related tasks and can have higher priority.

The inline retrieving of timestamp, of course, the best option, but for
82576 could you please consider using @do_aux_work in ptp_caps and do
proper ptp_schedule_worker()?

> 
> As the time sync interrupt already signals that the Tx timestamp is
> available, there's no need to schedule a work item in this case. I might
> have missed something though. But my testing looked good. The warn_on
> never triggered.
> 
>>
>>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>>> ---
>>>    drivers/net/ethernet/intel/igb/igb.h      |  1 +
>>>    drivers/net/ethernet/intel/igb/igb_main.c |  2 +-
>>>    drivers/net/ethernet/intel/igb/igb_ptp.c  | 22 ++++++++++++++++++++++
>>>    3 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
>>> index c3f4f7cd264e9b2ff70f03b580f95b15b528028c..102ca32e8979fa3203fc2ea36eac456f1943cfca 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb.h
>>> +++ b/drivers/net/ethernet/intel/igb/igb.h
>>> @@ -776,6 +776,7 @@ int igb_ptp_hwtstamp_get(struct net_device *netdev,
>>>    int igb_ptp_hwtstamp_set(struct net_device *netdev,
>>>    			 struct kernel_hwtstamp_config *config,
>>>    			 struct netlink_ext_ack *extack);
>>> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter);
>>>    void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
>>>    unsigned int igb_get_max_rss_queues(struct igb_adapter *);
>>>    #ifdef CONFIG_IGB_HWMON
>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>>> index a9a7a94ae61e93aa737b0103e00580e73601d62b..8ab6e52cb839bbb698007a74462798faaaab0071 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>> @@ -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);
>>> +		igb_ptp_tx_tstamp_event(adapter);
>>>    	}
>>>    
>>>    	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..20ecafecc60557353f8cc5ab505030246687c8e4 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
>>> @@ -796,6 +796,28 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
>>>    	return 0;
>>>    }
>>>    
>>> +/**
>>> + * igb_ptp_tx_tstamp_event
>>> + * @adapter: pointer to igb adapter
>>> + *
>>> + * This function checks the TSYNCTXCTL valid bit and stores the Tx hardware
>>> + * timestamp at the current skb.
>>> + **/
>>> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter)
>>> +{
>>> +	struct e1000_hw *hw = &adapter->hw;
>>> +	u32 tsynctxctl;
>>> +
>>> +	if (!adapter->ptp_tx_skb)
>>> +		return;
>>> +
>>> +	tsynctxctl = rd32(E1000_TSYNCTXCTL);
>>> +	if (WARN_ON_ONCE(!(tsynctxctl & E1000_TSYNCTXCTL_VALID)))
>>> +		return;
>>> +
>>> +	igb_ptp_tx_hwtstamp(adapter);
>>> +}
>>> +
>>>    /**
>>>     * igb_ptp_tx_work
>>>     * @work: pointer to work struct
>>
>> The diff looks fine.
>>
>> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Sebastian Andrzej Siewior]

On 2025-08-15 14:54:16 [+0200], Paul Menzel wrote:
> > > Do you have a reproducer for the issue, so others can test.
> > 
> > Yeah, I do have a reproducer:
> > 
> >   - Run ptp4l with 40ms tx timeout (--tx_timestamp_timeout)
> >   - Run periodic RT tasks (e.g. with SCHED_FIFO 1) with run time of
> >     50-100ms per CPU core
> > 
> > This leads to sporadic error messages from ptp4l such as "increasing
> > tx_timestamp_timeout or increasing kworker priority may correct this
> > issue, but a driver bug likely causes it"
> > 
> > However, increasing the kworker priority is not an option, simply
> > because this kworker is doing non-related PTP work items as well.
> > 
> > As the time sync interrupt already signals that the Tx timestamp is
> > available, there's no need to schedule a work item in this case. I might
> > have missed something though. But my testing looked good. The warn_on
> > never triggered.
> 
> Great. Maybe add that too, as, at least for me, realtime stuff is something
> I do not do, so pointers would help me.

If you ask for an explicit reproducer that you would have to have task
that does
	struct sched_param param = { .sched_priority = 1 };
	sched_setscheduler(0, SCHED_FIFO, &param);

and let it run for to exceed the ptp4l timeout. But in general it does
not require a real time task to trigger the problem. A busy CPU with a
lot of pending worker will do the trick, too. You just need "enough" CPU
work so the scheduler puts other tasks on the CPU before the kworker,
which retrieves the timestamp, is scheduled.

> Paul

Sebastian

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Kurt Kanzenbach]

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

On Fri Aug 15 2025, Vadim Fedorenko wrote:
> On 15/08/2025 09:17, Kurt Kanzenbach wrote:
>> Hi Paul,
>> 
>> On Fri Aug 15 2025, Paul Menzel wrote:
>>> Dear Kurt,
>>>
>>>
>>> Thank you for your patch.
>>>
>>> Am 15.08.25 um 08:50 schrieb Kurt Kanzenbach:
>>>> Retrieve Tx timestamp directly from interrupt handler.
>>>>
>>>> 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, fetch the timestamp directly from the interrupt handler.
>>>>
>>>> The work queue code stays for the Intel 82576. Tested on Intel i210.
>>>
>>> Excuse my ignorance, I do not understand the first sentence in the last
>>> line. Is it because the driver support different models? Why not change
>>> it for Intel 82576 too?
>> 
>> Yes, the driver supports lots of different NIC(s). AFAICS Intel 82576 is
>> the only one which does not use time sync interrupts. Probably it does
>> not have this feature. Therefore, the 82576 needs to schedule a work
>> queue item.
>> 
>>>
>>> Do you have a reproducer for the issue, so others can test.
>> 
>> Yeah, I do have a reproducer:
>> 
>>   - Run ptp4l with 40ms tx timeout (--tx_timestamp_timeout)
>>   - Run periodic RT tasks (e.g. with SCHED_FIFO 1) with run time of
>>     50-100ms per CPU core
>> 
>> This leads to sporadic error messages from ptp4l such as "increasing
>> tx_timestamp_timeout or increasing kworker priority may correct this
>> issue, but a driver bug likely causes it"
>> 
>> However, increasing the kworker priority is not an option, simply
>> because this kworker is doing non-related PTP work items as well.
>
> Well, in this case, as it pointed out for other drivers, the best
> practice would be to use a dedicated PTP worker which does only PTP
> related tasks and can have higher priority.
>
> The inline retrieving of timestamp, of course, the best option, but for
> 82576 could you please consider using @do_aux_work in ptp_caps and do
> proper ptp_schedule_worker()?

Sure, using the PTP aux worker is always the better than using the
system work queue. I ordered a 82576 for testing. But, that conversion
will be another patch.

>
>> 
>> As the time sync interrupt already signals that the Tx timestamp is
>> available, there's no need to schedule a work item in this case. I might
>> have missed something though. But my testing looked good. The warn_on
>> never triggered.
>> 
>>>
>>>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>>>> ---
>>>>    drivers/net/ethernet/intel/igb/igb.h      |  1 +
>>>>    drivers/net/ethernet/intel/igb/igb_main.c |  2 +-
>>>>    drivers/net/ethernet/intel/igb/igb_ptp.c  | 22 ++++++++++++++++++++++
>>>>    3 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
>>>> index c3f4f7cd264e9b2ff70f03b580f95b15b528028c..102ca32e8979fa3203fc2ea36eac456f1943cfca 100644
>>>> --- a/drivers/net/ethernet/intel/igb/igb.h
>>>> +++ b/drivers/net/ethernet/intel/igb/igb.h
>>>> @@ -776,6 +776,7 @@ int igb_ptp_hwtstamp_get(struct net_device *netdev,
>>>>    int igb_ptp_hwtstamp_set(struct net_device *netdev,
>>>>    			 struct kernel_hwtstamp_config *config,
>>>>    			 struct netlink_ext_ack *extack);
>>>> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter);
>>>>    void igb_set_flag_queue_pairs(struct igb_adapter *, const u32);
>>>>    unsigned int igb_get_max_rss_queues(struct igb_adapter *);
>>>>    #ifdef CONFIG_IGB_HWMON
>>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>>>> index a9a7a94ae61e93aa737b0103e00580e73601d62b..8ab6e52cb839bbb698007a74462798faaaab0071 100644
>>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>>>> @@ -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);
>>>> +		igb_ptp_tx_tstamp_event(adapter);
>>>>    	}
>>>>    
>>>>    	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..20ecafecc60557353f8cc5ab505030246687c8e4 100644
>>>> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
>>>> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
>>>> @@ -796,6 +796,28 @@ static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +/**
>>>> + * igb_ptp_tx_tstamp_event
>>>> + * @adapter: pointer to igb adapter
>>>> + *
>>>> + * This function checks the TSYNCTXCTL valid bit and stores the Tx hardware
>>>> + * timestamp at the current skb.
>>>> + **/
>>>> +void igb_ptp_tx_tstamp_event(struct igb_adapter *adapter)
>>>> +{
>>>> +	struct e1000_hw *hw = &adapter->hw;
>>>> +	u32 tsynctxctl;
>>>> +
>>>> +	if (!adapter->ptp_tx_skb)
>>>> +		return;
>>>> +
>>>> +	tsynctxctl = rd32(E1000_TSYNCTXCTL);
>>>> +	if (WARN_ON_ONCE(!(tsynctxctl & E1000_TSYNCTXCTL_VALID)))
>>>> +		return;
>>>> +
>>>> +	igb_ptp_tx_hwtstamp(adapter);
>>>> +}
>>>> +
>>>>    /**
>>>>     * igb_ptp_tx_work
>>>>     * @work: pointer to work struct
>>>
>>> The diff looks fine.
>>>
>>> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
>
> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Thanks!
Kurt

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

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Miroslav Lichvar]

On Fri, Aug 15, 2025 at 08:50:23AM +0200, Kurt Kanzenbach wrote:
> Retrieve Tx timestamp directly from interrupt handler.
> 
> 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, fetch the timestamp directly from the interrupt handler.
> 
> The work queue code stays for the Intel 82576. Tested on Intel i210.

I tested this patch on 6.17-rc1 with an Intel I350 card on a NTP
server (chrony 4.4), measuring packet rates and TX timestamp accuracy
with ntpperf. While the HW TX timestamping seems more reliable at some
lower request rates, there seems to be about 40% drop in the overall
performance of the server in how much requests it can handle (falling
back to SW timestamps when HW timestamp is missed). Is this expected
or something to be considered? 

Before:
               |          responses            |     TX timestamp offset (ns)
rate   clients |  lost invalid   basic  xleave |    min    mean     max stddev
1000       100   0.00%   0.00%   0.00% 100.00%      -77     -42      +1     14
1050       105   0.00%   0.00%   0.00% 100.00%      -68     -35      +2     13
1102       110   0.00%   0.00%   0.00% 100.00%      -60     -14     +27     15
1157       115   0.00%   0.00%   0.00% 100.00%      -28     +12     +53     14
1214       121   0.00%   0.00%   0.00% 100.00%       -3     +34     +68     13
1274       127   0.00%   0.00%   0.00% 100.00%       -5     +35     +70     14
1337       133   0.00%   0.00%   0.00% 100.00%       -2     +30     +64     13
1403       140   0.00%   0.00%   0.00% 100.00%      -27     +11     +57     14
1473       147   0.00%   0.00%   0.00% 100.00%      -15     +27     +65     15
1546       154   0.00%   0.00%   0.00% 100.00%      -56      -8     +41     18
1623       162   0.00%   0.00%   0.00% 100.00%      -69     -27     +28     17
1704       170   0.00%   0.00%   0.00% 100.00%      -39      -5     +30     13
1789       178   0.00%   0.00%   0.00% 100.00%      -32      +3     +37     13
1878       187   0.00%   0.00%   0.00% 100.00%      -22     +20     +75     18
1971       197   0.00%   0.00%   0.00% 100.00%      -27      +6     +40     12
2069       206   0.00%   0.00%   0.00% 100.00%      -22     +19     +62     15
2172       217   0.00%   0.00%   0.00% 100.00%      +16     +53     +92     13
2280       228   0.00%   0.00%   0.00% 100.00%      -30     +55     +92     18
2394       239   0.00%   0.00%   0.00% 100.00%      -61      -5     +33     18
2513       251   0.00%   0.00%   0.00% 100.00%      -66     -23     +20     14
2638       263   0.00%   0.00%   0.00% 100.00%      -34      -1     +32     12
2769       276   0.00%   0.00%   0.00% 100.00%      -20     +20     +60     14
2907       290   0.00%   0.00%   0.00% 100.00%      -35      +6     +68     20
3052       305   0.00%   0.00%   0.00% 100.00%      -29      +5     +40     12
3204       320   0.00%   0.00%   0.00% 100.00%      -25      +8     +42     12
3364       336   0.00%   0.00%   0.00% 100.00%      -12     +31     +70     14
3532       353   0.00%   0.00%   0.00% 100.00%      -12     +31     +88     17
3708       370   0.00%   0.00%   0.00% 100.00%       +4     +40     +80     13
3893       389   0.00%   0.00%   0.00% 100.00%      -47     +42     +93     28
4087       408   0.00%   0.00%   0.00% 100.00%      -63     -25     +33     18
4291       429   0.00%   0.00%   0.00% 100.00%      -57      -8     +37     15
4505       450   0.00%   0.00%   0.00% 100.00%       -8     +27     +64     13
4730       473   0.00%   0.00%   0.00% 100.00%       +5     +44     +81     13
4966       496   0.00%   0.00%   0.00% 100.00%      -30     +14  +11670    119
5214       521   0.00%   0.00%   0.00% 100.00%      -36      +2  +18077    251
5474       547   0.00%   0.00%   0.00% 100.00%      -30     +14  +16652    160
5747       574   0.00%   0.00%   0.00% 100.00%      -15     +37  +18081    292
6034       603   0.00%   0.00%   0.00% 100.00%       +8     +51  +16561    151
6335       633   0.00%   0.00%   0.00% 100.00%      -10     +28  +18073    161
6651       665   0.00%   0.00%   0.00% 100.00%      -23     +23  +16349    158
6983       698   0.00%   0.00%   0.00% 100.00%      -26     +26     +78     19
7332       733   0.00%   0.00%   0.00% 100.00%      -50     -10     +30     13
7698       769   0.00%   0.00%   0.00% 100.00%       +5     +45  +16397    148
8082       808   0.00%   0.00%   0.00% 100.00%      +17     +60  +18089    284
8486       848   0.00%   0.00%   0.00% 100.00%      -46     +30  +18197    195
8910       891   0.00%   0.00%   0.00% 100.00%      -48     -13  +16493    127
9355       935   0.00%   0.00%   0.00% 100.00%      -34     +15  +18049    225
9822       982   0.00%   0.00%   0.00% 100.00%       -4     +33  +16663    148
10313     1031   0.00%   0.00%   0.00% 100.00%      -31     +22  +15046    110
10828     1082   0.00%   0.00%   0.00% 100.00%      -14     +30  +18201    214
11369     1136   0.00%   0.00%   0.00% 100.00%       +0     +44  +16464    171
11937     1193   0.00%   0.00%   0.00% 100.00%       -7     +55  +18101    346
12533     1253   0.00%   0.00%   0.00% 100.00%      -21     +22  +18113    372
13159     1315   0.00%   0.00%   0.00% 100.00%      -18     +22  +18068    263
13816     1381   0.00%   0.00%   0.00% 100.00%      -33     +17  +18032    171
14506     1450   0.00%   0.00%   0.00% 100.00%      -49      -4  +18040    218
15231     1523   0.00%   0.00%   0.00% 100.00%      -40      -1  +18171    224
15992     1599   0.00%   0.00%   0.00% 100.00%      -24     +17  +18106    210
16791     1679   0.00%   0.00%   0.00% 100.00%       +2     +45  +17998    219
17630     1763   0.00%   0.00%   0.00% 100.00%      -78     -26  +16597    214
18511     1851   0.00%   0.00%   0.00% 100.00%      -40      +5  +18087    238
19436     1943   0.00%   0.00%   0.00% 100.00%      -33     +27  +18110    316
20407     2040   0.00%   0.00%   0.00% 100.00%       -4     +57  +18132    359
21427     2142   0.00%   0.00%   0.00% 100.00%      -21     +39  +18131    402
22498     2249   0.00%   0.00%   0.00% 100.00%      -53     +50  +18109    478
23622     2362   0.00%   0.00%   0.00% 100.00%      -23    +109  +18153    718
24803     2480   0.00%   0.00%   0.00% 100.00%      -13    +680  +18113   2227
26043     2604   0.00%   0.00%   0.00% 100.00%     -248   +4060  +18732   6280
27345     2734   0.00%   0.00%   0.00% 100.00%      -35   +1139  +18072   2774
28712     2871   0.00%   0.00%   0.00% 100.00%       +9   +1338  +18153   2828
30147     3014   0.00%   0.00%   0.00% 100.00%      +29   +1481  +17354   3083
31654     3165   0.00%   0.00%   0.00% 100.00%      -37   +1609  +18032   3405
33236     3323   0.00%   0.00%   0.00% 100.00%      -97   +2092  +18050   4248
34897     3489   0.00%   0.00%   0.00% 100.00%     -122   +2537  +18113   4933
36641     3664   0.00%   0.00%   0.00% 100.00%     -117   +3943  +18433   5985
38473     3847   0.00%   0.00%   0.00% 100.00%     -178   +4307  +18708   6488
40396     4039   0.00%   0.00%   0.00% 100.00%      +61   +3570  +18272   5927
42415     4241   0.00%   0.00%   0.00% 100.00%     -171   +5080  +18745   6633
44535     4453   0.00%   0.00%   0.00% 100.00%     -620   +3489  +18162   5717
46761     4676   0.00%   0.00%   0.00% 100.00%     -667   +2999  +18110   5335
49099     4909   0.00%   0.00%   0.00% 100.00%      +28   +2867  +18219   5023
51553     5155   0.00%   0.00%   0.00% 100.00%      +64   +2241  +18035   4339
54130     5413   0.00%   0.00%   0.00% 100.00%      +11   +1873  +18097   3915
56836     5683   0.00%   0.00%   0.00% 100.00%      -17   +1748  +17564   3828
59677     5967   0.00%   0.00%   0.00% 100.00%      -13   +1786  +17458   3800
62660     6266   0.00%   0.00%   0.00% 100.00%       -7   +1932  +18122   3905
65793     6579   0.00%   0.00%   0.00% 100.00%      -29   +1831  +17546   3731
69082     6908   0.00%   0.00%   0.06%  99.94%     -152   +2825  +18696   4125
72536     7253   0.00%   0.00%   0.00% 100.00%     +233   +2128  +18188   3404
76162     7616   0.00%   0.00%   0.00% 100.00%      +61   +2279  +18109   3774
79970     7997   0.00%   0.00%   0.00% 100.00%      -26   +2256  +18108   3819
83968     8396   0.00%   0.00%   0.00% 100.00%      -74   +1886  +17372   3276
88166     8816   0.00%   0.00%   0.05%  99.95%      -77   +1570  +17911   2158
92574     9257   0.00%   0.00%   0.02%  99.98%     -151   +1368  +18671   1848
97202     9720   0.00%   0.00%   0.00% 100.00%      +40   +1418  +17280   2003
102062   10206   0.00%   0.00%   0.00% 100.00%      +70   +1738  +17183   2442
107165   10716   0.00%   0.00%   0.00% 100.00%       +3   +1784  +17451   2487
112523   11252   0.00%   0.00%   0.00% 100.00%      -29   +1459  +16843   1623
118149   11814   0.00%   0.00%   0.00% 100.00%      -14   +1438  +16975   1516
124056   12405   0.00%   0.00%   0.00% 100.00%       -8   +1639  +17178   2050
130258   13025   0.00%   0.00%   0.00% 100.00%      -39   +1607  +17312   1735
136770   13677   0.00%   0.00%   0.00% 100.00%      -49   +1546  +18063   1553
143608   14360   0.00%   0.00%   0.00% 100.00%      -28   +1602  +17486   1510
150788   15078   0.00%   0.00%   0.00% 100.00%       -3   +1540  +16739   1364
158327   15832   0.05%   0.00%   0.16%  99.79%     -108   +2111  +18784   1405
166243   16384   0.17%   0.00%   0.00%  99.83%      +67   +1766  +18211   1235
174555   16384   0.00%   0.00%   0.00% 100.00%      +50   +1702  +16643   1165
183282   16384   0.00%   0.00%   0.00% 100.00%      -12   +1672  +16543   1196
192446   16384   0.00%   0.00%   0.00% 100.00%      -28   +1697  +16716   1197
202068   16384   0.00%   0.00%   0.00% 100.00%      -59   +1692  +16631   1179
212171   16384   0.00%   0.00%   0.00% 100.00%      -49   +1767  +16621   1179
222779   16384   0.16%   0.00%   0.07%  99.77%      -44   +1820  +16643   1147
233917   16384   3.60%   0.00%   0.20%  96.20%     -131   +1680  +31346   1089
245612   16384   7.58%   0.00%   0.32%  92.10%     -149   +2102  +86219   1316
257892   16384  11.15%   0.00%   0.40%  88.45%      +14   +2265  +21315   1351
270786   16384  15.75%   0.00%   0.50%  83.75%       +1   +2338  +18161   1394
284325   16384  19.72%   0.00%   0.60%  79.68%      -52   +2387  +40370   1437
298541   16384  22.83%   0.00%   0.66%  76.51%      -61   +2461  +21356   1467
313468   16384  26.26%   0.00%   0.70%  73.04%      -38   +2495  +24987   1466
329141   16384  29.02%   0.00%   0.75%  70.24%      -24   +2598  +25469   1484
345598   16384  32.21%   0.00%   0.78%  67.02%       -1   +2636  +24568   1489
362877   16384  35.26%   0.00%   0.80%  63.94%      -38   +2654  +21299   1495
381020   16384  38.66%   0.00%   0.96%  60.38%      -85   +2392  +20972   1415
400071   16384  41.41%   0.00%   1.02%  57.57%     -524   +2323  +20943   1508
420074   16384  43.53%   0.00%   1.09%  55.38%     -656   +2464  +68387   1564

After:
               |          responses            |     TX timestamp offset (ns)
rate   clients |  lost invalid   basic  xleave |    min    mean     max stddev
1000       100   0.00%   0.00%   0.00% 100.00%      -50      -1     +60     25
1050       105   0.00%   0.00%   0.00% 100.00%      -59     -25     +14     13
1102       110   0.00%   0.00%   0.00% 100.00%      -36      +4     +36     13
1157       115   0.00%   0.00%   0.00% 100.00%      -26     +20     +67     16
1214       121   0.00%   0.00%   0.00% 100.00%      +11     +49     +85     14
1274       127   0.00%   0.00%   0.00% 100.00%      -40     +36     +93     33
1337       133   0.00%   0.00%   0.00% 100.00%      -37      +4     +49     17
1403       140   0.00%   0.00%   0.00% 100.00%      -37      +9     +49     14
1473       147   0.00%   0.00%   0.00% 100.00%      -54     -14     +25     14
1546       154   0.00%   0.00%   0.00% 100.00%      -52      +9     +48     16
1623       162   0.00%   0.00%   0.00% 100.00%      -35      +7     +50     15
1704       170   0.00%   0.00%   0.00% 100.00%      -34      +1     +36     13
1789       178   0.00%   0.00%   0.00% 100.00%      -44      -8     +31     13
1878       187   0.00%   0.00%   0.00% 100.00%      -73     -33     +10     14
1971       197   0.00%   0.00%   0.00% 100.00%      -76     -40      +0     14
2069       206   0.00%   0.00%   0.00% 100.00%      -62      -4     +38     17
2172       217   0.00%   0.00%   0.00% 100.00%      -11     +28     +68     15
2280       228   0.00%   0.00%   0.00% 100.00%       +3     +47     +92     15
2394       239   0.00%   0.00%   0.00% 100.00%      -19     +47     +95     22
2513       251   0.00%   0.00%   0.00% 100.00%      -11     +22     +58     13
2638       263   0.00%   0.00%   0.00% 100.00%      -45      -5     +56     15
2769       276   0.00%   0.00%   0.00% 100.00%      -47     -10     +29     13
2907       290   0.00%   0.00%   0.00% 100.00%      -26     +13     +57     14
3052       305   0.00%   0.00%   0.00% 100.00%       -7     +34     +69     13
3204       320   0.00%   0.00%   0.00% 100.00%      -91     -16     +25     17
3364       336   0.00%   0.00%   0.00% 100.00%      -96     -45     +34     26
3532       353   0.00%   0.00%   0.00% 100.00%      -36     +13     +60     18
3708       370   0.00%   0.00%   0.00% 100.00%      -41      +2     +49     18
3893       389   0.00%   0.00%   0.00% 100.00%       +1     +43     +87     14
4087       408   0.00%   0.00%   0.00% 100.00%      +24     +69    +117     16
4291       429   0.00%   0.00%   0.00% 100.00%      +43     +88    +137     15
4505       450   0.00%   0.00%   0.00% 100.00%      -37     +19     +79     27
4730       473   0.00%   0.00%   0.00% 100.00%      -20     +34  +11484    119
4966       496   0.00%   0.00%   0.00% 100.00%      -47     +11     +57     21
5214       521   0.00%   0.00%   0.00% 100.00%      -83     -29   +4438     49
5474       547   0.00%   0.00%   0.00% 100.00%      -80      +5  +10236    105
5747       574   0.00%   0.00%   0.00% 100.00%      -22     +27     +95     22
6034       603   0.00%   0.00%   0.00% 100.00%      -11     +35  +16445    212
6335       633   0.00%   0.00%   0.00% 100.00%      -44      -9     +29     13
6651       665   0.00%   0.00%   0.00% 100.00%      -30     +19  +16177    141
6983       698   0.00%   0.00%   0.00% 100.00%      +20     +64   +2875     28
7332       733   0.00%   0.00%   0.00% 100.00%      +19     +78  +16312    136
7698       769   0.00%   0.00%   0.00% 100.00%      -48     +33  +16456    137
8082       808   0.00%   0.00%   0.00% 100.00%      -48      -4     +32     14
8486       848   0.00%   0.00%   0.00% 100.00%      -23     +15     +52     13
8910       891   0.00%   0.00%   0.00% 100.00%       -7     +32   +2514     23
9355       935   0.00%   0.00%   0.00% 100.00%      -33     +24   +9744     74
9822       982   0.00%   0.00%   0.00% 100.00%      -23     +18  +16182    227
10313     1031   0.00%   0.00%   0.00% 100.00%      -36      +4  +16439    135
10828     1082   0.00%   0.00%   0.00% 100.00%      -14     +30  +16198    177
11369     1136   0.00%   0.00%   0.00% 100.00%       +4     +53  +16298    144
11937     1193   0.00%   0.00%   0.00% 100.00%       +5     +45  +16296    164
12533     1253   0.00%   0.00%   0.00% 100.00%      -27     +23  +16488    173
13159     1315   0.00%   0.00%   0.00% 100.00%      -18     +24  +16318    135
13816     1381   0.00%   0.00%   0.00% 100.00%       -9     +40  +16615    143
14506     1450   0.00%   0.00%   0.00% 100.00%      -35     +18  +16327    155
15231     1523   0.00%   0.00%   0.00% 100.00%      -36      +3  +16232    153
15992     1599   0.00%   0.00%   0.00% 100.00%      -31     +12  +16371    148
16791     1679   0.00%   0.00%   0.00% 100.00%      -29     +15  +16443    195
17630     1763   0.00%   0.00%   0.00% 100.00%       -9     +30  +16399    188
18511     1851   0.00%   0.00%   0.00% 100.00%       +3   +2039  +18611   5296
19436     1943   0.00%   0.00%   0.00% 100.00%     -101   +1658  +18800   4692
20407     2040   0.00%   0.00%   0.00% 100.00%      +23    +120  +16389    217
21427     2142   0.00%   0.00%   0.00% 100.00%      -28    +126  +16177    331
22498     2249   0.00%   0.00%   0.00% 100.00%      -21    +150  +16525    738
23622     2362   0.00%   0.00%   0.00% 100.00%       +9    +293  +16218   1254
24803     2480   0.00%   0.00%   0.00% 100.00%      -40    +322  +16404   1423
26043     2604   0.00%   0.00%   0.00% 100.00%      -49    +389  +16426   1676
27345     2734   0.00%   0.00%   0.00% 100.00%      -51    +691  +16498   2505
28712     2871   0.00%   0.00%   0.00% 100.00%      -51    +963  +16607   3015
30147     3014   0.00%   0.00%   0.00% 100.00%      -19   +1199  +16634   3397
31654     3165   0.00%   0.00%   0.00% 100.00%      +10   +1592  +16675   3920
33236     3323   0.00%   0.00%   0.00% 100.00%      -37   +1437  +16766   3881
34897     3489   0.00%   0.00%   0.00% 100.00%      -34   +1031  +16765   3421
36641     3664   0.00%   0.00%   0.00% 100.00%      -31    +308  +16668   1906
38473     3847   0.00%   0.00%   0.00% 100.00%      -28    +327  +16841   1985
40396     4039   0.00%   0.00%   0.00% 100.00%      -11    +570  +16775   2738
42415     4241   0.00%   0.00%   0.00% 100.00%      -24    +392  +16872   2225
44535     4453   0.00%   0.00%   0.00% 100.00%      -12    +396  +17011   2146
46761     4676   0.00%   0.00%   0.00% 100.00%      -28    +406  +17292   2061
49099     4909   0.00%   0.00%   0.00% 100.00%      -43    +425  +17224   2025
51553     5155   0.00%   0.00%   0.00% 100.00%      -26    +578  +17207   2192
54130     5413   0.00%   0.00%   0.00% 100.00%      -22    +703  +17214   2254
56836     5683   0.00%   0.00%   0.00% 100.00%       -9    +780  +17239   2192
59677     5967   0.00%   0.00%   0.00% 100.00%      -41   +1194  +17258   2931
62660     6266   0.00%   0.00%   0.00% 100.00%      -51   +1245  +17285   3066
65793     6579   0.00%   0.00%   0.00% 100.00%      -64   +1247  +17246   3103
69082     6908   0.00%   0.00%   0.00% 100.00%     -784   +2074  +18422   3355
72536     7253   0.00%   0.00%   0.00% 100.00%     -982    +660  +16681   1700
76162     7616   0.00%   0.00%   0.00% 100.00%       +0   +1462  +17309   2513
79970     7997   0.00%   0.00%   0.00% 100.00%     +280   +3833  +17455   5181
83968     8396   0.00%   0.00%   0.00% 100.00%      -26   +1047  +17189   1689
88166     8816   0.00%   0.00%   0.00% 100.00%      -85    +835  +16783   1307
92574     9257   0.00%   0.00%   0.00% 100.00%     -122    +803  +16628   1311
97202     9720   0.00%   0.00%   0.00% 100.00%      -44    +947  +16791   1345
102062   10206   0.00%   0.00%   0.00% 100.00%       +7   +1033  +16637   1349
107165   10716   0.01%   0.00%   0.00%  99.99%       +7   +1002  +24510   1311
112523   11252   0.00%   0.00%   0.00% 100.00%      +14   +1054  +16479   1281
118149   11814   0.00%   0.00%   0.11%  99.89%     -170   +1560  +17766   1122
124056   12405   0.00%   0.00%   0.00% 100.00%      +24   +1255  +16566   1230
130258   13025   0.00%   0.00%   0.00% 100.00%      +28   +1218  +16479   1331
136770   13677   0.12%   0.00%   0.05%  99.82%      -13   +1336  +16770   1586
143608   14360   3.35%   0.00%   0.23%  96.41%      -31   +1547  +21513   1924
150788   15078   7.22%   0.00%   0.41%  92.37%      -37   +1617  +25333   1989
158327   15832  11.59%   0.00%   0.62%  87.78%      -44   +1757  +24397   2110
166243   16384  15.07%   0.00%   0.77%  84.16%      -36   +2074  +21110   2345
174555   16384  19.09%   0.00%   0.92%  80.00%      -45   +2109  +26063   2294
183282   16384  23.00%   0.00%   1.02%  75.98%      -35   +2222 +153077   2407
192446   16384  26.77%   0.00%   1.14%  72.09%      -41   +2267  +18813   2397
202068   16384  30.04%   0.00%   1.21%  68.74%      -45   +2374  +23266   2413
212171   16384  32.80%   0.00%   1.34%  65.86%      -49   +2563  +23285   2307
222779   16384  36.43%   0.00%   1.31%  62.26%     -158   +2693  +26937   2346
233917   16384  39.76%   0.00%   1.38%  58.86%      +85   +2848  +25018   2349
245612   16384  42.52%   0.00%   1.41%  56.07%      +89   +2846 +153305   2409
257892   16384  44.83%   0.00%   1.41%  53.76%      +14   +2789  +24800   2323
270786   16384  47.64%   0.00%   1.43%  50.94%      -19   +2766  +25002   2382
284325   16384  49.02%   0.00%   1.51%  49.47%     -886   +2687  +23521   2006
298541   16384  52.04%   0.00%   1.51%  46.44%     -917   +3332 +118317   2057

-- 
Miroslav Lichvar

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Kurt Kanzenbach]

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

On Mon Aug 18 2025, Miroslav Lichvar wrote:
> On Fri, Aug 15, 2025 at 08:50:23AM +0200, Kurt Kanzenbach wrote:
>> Retrieve Tx timestamp directly from interrupt handler.
>> 
>> 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, fetch the timestamp directly from the interrupt handler.
>> 
>> The work queue code stays for the Intel 82576. Tested on Intel i210.
>
> I tested this patch on 6.17-rc1 with an Intel I350 card on a NTP
> server (chrony 4.4), measuring packet rates and TX timestamp accuracy
> with ntpperf. While the HW TX timestamping seems more reliable at some
> lower request rates, there seems to be about 40% drop in the overall
> performance of the server in how much requests it can handle (falling
> back to SW timestamps when HW timestamp is missed). Is this expected
> or something to be considered? 

Thanks for testing! Nope, this is not really expected. Let me see if I
can reproduce your results and see where that comes from.

Thanks,
Kurt

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

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Kurt Kanzenbach]

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

On Mon Aug 18 2025, Miroslav Lichvar wrote:
> On Fri, Aug 15, 2025 at 08:50:23AM +0200, Kurt Kanzenbach wrote:
>> Retrieve Tx timestamp directly from interrupt handler.
>> 
>> 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, fetch the timestamp directly from the interrupt handler.
>> 
>> The work queue code stays for the Intel 82576. Tested on Intel i210.
>
> I tested this patch on 6.17-rc1 with an Intel I350 card on a NTP
> server (chrony 4.4), measuring packet rates and TX timestamp accuracy
> with ntpperf. While the HW TX timestamping seems more reliable at some
> lower request rates, there seems to be about 40% drop in the overall
> performance of the server in how much requests it can handle (falling
> back to SW timestamps when HW timestamp is missed). Is this expected
> or something to be considered? 

I have never ever used ntpperf before, so please bear with me. I've
setup two x86 machines with Debian Trixie, v6.17.0-rc1+ and Intel i210.

Installed ntpperf on one machine and chrony (v4.6) on the second. In the
chrony config there is 'hwtimestamp enp1s0'. I did run the first example
in ntpperf's README with the following results. 'rate' seems to be
higher with my patch applied. Anyway, your ntpperf output looks
completely different. What parameters are you using? I just want to
reproduce your results first.

* ntpperf with igb patch applied
Linux cml1 6.17.0-rc1+ #1 SMP PREEMPT_RT Tue Aug 19 12:56:41 CEST 2025 x86_64 GNU/Linux
Linux cml2 6.17.0-rc1+ #1 SMP PREEMPT_RT Tue Aug 19 12:56:41 CEST 2025 x86_64 GNU/Linux
root@cml1:~/ntpperf# ./ntpperf -i enp1s0 -m 6c:b3:11:52:39:15 -d 192.168.123.1 -s 172.18.0.0/16 -B -H
               |          responses            |        response time (ns)
rate   clients |  lost invalid   basic  xleave |    min    mean     max stddev
1000       100   0.00%   0.00% 100.00%   0.00%   +15124  +16491 +166838   4773
1500       150   0.00%   0.00% 100.00%   0.00%   +14589  +16163 +170222   4028
2250       225   0.00%   0.00% 100.00%   0.00%   +14337  +15825 +172604   3356
3375       337   0.00%   0.00% 100.00%   0.00%   +14267  +15549 +171365   2727
5062       506   0.00%   0.00% 100.00%   0.00%   +14033  +15389 +177490   2384
7593       759   0.00%   0.00% 100.00%   0.00%   +14307  +15418 +174652   1983
11389     1138   0.00%   0.00% 100.00%   0.00%   +14100  +16551 +169245   6036
17083     1708   0.00%   0.00% 100.00%   0.00%   +10077  +20077 +194647   9182
25624     2562   0.00%   0.00% 100.00%   0.00%   +10007  +25572 +181577  14952
38436     3843   0.00%   0.00% 100.00%   0.00%    +6851  +27466 +184041  14900
57654     5765   0.00%   0.00% 100.00%   0.00%    +5164  +29218 +246544  16169
86481     8648   0.00%   0.00% 100.00%   0.00%    +5346  +36063 +179512  14388
129721   12972   0.00%   0.00% 100.00%   0.00%    +7934  +49386 +229427  17600
194581   16384   0.00%   0.00% 100.00%   0.00%   +10760  +54961 +248325  18860
291871   16384   0.00%   0.00% 100.00%   0.00%   +13207  +57193 +248870  16908
437806   16384  25.42%   0.00%  74.58%   0.00%  +211479 +275061 +703480  20529
656709   16384  50.32%   0.00%  49.68%   0.00%  +230344 +292741 +485387  19119

* ntpperf without igb patch applied
Linux cml1 6.17.0-rc1+ #3 SMP PREEMPT_RT Tue Aug 19 15:31:35 CEST 2025 x86_64 GNU/Linux
Linux cml2 6.17.0-rc1+ #3 SMP PREEMPT_RT Tue Aug 19 15:31:35 CEST 2025 x86_64 GNU/Linux
root@cml1:~/ntpperf# ./ntpperf -i enp1s0 -m 6c:b3:11:52:39:15 -d 192.168.123.1 -s 172.18.0.0/16 -B -H
               |          responses            |        response time (ns)
rate   clients |  lost invalid   basic  xleave |    min    mean     max stddev
1000       100   0.00%   0.00% 100.00%   0.00%   +18246  +19931 +174292   4947
1500       150   0.00%   0.00% 100.00%   0.00%   +17934  +19494 +194876   4368
2250       225   0.00%   0.00% 100.00%   0.00%   +17969  +19254 +177394   3449
3375       337   0.00%   0.00% 100.00%   0.00%   +17687  +19090 +179947   2807
5062       506   0.00%   0.00% 100.00%   0.00%   +17798  +18999 +175463   2403
7593       759   0.00%   0.00% 100.00%   0.00%   +17901  +19039 +176656   1984
11389     1138   0.00%   0.00% 100.00%   0.00%   +17192  +20318 +184805   6802
17083     1708   0.00%   0.00% 100.00%   0.00%   +14241  +24428 +185137  10118
25624     2562   0.00%   0.00% 100.00%   0.00%    +4819  +22667 +201990   7962
38436     3843   0.00%   0.00% 100.00%   0.00%    +3869  +17957 +192566   8171
57654     5765   0.00%   0.00% 100.00%   0.00%    +2529  +14036 +173719   6240
86481     8648   0.00%   0.00% 100.00%   0.00%    +2774  +13642 +201026   5284
129721   12972   0.00%   0.00% 100.00%   0.00%    +2670  +14699 +242395   6692
194581   16384   0.00%   0.00% 100.00%   0.00%    +2520  +19712 +329254   9571
291871   16384   1.37%   0.00%  98.63%   0.00%    +2818  +77396 +15480693 182286
437806   16384  24.69%   0.00%  75.31%   0.00%  +108662 +246855 +2306431  38520
Could not send requests at rate 656709

Thanks,
Kurt

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

Re: [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 871 bytes --]



On 8/14/2025 11:50 PM, Kurt Kanzenbach wrote:
> Retrieve Tx timestamp directly from interrupt handler.
> 
> 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, fetch the timestamp directly from the interrupt handler.
> 
> The work queue code stays for the Intel 82576. Tested on Intel i210.
> 

The change makes sense to me. All we really have to do in the interrupt
is read the timestamp register, and then report it to stack. Its much
better to do that work immediately if we don't need any locking or other
CPU intensive tasks here.

I can confirm that 82576 does not have the interrupt, it was introduced
in later hardware.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1988 bytes --]



On 8/18/2025 5:24 AM, Miroslav Lichvar wrote:
> On Fri, Aug 15, 2025 at 08:50:23AM +0200, Kurt Kanzenbach wrote:
>> Retrieve Tx timestamp directly from interrupt handler.
>>
>> 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, fetch the timestamp directly from the interrupt handler.
>>
>> The work queue code stays for the Intel 82576. Tested on Intel i210.
> 
> I tested this patch on 6.17-rc1 with an Intel I350 card on a NTP
> server (chrony 4.4), measuring packet rates and TX timestamp accuracy
> with ntpperf. While the HW TX timestamping seems more reliable at some
> lower request rates, there seems to be about 40% drop in the overall
> performance of the server in how much requests it can handle (falling
> back to SW timestamps when HW timestamp is missed). Is this expected
> or something to be considered? 
> 

Hm. The new steps do have to perform all the tasks for completing a Tx
timestamp within the interrupt, but that involves reading the register,
converting it to the appropriate format, clearing a bitlock, and
submitting the skb to the stack. I can't imagine those tasks being super
problematic within interrupt context..

The Tx timestamp interrupt occurs on the other IRQ which is used
primarily for non-traffic causes which aren't high volume (other than
the Tx interrupt itself). I suppose possibly freeing the SKB could be
something that is causing issues in interrupt context?

I'm having trouble interpreting what exactly this data shows, as its
quite a lot of data and numbers. I guess that it is showing when it
switches over to software timestamps.. It would be nice if ntpperf
showed number of events which were software vs hardware timestamping, as
thats likely the culprit. igb hardare only has a single outstanding Tx
timestamp at a time.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Miroslav Lichvar]

On Tue, Aug 19, 2025 at 04:50:23PM +0200, Kurt Kanzenbach wrote:
> Installed ntpperf on one machine and chrony (v4.6) on the second. In the
> chrony config there is 'hwtimestamp enp1s0'. I did run the first example
> in ntpperf's README with the following results. 'rate' seems to be
> higher with my patch applied. Anyway, your ntpperf output looks
> completely different. What parameters are you using? I just want to
> reproduce your results first.
> 
> root@cml1:~/ntpperf# ./ntpperf -i enp1s0 -m 6c:b3:11:52:39:15 -d 192.168.123.1 -s 172.18.0.0/16 -B -H

Interleaved mode needs to be selected by the -I option (instead of -B)
in order for the server to enable SW+HW TX timestamps for the
responses it sends back to the client.

My ntpperf command line also had "-x 1.05" to increase the rate in
smaller steps and "-o 0.000000500" to print the offset between the
server TX and client RX timestamp instead of the offset between
server's TX and RX timestamp (response time), but that requires the
NIC PHCs to be synchronized to each other over a different network
link and the -o value to be calibrated for the delays in timestamping
and cable. It's not important for this issue, no need to bother with
that.

> * ntpperf with igb patch applied

> 129721   12972   0.00%   0.00% 100.00%   0.00%    +7934  +49386 +229427  17600
> 194581   16384   0.00%   0.00% 100.00%   0.00%   +10760  +54961 +248325  18860
> 291871   16384   0.00%   0.00% 100.00%   0.00%   +13207  +57193 +248870  16908
> 437806   16384  25.42%   0.00%  74.58%   0.00%  +211479 +275061 +703480  20529
> 
> * ntpperf without igb patch applied

> 129721   12972   0.00%   0.00% 100.00%   0.00%    +2670  +14699 +242395   6692
> 194581   16384   0.00%   0.00% 100.00%   0.00%    +2520  +19712 +329254   9571
> 291871   16384   1.37%   0.00%  98.63%   0.00%    +2818  +77396 +15480693 182286
> 437806   16384  24.69%   0.00%  75.31%   0.00%  +108662 +246855 +2306431  38520

Those results look the same within few percent, as I'd expect with
the basic NTP mode (-B option), which doesn't enable server's TX
timestamping. Normally, I see larger differences in subsequent runs in
the same configuration.

-- 
Miroslav Lichvar

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Miroslav Lichvar]

On Tue, Aug 19, 2025 at 04:31:49PM -0700, Jacob Keller wrote:
> I'm having trouble interpreting what exactly this data shows, as its
> quite a lot of data and numbers. I guess that it is showing when it
> switches over to software timestamps.. It would be nice if ntpperf
> showed number of events which were software vs hardware timestamping, as
> thats likely the culprit. igb hardare only has a single outstanding Tx
> timestamp at a time.

The server doesn't have a way to tell the client (ntpperf) which
timestamps are HW or SW, we can only guess from the measured offset as
HW timestamps should be more accurate, but on the server side the
number of SW and HW TX timestamps provided to the client can be
monitored with the "chronyc serverstats" command. The server requests
both SW and HW TX timestamps and uses the better one it gets from the
kernel, if it can actually get one before it receives the next
request from the same client (ntpperf simulates up to 16384 concurrent
clients).

When I run ntpperf at a fixed rate of 140000 requests per second
for 10 seconds (-r 140000 -t 10), I get the following numbers.

Without the patch:
NTP daemon TX timestamps   : 28056
NTP kernel TX timestamps   : 1012864
NTP hardware TX timestamps : 387239

With the patch:
NTP daemon TX timestamps   : 28047
NTP kernel TX timestamps   : 707674
NTP hardware TX timestamps : 692326

The number of HW timestamps is significantly higher with the patch, so
that looks good.

But when I increase the rate to 200000, I get this:

Without the patch:
NTP daemon TX timestamps   : 35835
NTP kernel TX timestamps   : 1410956
NTP hardware TX timestamps : 581575            

With the patch:
NTP daemon TX timestamps   : 476908
NTP kernel TX timestamps   : 646146
NTP hardware TX timestamps : 412095

With the patch, the server is now dropping requests and can provide
a smaller number of HW timestamps and also a smaller number of SW
timestamps, i.e. less work is done overall.

Could the explanation be that a single CPU core now needs to do more
work, while it was better distributed before?

-- 
Miroslav Lichvar

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 2959 bytes --]



On 8/20/2025 12:56 AM, Miroslav Lichvar wrote:
> On Tue, Aug 19, 2025 at 04:31:49PM -0700, Jacob Keller wrote:
>> I'm having trouble interpreting what exactly this data shows, as its
>> quite a lot of data and numbers. I guess that it is showing when it
>> switches over to software timestamps.. It would be nice if ntpperf
>> showed number of events which were software vs hardware timestamping, as
>> thats likely the culprit. igb hardare only has a single outstanding Tx
>> timestamp at a time.
> 
> The server doesn't have a way to tell the client (ntpperf) which
> timestamps are HW or SW, we can only guess from the measured offset as
> HW timestamps should be more accurate, but on the server side the
> number of SW and HW TX timestamps provided to the client can be
> monitored with the "chronyc serverstats" command. The server requests
> both SW and HW TX timestamps and uses the better one it gets from the
> kernel, if it can actually get one before it receives the next
> request from the same client (ntpperf simulates up to 16384 concurrent
> clients).
> 
> When I run ntpperf at a fixed rate of 140000 requests per second
> for 10 seconds (-r 140000 -t 10), I get the following numbers.
> 
> Without the patch:
> NTP daemon TX timestamps   : 28056
> NTP kernel TX timestamps   : 1012864
> NTP hardware TX timestamps : 387239
> 
> With the patch:
> NTP daemon TX timestamps   : 28047
> NTP kernel TX timestamps   : 707674
> NTP hardware TX timestamps : 692326
> 
> The number of HW timestamps is significantly higher with the patch, so
> that looks good.
> 
> But when I increase the rate to 200000, I get this:
> 
> Without the patch:
> NTP daemon TX timestamps   : 35835
> NTP kernel TX timestamps   : 1410956
> NTP hardware TX timestamps : 581575            
> 
> With the patch:
> NTP daemon TX timestamps   : 476908
> NTP kernel TX timestamps   : 646146
> NTP hardware TX timestamps : 412095
> 

When does the NTP daemon decide to go with timestamping within the
daemon vs timestamping in the kernel? It seems odd that we don't achieve
100% kernel timestamps...

> With the patch, the server is now dropping requests and can provide
> a smaller number of HW timestamps and also a smaller number of SW
> timestamps, i.e. less work is done overall.
> 
> Could the explanation be that a single CPU core now needs to do more
> work, while it was better distributed before?
> 

Hm. The interrupt vector may be fired on the same CPU maybe? The work
items can go into the general pool which spreads to all CPUs, and I
guess the amount of work to submit the timestamp is high enough that we
do end up costing too much?

Hmm.

We could experiment with using a kthread via the ptp_aux_work setup and
tuning to ensure that thread has good prioritization? I don't know what
the best compromise is since its clear the interrupt is best if the
timestamp volume isn't too high.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Miroslav Lichvar]

On Wed, Aug 20, 2025 at 01:29:31PM -0700, Jacob Keller wrote:
> On 8/20/2025 12:56 AM, Miroslav Lichvar wrote:
> > Without the patch:
> > NTP daemon TX timestamps   : 35835
> > NTP kernel TX timestamps   : 1410956
> > NTP hardware TX timestamps : 581575            
> > 
> > With the patch:
> > NTP daemon TX timestamps   : 476908
> > NTP kernel TX timestamps   : 646146
> > NTP hardware TX timestamps : 412095
> > 
> 
> When does the NTP daemon decide to go with timestamping within the
> daemon vs timestamping in the kernel? It seems odd that we don't achieve
> 100% kernel timestamps...

Yes, it is odd. The daemon uses the best timestamp it has when the
new request comes from the client asking for the TX timestamp of the
previous response. With 16384 clients and 200000 requests per second,
that's 12 milliseconds between two requests of a client. I tried
increasing the receive buffer size of the server UDP socket and also
increase the number of clients to make the server wait longer for the
SW TX timestamps, but that didn't help. It looks like they are lost.

Due to the way the server implements the interleaved mode, it's the
first two exchanges with a client always have the TX daemon timestamp,
so the "without the patch" result above has only 35835 - 2 * 16384 =
3067 missing SW or HW timestamps (1.5% of all responses), but "with
the patch" it is 476908 - 2 * 16384 = 444140 (28.9% of all responses). 

-- 
Miroslav Lichvar

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Kurt Kanzenbach]

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

On Wed Aug 20 2025, Jacob Keller wrote:
> On 8/20/2025 12:56 AM, Miroslav Lichvar wrote:
>> On Tue, Aug 19, 2025 at 04:31:49PM -0700, Jacob Keller wrote:
>>> I'm having trouble interpreting what exactly this data shows, as its
>>> quite a lot of data and numbers. I guess that it is showing when it
>>> switches over to software timestamps.. It would be nice if ntpperf
>>> showed number of events which were software vs hardware timestamping, as
>>> thats likely the culprit. igb hardare only has a single outstanding Tx
>>> timestamp at a time.
>> 
>> The server doesn't have a way to tell the client (ntpperf) which
>> timestamps are HW or SW, we can only guess from the measured offset as
>> HW timestamps should be more accurate, but on the server side the
>> number of SW and HW TX timestamps provided to the client can be
>> monitored with the "chronyc serverstats" command. The server requests
>> both SW and HW TX timestamps and uses the better one it gets from the
>> kernel, if it can actually get one before it receives the next
>> request from the same client (ntpperf simulates up to 16384 concurrent
>> clients).
>> 
>> When I run ntpperf at a fixed rate of 140000 requests per second
>> for 10 seconds (-r 140000 -t 10), I get the following numbers.
>> 
>> Without the patch:
>> NTP daemon TX timestamps   : 28056
>> NTP kernel TX timestamps   : 1012864
>> NTP hardware TX timestamps : 387239
>> 
>> With the patch:
>> NTP daemon TX timestamps   : 28047
>> NTP kernel TX timestamps   : 707674
>> NTP hardware TX timestamps : 692326
>> 
>> The number of HW timestamps is significantly higher with the patch, so
>> that looks good.
>> 
>> But when I increase the rate to 200000, I get this:
>> 
>> Without the patch:
>> NTP daemon TX timestamps   : 35835
>> NTP kernel TX timestamps   : 1410956
>> NTP hardware TX timestamps : 581575            
>> 
>> With the patch:
>> NTP daemon TX timestamps   : 476908
>> NTP kernel TX timestamps   : 646146
>> NTP hardware TX timestamps : 412095
>> 
>
> When does the NTP daemon decide to go with timestamping within the
> daemon vs timestamping in the kernel? It seems odd that we don't achieve
> 100% kernel timestamps...
>
>> With the patch, the server is now dropping requests and can provide
>> a smaller number of HW timestamps and also a smaller number of SW
>> timestamps, i.e. less work is done overall.
>> 
>> Could the explanation be that a single CPU core now needs to do more
>> work, while it was better distributed before?
>> 
>
> Hm. The interrupt vector may be fired on the same CPU maybe? The work
> items can go into the general pool which spreads to all CPUs, and I
> guess the amount of work to submit the timestamp is high enough that we
> do end up costing too much?
>
> Hmm.
>
> We could experiment with using a kthread via the ptp_aux_work setup and
> tuning to ensure that thread has good prioritization? I don't know what
> the best compromise is since its clear the interrupt is best if the
> timestamp volume isn't too high.

Miroslav, can you test the following patch? Does this help?

From 0b795f37fecd235bf4c9965148afaf33e4a5139c Mon Sep 17 00:00:00 2001
From: Kurt Kanzenbach <kurt@linutronix.de>
Date: Wed, 20 Aug 2025 14:01:45 +0200
Subject: [PATCH] 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 according
to use case.

Tested on Intel i210.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.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  | 26 ++++++++++++-----------
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 43401c3c824f..0d2bdde47c32 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 fe0c0a5caa85..5974180721b1 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6665,7 +6665,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++;
 		}
@@ -6701,7 +6701,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);
 	}
 
@@ -7169,7 +7169,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 a7876882aeaf..f0b08809cf91 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -803,15 +803,15 @@ 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)
+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;
-- 
2.47.2


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

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Miroslav Lichvar]

On Thu, Aug 21, 2025 at 01:38:44PM +0200, Kurt Kanzenbach wrote:
> On Wed Aug 20 2025, Jacob Keller wrote:
> > On 8/20/2025 12:56 AM, Miroslav Lichvar wrote:
> >> But when I increase the rate to 200000, I get this:
> >> 
> >> Without the patch:
> >> NTP daemon TX timestamps   : 35835
> >> NTP kernel TX timestamps   : 1410956
> >> NTP hardware TX timestamps : 581575            
> >> 
> >> With the patch:
> >> NTP daemon TX timestamps   : 476908
> >> NTP kernel TX timestamps   : 646146
> >> NTP hardware TX timestamps : 412095

> Miroslav, can you test the following patch? Does this help?

It seems better than with the original patch, but not as good as
before, at least in the tests I'm doing. The maximum packet rate the
server can handle is now only about 5% worse (instead of 40%), but the
the number of missing timestamps on the server still seems high.

With the new patch at 200000 requests per second:
NTP daemon TX timestamps   : 192404
NTP kernel TX timestamps   : 1318971
NTP hardware TX timestamps : 418805

I didn't try to adjust the aux worker priority.

-- 
Miroslav Lichvar

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Kurt Kanzenbach]

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

On Thu Aug 21 2025, Miroslav Lichvar wrote:
> On Thu, Aug 21, 2025 at 01:38:44PM +0200, Kurt Kanzenbach wrote:
>> On Wed Aug 20 2025, Jacob Keller wrote:
>> > On 8/20/2025 12:56 AM, Miroslav Lichvar wrote:
>> >> But when I increase the rate to 200000, I get this:
>> >> 
>> >> Without the patch:
>> >> NTP daemon TX timestamps   : 35835
>> >> NTP kernel TX timestamps   : 1410956
>> >> NTP hardware TX timestamps : 581575            
>> >> 
>> >> With the patch:
>> >> NTP daemon TX timestamps   : 476908
>> >> NTP kernel TX timestamps   : 646146
>> >> NTP hardware TX timestamps : 412095
>
>> Miroslav, can you test the following patch? Does this help?
>
> It seems better than with the original patch, but not as good as
> before, at least in the tests I'm doing. The maximum packet rate the
> server can handle is now only about 5% worse (instead of 40%), but the
> the number of missing timestamps on the server still seems high.
>
> With the new patch at 200000 requests per second:
> NTP daemon TX timestamps   : 192404
> NTP kernel TX timestamps   : 1318971
> NTP hardware TX timestamps : 418805
>
> I didn't try to adjust the aux worker priority.

Here's what I can see in the traces: In the current implementation, the
kworker runs directly after the IRQ on the *same* CPU. With the AUX
worker approach the kthread can be freely distributed to any other
CPU. This in turn involves remote wakeups etc.

You could try to pin the PTP AUX worker (e.g. called ptp0) with taskset
to the same CPU where the TS IRQs are processed. That might help to get
the old behavior back. Adjusting the priority is not necessary, both the
kworker and AUX thread run with 120 (SCHED_OTHER, nice value 0) by
default.

Thanks,
Kurt

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

Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp directly from interrupt

[Miroslav Lichvar]

On Thu, Aug 21, 2025 at 04:08:02PM +0200, Kurt Kanzenbach wrote:
> On Thu Aug 21 2025, Miroslav Lichvar wrote:
> >> >> Without the patch:
> >> >> NTP daemon TX timestamps   : 35835
> >> >> NTP kernel TX timestamps   : 1410956
> >> >> NTP hardware TX timestamps : 581575            
> >> >> 
> >> >> With the patch:
> >> >> NTP daemon TX timestamps   : 476908
> >> >> NTP kernel TX timestamps   : 646146
> >> >> NTP hardware TX timestamps : 412095

> > With the new patch at 200000 requests per second:
> > NTP daemon TX timestamps   : 192404
> > NTP kernel TX timestamps   : 1318971
> > NTP hardware TX timestamps : 418805

> Here's what I can see in the traces: In the current implementation, the
> kworker runs directly after the IRQ on the *same* CPU. With the AUX
> worker approach the kthread can be freely distributed to any other
> CPU. This in turn involves remote wakeups etc.
> 
> You could try to pin the PTP AUX worker (e.g. called ptp0) with taskset
> to the same CPU where the TS IRQs are processed. That might help to get
> the old behavior back. Adjusting the priority is not necessary, both the
> kworker and AUX thread run with 120 (SCHED_OTHER, nice value 0) by
> default.

Yes, that helps!

The server timestamping stats now show:
NTP daemon TX timestamps   : 32902
NTP kernel TX timestamps   : 1479293
NTP hardware TX timestamps : 520146

And the maximum response rate is only about 2-3% lower, so that looks
good to me.

Thanks,

-- 
Miroslav Lichvar