Received-SPF: pass (google.com: domain of linux-kernel+bounces-79282-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <a84df9ec-475d-4ffc-a975-a0911a57901e@intel.com>
Date: Fri, 23 Feb 2024 14:48:51 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC net-next v1 1/6] ethtool: add interface to read Tx
 hardware timestamping statistics
Content-Language: en-US
To: Rahul Rameshbabu <rrameshbabu@nvidia.com>
CC: Saeed Mahameed <saeed@kernel.org>, Leon Romanovsky <leon@kernel.org>,
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, "Jonathan
 Corbet" <corbet@lwn.net>, Richard Cochran <richardcochran@gmail.com>, "Tariq
 Toukan" <tariqt@nvidia.com>, Gal Pressman <gal@nvidia.com>, Vadim Fedorenko
	<vadim.fedorenko@linux.dev>, Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit
	<hkallweit1@gmail.com>, Przemek Kitszel <przemyslaw.kitszel@intel.com>,
	"Ahmed Zaki" <ahmed.zaki@intel.com>, Alexander Lobakin
	<aleksander.lobakin@intel.com>, Hangbin Liu <liuhangbin@gmail.com>, "Paul
 Greenwalt" <paul.greenwalt@intel.com>, Justin Stitt <justinstitt@google.com>,
	Randy Dunlap <rdunlap@infradead.org>, Maxime Chevallier
	<maxime.chevallier@bootlin.com>, Kory Maincent <kory.maincent@bootlin.com>,
	Wojciech Drewek <wojciech.drewek@intel.com>, Vladimir Oltean
	<vladimir.oltean@nxp.com>, Jiri Pirko <jiri@resnulli.us>, Alexandre Torgue
	<alexandre.torgue@foss.st.com>, Jose Abreu <joabreu@synopsys.com>, "Dragos
 Tatulea" <dtatulea@nvidia.com>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>
