| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87bjofoct2.fsf@jax.kurt.home>
Date: Sat, 16 Aug 2025 11:06:49 +0200
From: Kurt Kanzenbach <kurt@...utronix.de>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Paul Menzel
<pmenzel@...gen.mpg.de>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, Przemek Kitszel
<przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, Richard Cochran <richardcochran@...il.com>, Vinicius
Costa Gomes <vinicius.gomes@...el.com>, Sebastian Andrzej Siewior
<bigeasy@...utronix.de>, intel-wired-lan@...ts.osuosl.org,
netdev@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-next] igb: Retrieve Tx timestamp
directly from interrupt
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@...utronix.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@...gen.mpg.de>
>
> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Thanks!
Kurt
Download attachment "signature.asc" of type "application/pgp-signature" (862 bytes)
Powered by blists - more mailing lists