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

From: Rahul Rameshbabu <[email-masked]>
Date: Thu, 25 Apr 2024 10:10:56 -0700

On Thu, 25 Apr, 2024 16:28:22 +0000 "Keller, Jacob E" <[email-masked]> wrote:
>> -----Original Message-----
>> From: Kitszel, Przemyslaw <[email-masked]>
>> Sent: Thursday, April 25, 2024 3:52 AM
>> To: Keller, Jacob E <[email-masked]>; Polchlopek, Mateusz
>> <[email-masked]>; Rahul Rameshbabu
>> <[email-masked]>
>> Cc: [email-masked]; [email-masked]; [email-masked];
>> Nguyen, Anthony L <[email-masked]>; Drewek, Wojciech
>> <[email-masked]>
>> 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 <[email-masked]>
>> >> Sent: Monday, April 22, 2024 2:23 AM
>> >> To: Rahul Rameshbabu <[email-masked]>
>> >> Cc: [email-masked]; [email-masked];
>> [email-masked];
>> >> Nguyen, Anthony L <[email-masked]>; Keller, Jacob E
>> >> <[email-masked]>; Drewek, Wojciech <[email-masked]>
>> >> 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
>> >> <[email-masked]> wrote:
>> >>>> From: Jacob Keller <[email-masked]>
>> >>>>
>> >>>> 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 <[email-masked]>
>> >>>> Signed-off-by: Jacob Keller <[email-masked]>
>> >>>> Co-developed-by: Mateusz Polchlopek <[email-masked]>
>> >>>> Signed-off-by: Mateusz Polchlopek <[email-masked]>
>> >>>> ---
>> >>>>    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