References: <20240223192658.45893-1-rrameshbabu@nvidia.com>
 <20240223192658.45893-2-rrameshbabu@nvidia.com>
 <abbff26c-c626-42ce-82a9-4dc983372de3@intel.com> <875xyex10q.fsf@nvidia.com>
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <875xyex10q.fsf@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?azJHZyswaThOeDZNTlBXenpTbFBmKytjN2ZnWkVOdmhwREZuRXJZcFNHOHdW?=
 =?utf-8?B?bGVtTnMzTUxvajdHNWZKSjFVc2lyOThBeUJ3OExDM3A3aUs2eEhlK3J0cEJG?=
 =?utf-8?B?bmN4VzMxcVBmakR2ajRyVk92eEdQaHFNL2dPWGRrYXlxL0hpc0RacGxVUDlI?=
 =?utf-8?B?WlVueHNzMXhWcUpYWWFDN215OFRhOElwR21sY3pxejVMUHZMYngyU3lpM0lU?=
 =?utf-8?B?QnoveE85UTFEOHhlOWgrcFE3K1ZUN0JoRUYrNXA4NGNjZTdVaXpIY0FjYlo3?=
 =?utf-8?B?VzNha0l3bHl3aVM1TW5KT05saHErNkxJUkRTOVR3S0pLSVNQL24vUEU1anNN?=
 =?utf-8?B?a2picnRYRWozaDNqczVZLzRGL0RJSVZYM05ZVkd4Z09ENk10Wlo3UUY0SGgv?=
 =?utf-8?B?UjNrQ1BhMGYxbHNGTnRHbzd5bjAraUorREdadmFwRjZwSWlSdEdMTXIxYkdI?=
 =?utf-8?B?SXBJd2JTaUp4UFB4TS9adzl0bmJXM2plcXJobWpuQ0MvQUhHS1pVZFNtODl0?=
 =?utf-8?B?VTZIdEllUnhFSXhDZ09iaGI0NVZ6LzllSXhzSXZZSmR0cXpPamFjTDBoU081?=
 =?utf-8?B?Z2EybXNLOXladHo5THBBMmVhUXFOUUswK0lpenkrcmxWU3ZGS0tUTkkxNldC?=
 =?utf-8?B?bWRVYXZEck1rMEtrK0UxVXp5cHJGdlpEamxsb1h1VndVMjJqWkRCWEtUQ3h0?=
 =?utf-8?B?SWZyQnpUUzkzQkJ0L2FPZ1VoWllNVVorbUpNVmZ2RlpVTVhRR1RBU0J2QzNa?=
 =?utf-8?B?NE91M2s2UnBnZ0loc294eHRQZXJ2c0dMaWZ1S3NqbU9ockNjWGRSenRLZDY5?=
 =?utf-8?B?eUszdFp3MEc5aDdUT3kwdUEyd0pWRDBrWlFhazR6VC92Tkx4dXEvaExJWnBo?=
 =?utf-8?B?N2NET1grMEFGc0EzNzhNZnpLV1U3MDZJY2pVRFR1SDRPRzBqUFY0SFZPS2Qx?=
 =?utf-8?B?TVpLL2J3b1ZkcFdzVFRyWnRsMThoelVXVnFOV0dONFFzelZLMW5aejMxc2RF?=
 =?utf-8?B?TkxBeWIyN1NqME9tKzkrbzJBSXBlZXo1M2NpS3ZGR1BBS0lxNGc3MENuQXFy?=
 =?utf-8?B?ajZ0T2g1dk44YmZkVDFhVUtuVERoVUpxRUovaWFhUUJkR3BZSDlaMUV0VGE5?=
 =?utf-8?B?MUp2ODhLd1VYRzNmRVJKRml0bSt3VjZkM1VPYWpZc1BydklENzhyaXRRZ0Yv?=
 =?utf-8?B?cGdJK0JtNis2MjNib1dXNlhuNm1MN2h5Z01XNU52TEgraW92d0xSbzNIc2ZF?=
 =?utf-8?B?OUZKT2dtUTdQclh6UUE0YmJTbmVOeGhyWWVRWkx6T0Zkay9OU0tkMWR3eUZZ?=
 =?utf-8?B?WlBCUi91R1UyVjNzc1JkbGh1RDRnVVlKd0lrdWQyV2VSUU9RNnRVSTRXNXM1?=
 =?utf-8?B?RElIYXRhV3ZXWC9ycFU2b1RvcExsWGlEc1NtS1ZOSHRCVTB6dXlaWFlra1pR?=
 =?utf-8?B?S2d6WEkrUjU2alJYZ0lqaFBLR09zUXl0WVpYaENVemNwdHVsdENsNndpNnlI?=
 =?utf-8?B?dWUxSUZJeE0ybS9PUHFCampqSUxSQXdMc21VRTlKSERPUTdxaEpjWVRGWEl2?=
 =?utf-8?B?NU40V3dLdExMQm1BMUtDM0N1NDByMTlLbU5CMnJZV3Z5Mkw2RElHdlZTWTlO?=
 =?utf-8?B?eHEra29rTnVNUnFJandMQnR3YzdjZ09UR3p1QzVQR0dyM1Z5L09qdGZROGVR?=
 =?utf-8?B?MGFqcnh6a0hIckZFNW9PbmN1ZkdJTk5sWENHd2tDZmNqZ3g3RUNuOGFWTmJx?=
 =?utf-8?B?bXg1a1hoa3ZETWFIZEY5WmU3a2F3ZTNXOWN5dHY2NmMyUlNnQlJielRUdmZP?=
 =?utf-8?B?cjk5ZzA1YXB5V2xTK2dKQ3A5ckdEaFpQWHVMMnFFQnB1QWNyQUxLbTF0clNt?=
 =?utf-8?B?WE1ycnMvRExtSU82QjUxR3dEZFM3QzZVZHhBUk9tK1NhUS9iNlRvcmlRbjBR?=
 =?utf-8?B?dkIzUUkzRW5rQ2plQzI3YldjQytQQWpDYzN4MG1nd0pNbnFLUE5LQVYrYVQv?=
 =?utf-8?B?dWZEV1NBT0dsd09UY20wT2JoN0JadDBQVkV6NFVoSFRoa1R3alE0MzVNVXFV?=
 =?utf-8?B?ck54ZERzM3dGSFlXVXIzR1UvL2YzUU1JK2RHS1kyOXlrWlM3UGEyS2lXMDBz?=
 =?utf-8?B?U3AvUG9jSW01a0JiWGd0TWlVaWlPZlV3cGlhSTY3RW1kSnJjWkllTUovWlB5?=
 =?utf-8?B?RUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 05bbd331-1a00-410c-7fc8-08dc34c19c76
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Feb 2024 22:48:53.9634
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SY45Ols9vFHM+fHq5Njivauz1UKpoU6cUeUv6NqwZkrbzSIhbikqZH84RiLsk4gw1g+F0Xr2rRKdT2pcgXssc+kYYiG40LRkb7xDn443YRY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6762
X-OriginatorOrg: intel.com


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.

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

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

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?

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