Re: [net-next 03/15] net/mlx5: Add adjphase function to support hardware-only offset control

From: Rahul Rameshbabu <rrameshbabu@nvidia.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Jacob Keller <jacob.e.keller@intel.com>,
	Saeed Mahameed <saeed@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>,
	Saeed Mahameed <saeedm@nvidia.com>, <netdev@vger.kernel.org>,
	Tariq Toukan <tariqt@nvidia.com>, Gal Pressman <gal@nvidia.com>,
	Richard Cochran <richardcochran@gmail.com>,
	Vincent Cheng <vincent.cheng.xh@renesas.com>
Subject: Re: [net-next 03/15] net/mlx5: Add adjphase function to support hardware-only offset control
Date: Sun, 22 Jan 2023 13:11:57 -0800	[thread overview]
Message-ID: <87ilgyw9ya.fsf@nvidia.com> (raw)
In-Reply-To: <20230120160609.19160723@kernel.org> (Jakub Kicinski's message of "Fri, 20 Jan 2023 16:06:09 -0800")

Held off on replying any further till I verified my understanding with a
PTP architect on our side. Looks like my understanding was correct.

On Fri, 20 Jan, 2023 16:06:09 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri, 20 Jan 2023 15:58:25 -0800 Jacob Keller wrote:
>> Sure. I guess what I don't understand is why or when one would want to
>> use adjphase instead of just performing frequency adjustment via the
>> .adjfine operation...
>> 
>> Especially since "gradual adjustment over time" means it will still be
>> slow to converge (just like adjusting frequency is today).
>> 
>> We should definitely improve the doc to explain the diff between them
>> and make sure that its more clear to driver implementations.

Our PTP architect also was subtly confused by the differences of adjtime
and adjphase when the adjphase patch was first introduced. Vincent's
follow up helped with better understanding the difference.

The way NVIDIA devices internally handle adjphase is to adjust the
frequency for a short period of time to make the small time offset
adjustments smooth (using some internal calculations) where the time
offset "nudge" is applied but frequency is also adjusted to prevent
immediate drift after that time adjustment. However, we aren't sure if
this is the only approach possible to achieve accurate corrections for
small offset adjustments with adjphase, so I would suggest that the
documentation be updated to state something discussing that adjphase is
expected to support small jumps in offset precisely without necessarily
bringing up frequency manipulation potentially done to achieve this.

>
> Fair point, I assumed that the adjustment is precise (as in the device
> is programmed with both the extra addend and number of cycles over
> which to use it). Yielding an exact adjustment.
>
> But then again, I also thought that .adjtime is supposed to be a precise
> single-shot nudge, while most drivers just do:
>
> time = read_time()
> time += delta
> write_time(time)
>
> :S  So yeah, let's document..
>

Our architect used the following to compare adjtime and adjphase.

  time adjustment is a 'jump' in time while phase adjustment is a smooth correction.

I think the thing though is how that smooth correction for small offset
values is achieved is not very well defined/something the implementer
can define. Will update the comments for adjphase in my patch series
based on this.

>> It also makes it harder to justify mapping small .adjtime to .adjphase,
>> as it seems like .adjphase isn't required to adjust the offset
>> immediately. Perhaps the adjustment size is small enough that isn't a
>> big problem?