netdev - Re: [PATCH net-next v3 2/5] eth: fbnic: add initial PHC support

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: <a836c401-d071-42b2-9d2f-45d821941286@intel.com>
Date: Mon, 7 Oct 2024 16:49:45 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Vadim Fedorenko
	<vadim.fedorenko@...ux.dev>
CC: Vadim Fedorenko <vadfed@...a.com>, David Ahern <dsahern@...nel.org>,
	"Paolo Abeni" <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>,
	"Alexander Duyck" <alexanderduyck@...com>, <netdev@...r.kernel.org>, Richard
 Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next v3 2/5] eth: fbnic: add initial PHC support



On 10/7/2024 4:09 PM, Jakub Kicinski wrote:
> On Mon, 7 Oct 2024 14:07:17 +0100 Vadim Fedorenko wrote:
>> On 05/10/2024 00:05, Jacob Keller wrote:
>>> On 10/3/2024 5:39 AM, Vadim Fedorenko wrote:  
>>>> +/* FBNIC timing & PTP implementation
>>>> + * Datapath uses truncated 40b timestamps for scheduling and event reporting.
>>>> + * We need to promote those to full 64b, hence we periodically cache the top
>>>> + * 32bit of the HW time counter. Since this makes our time reporting non-atomic
>>>> + * we leave the HW clock free running and adjust time offsets in SW as needed.
>>>> + * Time offset is 64bit - we need a seq counter for 32bit machines.
>>>> + * Time offset and the cache of top bits are independent so we don't need
>>>> + * a coherent snapshot of both - READ_ONCE()/WRITE_ONCE() + writer side lock
>>>> + * are enough.
>>>> + */
>>>> +  
>>>
>>> If you're going to implement adjustments only in software anyways, can
>>> you use a timecounter+cyclecounter instead of re-implementing?  
>>
>> Thanks for pointing this out, I'll make it with timecounter/cyclecounter
> 
> Please don't, the clock is synthonized, we only do simple offsetting.
> 
I still think it makes sense to re-use the logic for converting cycles
to full 64bit time values if possible.

If you're already doing offset adjustment, you still have to apply the
same logic to every timestamp, which is exactly what a timecounter does
for you.

You can even use a timecounter and cyclecounter without using its
ability to do syntonizing, by just setting the cyclecounter values
appropriately, and leaving the syntonizing to the hardware mechanism.

I think the result is much easier to understand and follow than
re-implementing a different mechanism for offset correction with bespoke
code.