Message-ID: <87ecswq5vj.fsf@jax.kurt.home>
Date: Wed, 27 Aug 2025 14:57:36 +0200
From: Kurt Kanzenbach <kurt@...utronix.de>
To: Jacob Keller <jacob.e.keller@...el.com>, Sebastian Andrzej Siewior
 <bigeasy@...utronix.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>, 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 Tue Aug 26 2025, Jacob Keller wrote:
> 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.

Does that mean we're going back to v1 + the AUX worker for 82576? Let me
prepare v3 then.

Thanks,
Kurt

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

Powered by blists - more mailing lists