netdev - RE: [Intel-wired-lan] [PATCH v10 iwl-next 10/11] idpf: add Tx timestamp flows

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: <MW4PR11MB5889E02D47253C0951D1B4FB8EB72@MW4PR11MB5889.namprd11.prod.outlook.com>
Date: Thu, 10 Apr 2025 13:28:19 +0000
From: "Olech, Milena" <milena.olech@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Hay, Joshua A" <joshua.a.hay@...el.com>,
	"Salin, Samuel" <samuel.salin@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH v10 iwl-next 10/11] idpf: add Tx
 timestamp flows

On 4/8/2025 11:31 PM, Jacob Keller wrote:

>On 4/8/2025 3:31 AM, Milena Olech wrote:
>> +/**
>> + * idpf_ptp_tstamp_extend_32b_to_64b - Convert a 32b nanoseconds Tx or Rx
>> + *				       timestamp value to 64b.
>> + * @cached_phc_time: recently cached copy of PHC time
>> + * @in_timestamp: Ingress/egress 32b nanoseconds timestamp value
>> + *
>> + * Hardware captures timestamps which contain only 32 bits of nominal
>> + * nanoseconds, as opposed to the 64bit timestamps that the stack expects.
>> + *
>> + * Return: Tx timestamp value extended to 64 bits based on cached PHC time.
>> + */
>> +u64 idpf_ptp_tstamp_extend_32b_to_64b(u64 cached_phc_time, u32 in_timestamp)
>> +{
>> +	s64 delta;
>> +
>> +	delta = in_timestamp - lower_32_bits(cached_phc_time);
>> +
>> +	return cached_phc_time + delta;
>> +}
>
>
>This logic looks quite different from what we did in ice and iavf, which
>was based on the math from timecounters. It looks like you do check if
>the value is too old which is good to verify. Perhaps I'm just
>misunderstanding the math.
>
>For clarity, here's what we have in ice:
>
>> static u64 ice_ptp_extend_32b_ts(u64 cached_phc_time, u32 in_tstamp)
>> {
>>         u32 delta, phc_time_lo;
>>         u64 ns;
>> 
>>         /* Extract the lower 32 bits of the PHC time */
>>         phc_time_lo = (u32)cached_phc_time;
>> 
>>         /* Calculate the delta between the lower 32bits of the cached PHC
>>          * time and the in_tstamp value
>>          */
>>         delta = (in_tstamp - phc_time_lo);
>> 
>>         /* Do not assume that the in_tstamp is always more recent than the
>>          * cached PHC time. If the delta is large, it indicates that the
>>          * in_tstamp was taken in the past, and should be converted
>>          * forward.
>>          */
>>         if (delta > (U32_MAX / 2)) {
>>                 /* reverse the delta calculation here */
>>                 delta = (phc_time_lo - in_tstamp);
>>                 ns = cached_phc_time - delta;
>>         } else {
>>                 ns = cached_phc_time + delta;
>>         }
>> 
>>         return ns;
>> }
>
>
>In particular, this ensures that we correctly handle the case where a
>timestamp is captured just before an update to the cached PHC time.
>Without that check, you can't guarantee that the timestamp is updated
>correctly with lockess PHC updating.
>
>With these checks, as long as the timestamp is recent, we can extend it
>safely without worrying about whether the cached PHC time we are using
>is slightly old or not. (As long as its no more than 2 seconds old).
>
>Could you explain why this was changed for idpf?
>
>Bonus points if we extracted this method into libie/libeth and shared it
>across ice, idpf, and iavf, which I believe recently gained support for
>timestamping as well.
>

TBH I've started using the same algorithm at the beginning, but then I
tried to introduce some simplifications, and this is the final shape.
But after checking it again for both positive and negative delta I see
some discrepancies in timestamps values. So in the next version I'll
switch to well-known method from ice/iavf.

Milena