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 <[email protected]> >>>> --- >>>> 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 <[email protected]> > > Reviewed-by: Vadim Fedorenko <[email protected]> Thanks! Kurt
signature.asc
Description: PGP signature
