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

[Paul Menzel <pmenzel@molgen.mpg.de>]

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