Re: [Intel-wired-lan] [PATCH iwl-next v5 08/12] iavf: periodically cache PHC time

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Thu, 25 Apr, 2024 16:28:22 +0000 "Keller, Jacob E" <jacob.e.keller@intel.com> wrote:
>> -----Original Message-----
>> From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
>> Sent: Thursday, April 25, 2024 3:52 AM
>> To: Keller, Jacob E <jacob.e.keller@intel.com>; Polchlopek, Mateusz
>> <mateusz.polchlopek@intel.com>; Rahul Rameshbabu
>> <rrameshbabu@nvidia.com>
>> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; horms@kernel.org;
>> Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Drewek, Wojciech
>> <wojciech.drewek@intel.com>
>> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v5 08/12] iavf: periodically cache
>> PHC time
>> 
>> On 4/25/24 00:03, Keller, Jacob E wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Polchlopek, Mateusz <mateusz.polchlopek@intel.com>
>> >> Sent: Monday, April 22, 2024 2:23 AM
>> >> To: Rahul Rameshbabu <rrameshbabu@nvidia.com>
>> >> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
>> horms@kernel.org;
>> >> Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Keller, Jacob E
>> >> <jacob.e.keller@intel.com>; Drewek, Wojciech <wojciech.drewek@intel.com>
>> >> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v5 08/12] iavf: periodically cache
>> >> PHC time
>> >>
>> >>
>> >>
>> >> On 4/18/2024 9:51 PM, Rahul Rameshbabu wrote:
>> >>> On Thu, 18 Apr, 2024 01:24:56 -0400 Mateusz Polchlopek
>> >> <mateusz.polchlopek@intel.com> wrote:
>> >>>> From: Jacob Keller <jacob.e.keller@intel.com>
>> >>>>
>> >>>> The Rx timestamps reported by hardware may only have 32 bits of storage
>> >>>> for nanosecond time. These timestamps cannot be directly reported to the
>> >>>> Linux stack, as it expects 64bits of time.
>> >>>>
>> >>>> To handle this, the timestamps must be extended using an algorithm that
>> >>>> calculates the corrected 64bit timestamp by comparison between the PHC
>> >>>> time and the timestamp. This algorithm requires the PHC time to be
>> >>>> captured within ~2 seconds of when the timestamp was captured.
>> >>>>
>> >>>> Instead of trying to read the PHC time in the Rx hotpath, the algorithm
>> >>>> relies on a cached value that is periodically updated.
>> >>>>
>> >>>> Keep this cached time up to date by using the PTP .do_aux_work kthread
>> >>>> function.
>> >>>
>> >>> Seems reasonable.
>> >>>
>> >>>>
>> >>>> The iavf_ptp_do_aux_work will reschedule itself about twice a second,
>> >>>> and will check whether or not the cached PTP time needs to be updated.
>> >>>> If so, it issues a VIRTCHNL_OP_1588_PTP_GET_TIME to request the time
>> >>>> from the PF. The jitter and latency involved with this command aren't
>> >>>> important, because the cached time just needs to be kept up to date
>> >>>> within about ~2 seconds.
>> >>>>
>> >>>> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> >>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> >>>> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> >>>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> >>>> ---
>> >>>>    drivers/net/ethernet/intel/iavf/iavf_ptp.c | 52
>> ++++++++++++++++++++++
>> >>>>    drivers/net/ethernet/intel/iavf/iavf_ptp.h |  1 +
>> >>>>    2 files changed, 53 insertions(+)
>> >>>>
>> >>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.c
>> >> b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
>> >>> <snip>
>> >>>> +/**
>> >>>> + * iavf_ptp_do_aux_work - Perform periodic work required for PTP support
>> >>>> + * @ptp: PTP clock info structure
>> >>>> + *
>> >>>> + * Handler to take care of periodic work required for PTP operation. This
>> >>>> + * includes the following tasks:
>> >>>> + *
>> >>>> + *   1) updating cached_phc_time
>> >>>> + *
>> >>>> + *      cached_phc_time is used by the Tx and Rx timestamp flows in order to
>> >>>> + *      perform timestamp extension, by carefully comparing the timestamp
>> >>>> + *      32bit nanosecond timestamps and determining the corrected 64bit
>> >>>> + *      timestamp value to report to userspace. This algorithm only works if
>> >>>> + *      the cached_phc_time is within ~1 second of the Tx or Rx timestamp
>> >>>> + *      event. This task periodically reads the PHC time and stores it, to
>> >>>> + *      ensure that timestamp extension operates correctly.
>> >>>> + *
>> >>>> + * Returns: time in jiffies until the periodic task should be re-scheduled.
>> >>>> + */
>> >>>> +long iavf_ptp_do_aux_work(struct ptp_clock_info *ptp)
>> >>>> +{
>> >>>> +	struct iavf_adapter *adapter = clock_to_adapter(ptp);
>> >>>> +
>> >>>> +	iavf_ptp_cache_phc_time(adapter);
>> >>>> +
>> >>>> +	/* Check work about twice a second */
>> >>>> +	return msecs_to_jiffies(500);
>> >>>
>> >>> HZ / 2 might be more intuitive?
>> >>>
>> >
>> > I've always found it more intuitive to think in terms of msecs myself, but HZ / 2 is
>> ok if other folks agree.
>> 
>> HZ/2 or HZ/3 as a timer period could be understood without thinking, but
>> the same stands for 400ms. Problems starts when one thinks about it ;)
>> 
>> For me HZ, which could be both literally and colloquially understood as
>> "per second" should not mean 1000ms (just evaluate to).
>> 2Hz is a frequency with half second period, but 2*HZ evaluates to 2000ms
>> which is 4 times more :/
>> 
>
> That’s part of why I switched ice over from using HZ generally to using
> jiffies_to_msec in a lot of cases. It really depends on what you personally find
> intuitive. Those used to seeing and reading HZ may find it easier.
>

Makes sense to stick with the same if ice is using jiffies_to_msec in
general. I, recently, was re-reading the Linux Device Drivers book,
which has a section that elaborates on HZ a bit.

--
Thanks,

Rahul Rameshbabu