RE: [RFC v3 0/3] add frequency adjustment support for PTP timesync

["Su, Simei" <simei.su@intel.com>]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, June 8, 2023 2:29 AM
> To: Su, Simei <simei.su@intel.com>; thomas@monjalon.net;
> andrew.rybchenko@oktetlabs.ru; Rybalchenko, Kirill
> <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Subject: Re: [RFC v3 0/3] add frequency adjustment support for PTP timesync
> 
> On 6/7/2023 9:19 AM, Su, Simei wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Saturday, June 3, 2023 3:44 AM
> >> To: Su, Simei <simei.su@intel.com>; thomas@monjalon.net;
> >> andrew.rybchenko@oktetlabs.ru; Rybalchenko, Kirill
> >> <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>
> >> Subject: Re: [RFC v3 0/3] add frequency adjustment support for PTP
> >> timesync
> >>
> >> On 5/22/2023 2:23 PM, Simei Su wrote:
> >>> This patchset cover below parts:
> >>> (1)Introduce a new timesync API called "rte_eth_timesync_adjust_freq"
> >> that
> >>>    enables frequency adjustment during PTP timesync. This new API
> >>> aligns
> >> with
> >>>    the kernel PTP which already supports frequency adjustment. It
> >>> brings
> >> DPDK
> >>>    closer in alignment with the kernel's best practice.
> >>>
> >>
> >> Hi Simei,
> >>
> >> I am not expert on PTP, so please help me understand,
> >>
> >> I can see there is a new API to update device frequency by updating
> >> device clock, 'rte_eth_timesync_adjust_fine()', that part is OK.
> >> (Although I have some reservation with the API name, we can visit it
> >> later.)
> >
> > Yes, this API is to adjust the frequency in a finer granularity, so naming it as
> 'rte_eth_timesync_adjust_fine()'.
> >
> 
> Can existing 'rte_eth_timesync_adjust_time()' API also used to adjust the
> device clock frequency?

' rte_eth_timesync_adjust_time()' API focuses on phase adjustment while
' rte_eth_timesync_adjust_fine()' API focused on frequency adjustment.
They focus on different aspects.

> 
> If so, since 'fine' is subjective, how user can decide to use one or other, and
> why we can't use existing API?

Previously, we only use ' rte_eth_timesync_adjust_time()' API to do time synchronization,
but the accuracy is low. We found the gap maybe missing frequency adjustment
which is already supported in kernel driver and linuxptp application.
So we implement it to bring DPDK closer in alignment with the kernel's best practice and
find the accuracy has a significant improvement.

E.g, below is a simple usage:
if master offset > master offset threshold
   do rte_eth_timesync_adjust_time;
else
   do rte_eth_timesync_adjust_fine;.

> 
> >>
> >> PTP doesn't enforce any specific sync method, right?
> >> Like adjusting by adding/subtracting or adjusting by updating clock
> frequency.
> >> As far as I understand updating device clock frequency is HW
> >> capability to adjust time, perhaps in a finer grain, is this correct?
> >
> > Yes, it's correct.
> >
> >>
> >> If so, is this HW capability should be exposed to user with a new API?
> >> If this is a generic HW capability, I think it is OK to have single
> >> responsibility API, but if this is a vendor specific feature it should be behind
> a generic API.
> >
> > It's a generic HW capability instead of vendor specific feature.
> >
> >>
> >>
> >> Or should we hide this capability behind existing
> >> 'rte_eth_timesync_adjust_time()' API, and HW/driver use best possible
> >> method for synchronization, so this can be transparent the user.
> >> What is the benefit to make user selected explicitly one option or other?
> >>
> >
> > In general, 'rte_eth_timesync_adjust_fine()' is used in conjunction
> > with 'rte_eth_timesync_adjust_time()' at different moment to do more
> precise time control.
> >
> 
> If I understand this is not defined by PTP control, so how user can do
> distinction between two?
> 
> Or same argument again, can user call same existing API and this detail can be
> hidden behind implementation? Meaning driver/HW selects that moment to
> call one or other.

In summary, the new API is introduced because of below consideration:
(1) align with kernel driver and linuxptp application
(2) it's not vendor specific API but generic API
(3) refine current algorithm in ptpclient application to improve time accuracy
(4) the focus is different from ' rte_eth_timesync_adjust_time'

In order not to destroy original framework in ptpclient application and let it can be used by other PMDs which is without frequency adjustment for normal use,
we add --controller=pi in starting command to select using PI algorithm to improve accuracy. Because besides PI algorithm, other linear algorithm can
be used for it.

Thanks,
Simei

> 
> >>
> >>
> >>> (2)Refine the ptpclient application by applying a PI algorithm that
> leverages
> >>>    the new API to further improve timesync accuracy. These changes
> >> doesn't break
> >>>    original solution and creates a more accurate solution for
> >>> DPDK-based
> >> high
> >>>    accuracy PTP. We have provided significant improvements for
> >>> timesync
> >> accuracy
> >>>    on e810 and we believe these improvements will also benefit other
> >> devices.
> >>>
> >>
> >> What is "PI algorithm"? and what PI stands for?
> >
> > Proportional-integral-derivative control is called PID controller. It is widely
> used in industrial control.
> > The control deviation is constituted based on the given value and the actual
> output value.
> > The deviation produces control quantity based on proportional, integrated
> and differentiated linear combination, thereby controlling the controlled
> object.
> >
> > 'P' means proportional controller. It changes the magnitude only.
> > 'I' means integral controller. It lags the output phase.
> > 'PI' means proportional Integral controller. It changes the magnitude as well
> as laging the output.
> >
> > Proportional-Integral(PI) controller is used to produce a fractional
> > tick-rate adjustment that coordinates the local clock with master
> > clock time. The PI controller corrects both the time and rate of the
> > local clock. The proportional term tracks and corrects the direct input,
> which is the time difference between two clocks. The integral term tracks and
> corrects steady-state error, which is the rate difference between two clocks.
> >
> 
> Thank you.
> 
> > Thanks,
> > Simei
> >
> >>
> >>> The original command for starting ptpclient is:
> >>> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
> >>> 0x1
> >>>
> >>> The command with PI algorithm is:
> >>> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
> >>> 0x1 -- controller=pi
> >>>
> >>> [RFC v3 1/3] ethdev: add frequency adjustment API.
> >>> [RFC v3 2/3] examples/ptpclient: refine application.
> >>> [RFC v3 3/3] examples/ptpclient: add frequency adjustment support.
> >>>
> >>> v3:
> >>> * Refine commit log in patch.
> >>> * Add more description for the new API.
> >>>
> >>> v2:
> >>> * Remove the ice PMD part from the RFC.
> >>> * Add description in cover letter.
> >>> * Refine commit log in patch.
> >>>
> >>> Simei Su (3):
> >>>   ethdev: add frequency adjustment API
> >>>   examples/ptpclient: refine application
> >>>   examples/ptpclient: add frequency adjustment support
> >>>
> >>>  examples/ptpclient/ptpclient.c   | 222
> >> +++++++++++++++++++++++++++++++++------
> >>>  lib/ethdev/ethdev_driver.h       |   5 +
> >>>  lib/ethdev/ethdev_trace.h        |   9 ++
> >>>  lib/ethdev/ethdev_trace_points.c |   3 +
> >>>  lib/ethdev/rte_ethdev.c          |  19 ++++
> >>>  lib/ethdev/rte_ethdev.h          |  38 +++++++
> >>>  6 files changed, 265 insertions(+), 31 deletions(-)
> >>>
> >