Message-ID: <87il2evhtk.fsf@nvidia.com>
Date: Fri, 23 Feb 2024 15:43:27 -0800
From: Rahul Rameshbabu <rrameshbabu@...dia.com>
To: Jacob Keller <jacob.e.keller@...el.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 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:
>>> The Intel ice drivers has the following Tx timestamp statistics:
>>>
>>> tx_hwtstamp_skipped - indicates when we get a Tx timestamp request but
>>> are unable to fulfill it.
>>> tx_hwtstamp_timeouts - indicates we had a Tx timestamp skb waiting for a
>>> timestamp from hardware but it didn't get received within some internal
>>> time limit.
>> 
>> This is interesting. In mlx5 land, the only case where we are unable to
>> fulfill a hwtstamp is when the timestamp information is lost or late.
>> 
>
> For ice, the timestamps are captured in the PHY and stored in a block of
> registers with limited slots. The driver tracks the available slots and
> uses one when a Tx timestamp request comes in.
>
> So we have "skipped" because its possible to request too many timestamps
> at once and fill up all the slots before the first timestamp is reported
> back.
>
>> lost for us means that the timestamp never arrived within some internal
>> time limit that our device will supposedly never be able to deliver
>> timestamp information after that point.
>> 
>
> That is more or less the equivalent we have for timeout.
>
>> late for us is that we got hardware timestamp information delivered
>> after that internal time limit. We are able to track this by using
>> identifiers in our completion events and we only release references to
>> these identifiers upon delivery (never delivering leaks the references.
>> Enough build up leads to a recovery flow). The theory for us is that
>> late timestamp information arrival after that period of time should not
>> happen. However the truth is that it does happen and we want our driver
>> implementation to be resilient to this case rather than trusting the
>> time interval.
>> 
>
> In our case, because of how the slots work, once we "timeout" a slot, it
> could get re-used. We set the timeout to be pretty ridiculous (1 second)
> to ensure that if we do timeout its almost certainly because hardware
> never timestamped the packet.

We were thinking about that design as well. We use a 1 second timeout to
be safe.

  #define MLX5E_PTP_TS_CQE_UNDELIVERED_TIMEOUT (1 * NSEC_PER_SEC)

Our device does not do any bookkeeping internally to prevent a
completion event with timestamp information from arriving after 1
second. Some internal folks have said it shouldn't be possible, but we
did not want to take any chances and built a model that is resilient to
timestamp deliveries after any period of time even after consuming the
skb without appending timestamp information. If no other vendor finds
this useful, we could roll this up into the error counter and leave the
late counter as vendor specific. I do not want to introduce too many
counters that are hard to understand. We document the device specific
counters on top of introducing them in the code base already.

  https://docs.kernel.org/networking/device_drivers/ethernet/mellanox/mlx5/counters.html

>
>> 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.)

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.

  https://docs.kernel.org/networking/timestamping.html#hardware-timestamping-configuration-siocshwtstamp-and-siocghwtstamp

>
> There have been a few rare cases where things like a link event or
> issues with the MAC dropping a packet where the PHY simply never gets
> the packet and thus never timestamps it. This is typically the result of
> a lost timestamp.
>
> Flushed, for us, is when we reset the timestamp block while it has
> timestamps oustanding. This can happen for example due to link changes,
> where we ultimately
>
>>> tx_hwtstamp_flushed - indicates that we flushed an outstanding timestamp
>>> before it completed, such as if the link resets or similar.
>>> tx_hwtstamp_discarded - indicates that we obtained a timestamp from
>>> hardware but were unable to complete it due to invalid cached data used
>>> for timestamp extension.
>>>
>>> I think these could be translated roughly to one of the lost, late, or
>>> err stats. I am a bit confused as to how drivers could distinguish
>>> between lost and late, but I guess that depends on the specific hardware
>>> design.
>>>
>>> In theory we could keep some of these more detailed stats but I don't
>>> think we strictly need to be as detailed as the ice driver is.
>> 
>> We also converged a statistic in the mlx5 driver to the simple error
>> counter here. I think what makes sense is design specific counters
>> should be exposed as driver specific counters and more common counters
>> should be converged into the ethtool_ts_stats struct.
>> 
>
> Sure that seems reasonable.
>
>>>
>>> The only major addition I think is the skipped stat, which I would
>>> prefer to have. Perhaps that could be tracked in the netdev layer by
>>> checking whether the skb flags to see whether or not the driver actually
>>> set the appropriate flag?
>> 
>> I guess the problem is how would the core stack know at what layer this
>> was skipped at (I think Kory's patch series can be used to help with
>> this since it's adding a common interface in ethtool to select the
>> timestamping layer). As of today, mlx5 is the only driver I know of that
>> supports selecting between the DMA and PHY layers for timestamp
>> information.
>> 
>
> Well, the way the interface worked in my understanding was that the core
> sets the SKBTX_HW_TSTAMP flag. The driver is supposed to then prepare
> the packet for timestamp and set the SKBTX_IN_PROGRESS flag. I just
> looked though, and it looks like ice doesn't actually set this flag...

That would be a good fix. We set this in mlx5.

	/* device driver is going to provide hardware time stamp */
	SKBTX_IN_PROGRESS = 1 << 2,

>
> If we fixed this, in theory the stack should be able to check after the
> packet gets sent with SKBTX_HW_TSTAMP, if SKBTX_IN_PROGRESS isn't set
> then it would be a skipped timestamp?

One question I have about this idea. Wouldn't SKBTX_IN_PROGRESS also not
be set in the case when timestamp information is lost/a timeout occurs?
I feel like the problem is not being able to separate these two cases
from the perspective of the core stack.

Btw, mlx5 does keep the flag even when we fail to write timestamp
information..... I feel like it might be a good idea to add a warning in
the core stack if both SKBTX_HW_TSTAMP and SKBTX_IN_PROGRESS are set but
the skb is consumed without skb_hwtstamps(skb) being written by the
driver before consuming the skb.

>
> Its not really a huge deal, and this could just be lumped into either
> lost or err.

--
Thanks,

Rahul Rameshbabu

Powered by blists - more mailing lists