Message-ID: <e656a4ee-281c-4205-9183-bc3c7dbc9173@intel.com>
Date: Tue, 26 Aug 2025 11:23:55 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
CC: Kurt Kanzenbach <kurt@...utronix.de>, 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>, Paul Menzel
	<pmenzel@...gen.mpg.de>, Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Miroslav Lichvar <mlichvar@...hat.com>, <intel-wired-lan@...ts.osuosl.org>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-next v2] igb: Convert Tx timestamping to PTP aux
 worker



On 8/26/2025 5:59 AM, Sebastian Andrzej Siewior wrote:
> On 2025-08-25 16:28:38 [-0700], Jacob Keller wrote:
>> Ya, I don't think we fully understand either. Miroslav said he tested on
>> I350 which is a different MAC from the I210, so it could be something
>> there. Theoretically we could handle just I210 directly in the interrupt
>> and leave the other variants to the kworker.. but I don't know how much
>> benefit we get from that. The data sheet for the I350 appears to have
>> more or less the same logic for Tx timestamps. It is significantly
>> different for Rx timestamps though.
> 
> From logical point of view it makes sense to retrieve the HW timestamp
> immediately when it becomes available and feed it to the stack. I can't
> imagine how delaying it to yet another thread improves the situation.
> The benchmark is about > 1k packets/ second while in reality you have
> less than 20 packets a second. With multiple applications you usually
> need a "second timestamp register" or you may lose packets.
> 
> Delaying it to the AUX worker makes sense for hardware which can't fire
> an interrupt and polling is the only option left. This is sane in this
> case but I don't like this solution as some kind compromise for
> everyone. Simply because it adds overhead and requires additional
> configuration.
> 

I agree. Its just frustrating that doing so appears to cause a
regression in at least one test setup on hardware which uses this method.

>>> Also I couldn't really see a performance degradation with ntpperf. In my
>>> tests the IRQ variant reached an equal or higher rate. But sometimes I
>>> get 'Could not send requests at rate X'. No idea what that means.
>>>
>>> Anyway, this patch is basically a compromise. It works for Miroslav and
>>> my use case.
>>>
>>>> This is also what the igc does and the performance improved
>>>> 	afa141583d827 ("igc: Retrieve TX timestamp during interrupt handling")
>>>>
>>
>> igc supports several hardware variations which are all a lot similar to
>> i210 than i350 is to i210 in igb. I could see this working fine for i210
>> if it works fine in igb.. I honestly am at a loss currently why i350 is
>> much worse.
>>
>>>> and here it causes the opposite?
>>>
>>> As said above, I'm out of ideas here.
>>>
>>
>> Same. It may be one of those things where the effort to dig up precisely
>> what has gone wrong is so large that it becomes not feasible relative to
>> the gain :(
> 
> Could we please use the direct retrieval/ submission for HW which
> supports it and fallback to the AUX worker (instead of the kworker) for
> HW which does not have an interrupt for it?
> 

I have no objection. Perhaps we could assume the high end of the ntpperf
benchmark is not reflective of normal use case? We *are* limited to only
one timestamp register, which the igb driver does protect by bitlock.

>>> Thanks,
>>> Kurt
> 
> Sebastian



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)

Powered by blists - more mailing lists