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