netdev - Re: [PATCH RFC net-next v1 1/6] ethtool: add interface to read Tx hardware timestamping statistics

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d030d02d-150d-4337-a063-69da28049548@intel.com>
Date: Thu, 7 Mar 2024 19:29:08 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Rahul Rameshbabu <rrameshbabu@...dia.com>
CC: Saeed Mahameed <saeed@...nel.org>, Leon Romanovsky <leon@...nel.org>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "Jonathan
 Corbet" <corbet@....net>, Richard Cochran <richardcochran@...il.com>, "Tariq
 Toukan" <tariqt@...dia.com>, Gal Pressman <gal@...dia.com>, Vadim Fedorenko
	<vadim.fedorenko@...ux.dev>, Andrew Lunn <andrew@...n.ch>, Heiner Kallweit
	<hkallweit1@...il.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	"Ahmed Zaki" <ahmed.zaki@...el.com>, Alexander Lobakin
	<aleksander.lobakin@...el.com>, Hangbin Liu <liuhangbin@...il.com>, "Paul
 Greenwalt" <paul.greenwalt@...el.com>, Justin Stitt <justinstitt@...gle.com>,
	Randy Dunlap <rdunlap@...radead.org>, Maxime Chevallier
	<maxime.chevallier@...tlin.com>, Kory Maincent <kory.maincent@...tlin.com>,
	Wojciech Drewek <wojciech.drewek@...el.com>, Vladimir Oltean
	<vladimir.oltean@....com>, Jiri Pirko <jiri@...nulli.us>, Alexandre Torgue
	<alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, "Dragos
 Tatulea" <dtatulea@...dia.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>
Subject: Re: [PATCH RFC net-next v1 1/6] ethtool: add interface to read Tx
 hardware timestamping statistics



On 3/7/2024 10:47 AM, Rahul Rameshbabu wrote:
> Hi Jacob,
> 
> On Mon, 26 Feb, 2024 11:54:49 -0800 Jacob Keller <jacob.e.keller@...el.com> wrote:
>> On 2/23/2024 3:43 PM, Rahul Rameshbabu wrote:
>>>
>>> On Fri, 23 Feb, 2024 14:48:51 -0800 Jacob Keller <jacob.e.keller@...el.com>
>>> wrote:
>>>> On 2/23/2024 2:21 PM, Rahul Rameshbabu wrote:
>>>>> Do you have any example of a case of skipping timestamp information that
>>>>> is not related to lack of delivery over time? I am wondering if this
>>>>> case is more like a hardware error or not. Or is it more like something
>>>>> along the lines of being busy/would impact line rate of timestamp
>>>>> information must be recorded?
>>>>>
>>>>
>>>> The main example for skipped is the event where all our slots are full
>>>> at point of timestamp request.
>>>
>>> This is what I was guessing as the main (if not only reason). For this
>>> specific reason, I think a general "busy" stats counter makes sense.
>>> mlx5 does not need this counter, but I can see a lot of other hw
>>> implementations needing this. (The skipped counter name obviously should
>>> be left only in the ice driver. Just felt "busy" was easy to understand
>>> for generalized counters.)
>>
>> Yea, I don't expect this would be required for all hardware but it seems
>> like a common approach if you have limited slots for Tx timestamps
>> available.
>>
> Sorry to bump this thread once more, but I had a question regarding the
> Intel driver in regards to this. Instead of having a busy case when all
> the slots are full, would it make sense to stop the netdev queues in
> this case, we actually do this in mlx5 (though keep in mind that we have
> a dedicated queue just for port/phy timestamping that we start/stop).
> 
> Maybe in your case, you can have a mix of HW timestamping and non-HW
> timestamping in the same queue, which is why you have a busy case?
> 

We don't use a dedicated queue. The issue isn't queue capacity so much
as it is the number of slots in the PHY for where it can save the
timestamp data.

In practice the most common application (ptp4l) synchronously waits for
timestamps, and only has one outstanding at a time. Likely due to
limitations with original hardware that only supported one outstanding
Tx timestamp.

> Wanted to inquire about this before sending out a RFC v2.

That's actually an interesting approach to change to a dedicated queue
which we could lock and start/stop it when the indexes are full. How
does that interact with the stack UDP and Ethernet stacks? Presumably
when you go to transmit, you'd need to pick a queue and if its stopped
you'd have to drop or tell the stack?

I think I remember someone experimenting with returning NETDEV_TX_BUSY
when the slots were full, but in practice this caused a lot of issues.
None of the other devices we have with only a single slot (one set of
registers, ixgbe, i40e, igb, e1000) did that either.

If this queue model behaves in a sane way (or if we can communicate
something similar by reporting back up the stack without needing a
dedicated queue?) that could be better than the current situation.


>>>
>>> The reason why I prefer busy is that "skip" to me makes me think someone
>>> used SIOCSHWTSTAMP to filter which packets get timestamped which is very
>>> different from something like lack of resource availability.
>>>
>>
>> Busy is fine with me.
>>
> 
> --
> Thanks,
> 
> Rahul Rameshbabu