| Message ID | 20221003095519.539390-2-karol.kolacinski@intel.com |
|---|---|
| State | Rejected |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [net-next,1/2] ice: Check for PTP HW lock more frequently | expand |
On 10/3/2022 2:55 AM, Karol Kolacinski wrote: > Reading the time should not block other accesses to the PTP hardware. > There isn't a significant risk of reading bad values while another > thread is modifying the clock. Removing the hardware lock around the > gettime allows multiple application threads to read the clock time with > less contention. > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > --- Vinicius had a comment/concern on this. Adding him to this and bringing the comment over. " I think the problem is less about concurrent writes/reads than concurrent reads: the fact that the registers are latched when the "lower" register is read, makes me worried that there's a (very narrow) window during rollover in which the "losing" read (of multiple threads doing reads) can return a wrong value. I could be missing something. " Thanks, Tony > drivers/net/ethernet/intel/ice/ice_ptp.c | 31 +++--------------------- > 1 file changed, 3 insertions(+), 28 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > index 9d1afeca9624..1f481d455134 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -979,26 +979,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf) > ice_ptp_flush_tx_tracker(pf, &pf->ptp.port.tx); > } > > -/** > - * ice_ptp_read_time - Read the time from the device > - * @pf: Board private structure > - * @ts: timespec structure to hold the current time value > - * @sts: Optional parameter for holding a pair of system timestamps from > - * the system clock. Will be ignored if NULL is given. > - * > - * This function reads the source clock registers and stores them in a timespec. > - * However, since the registers are 64 bits of nanoseconds, we must convert the > - * result to a timespec before we can return. > - */ > -static void > -ice_ptp_read_time(struct ice_pf *pf, struct timespec64 *ts, > - struct ptp_system_timestamp *sts) > -{ > - u64 time_ns = ice_ptp_read_src_clk_reg(pf, sts); > - > - *ts = ns_to_timespec64(time_ns); > -} > - > /** > * ice_ptp_write_init - Set PHC time to provided value > * @pf: Board private structure > @@ -1835,15 +1815,10 @@ ice_ptp_gettimex64(struct ptp_clock_info *info, struct timespec64 *ts, > struct ptp_system_timestamp *sts) > { > struct ice_pf *pf = ptp_info_to_pf(info); > - struct ice_hw *hw = &pf->hw; > + u64 time_ns; > > - if (!ice_ptp_lock(hw)) { > - dev_err(ice_pf_to_dev(pf), "PTP failed to get time\n"); > - return -EBUSY; > - } > - > - ice_ptp_read_time(pf, ts, sts); > - ice_ptp_unlock(hw); > + time_ns = ice_ptp_read_src_clk_reg(pf, sts); > + *ts = ns_to_timespec64(time_ns); > > return 0; > }
Hi Vinicius, > I think the problem is less about concurrent writes/reads than > concurrent reads: the fact that the registers are latched when the > "lower" register is read, makes me worried that there's a (very narrow) > window during rollover in which the "losing" read (of multiple threads > doing reads) can return a wrong value. The issue in this case is, it's either risk of reading slightly wrong value or having multiple timeouts and errors. We experienced a lot of simultaneous reads on multiple PFs (especially on E822 HW with 8 ports) and even with increased timeout to acquire the HW semaphore, it still failed. Thanks, Karol
"Kolacinski, Karol" <karol.kolacinski@intel.com> writes: > Hi Vinicius, > >> I think the problem is less about concurrent writes/reads than >> concurrent reads: the fact that the registers are latched when the >> "lower" register is read, makes me worried that there's a (very narrow) >> window during rollover in which the "losing" read (of multiple threads >> doing reads) can return a wrong value. > > The issue in this case is, it's either risk of reading slightly wrong > value or having multiple timeouts and errors. > We experienced a lot of simultaneous reads on multiple PFs (especially > on E822 HW with 8 ports) and even with increased timeout to acquire > the HW semaphore, it still failed. I am wondering if using a hw semaphore is making the problem worse than it needs to be. Why a kernel spinlock can't be used? Cheers,
On 10/5/2022 2:10 PM, Vinicius Costa Gomes wrote: > "Kolacinski, Karol" <karol.kolacinski@intel.com> writes: > >> Hi Vinicius, >> >>> I think the problem is less about concurrent writes/reads than >>> concurrent reads: the fact that the registers are latched when the >>> "lower" register is read, makes me worried that there's a (very narrow) >>> window during rollover in which the "losing" read (of multiple threads >>> doing reads) can return a wrong value. >> >> The issue in this case is, it's either risk of reading slightly wrong >> value or having multiple timeouts and errors. >> We experienced a lot of simultaneous reads on multiple PFs (especially >> on E822 HW with 8 ports) and even with increased timeout to acquire >> the HW semaphore, it still failed. > > I am wondering if using a hw semaphore is making the problem worse than > it needs to be. Why a kernel spinlock can't be used? > > > Cheers, The same clock is shared across multiple ports which operate as independent PCIe devices, hence having their own instance of the ice driver structures. A spinlock doesn't work because they wouldn't be using the same lock. We could try to share the lock in software between PFs, but its actually quite difficult to do that with the existing PCIe driver model.
Jacob Keller <jacob.e.keller@intel.com> writes: > On 10/5/2022 2:10 PM, Vinicius Costa Gomes wrote: >> "Kolacinski, Karol" <karol.kolacinski@intel.com> writes: >> >>> Hi Vinicius, >>> >>>> I think the problem is less about concurrent writes/reads than >>>> concurrent reads: the fact that the registers are latched when the >>>> "lower" register is read, makes me worried that there's a (very narrow) >>>> window during rollover in which the "losing" read (of multiple threads >>>> doing reads) can return a wrong value. >>> >>> The issue in this case is, it's either risk of reading slightly wrong >>> value or having multiple timeouts and errors. >>> We experienced a lot of simultaneous reads on multiple PFs (especially >>> on E822 HW with 8 ports) and even with increased timeout to acquire >>> the HW semaphore, it still failed. >> >> I am wondering if using a hw semaphore is making the problem worse than >> it needs to be. Why a kernel spinlock can't be used? >> >> >> Cheers, > > The same clock is shared across multiple ports which operate as > independent PCIe devices, hence having their own instance of the ice > driver structures. A spinlock doesn't work because they wouldn't be > using the same lock. > Oh! I should have realized that. The thought that there could be multiple devices/ports sharing some resources didn't cross my mind. > We could try to share the lock in software between PFs, but its actually > quite difficult to do that with the existing PCIe driver model. I can see how that would be difficult, yeah. Did you happen to test if my fears were true or not? For example, 'phc2sys' running in parallel with a few (4?) 'while true { phc_ctl get }'. Do you notice any weirdness? Cheers,
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Karol Kolacinski > Sent: Monday, October 3, 2022 3:25 PM > To: intel-wired-lan@lists.osuosl.org > Cc: Kolacinski, Karol <karol.kolacinski@intel.com> > Subject: [Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW > semaphore > > Reading the time should not block other accesses to the PTP hardware. > There isn't a significant risk of reading bad values while another thread is > modifying the clock. Removing the hardware lock around the gettime allows > multiple application threads to read the clock time with less contention. > > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ptp.c | 31 +++--------------------- > 1 file changed, 3 insertions(+), 28 deletions(-) > Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 9d1afeca9624..1f481d455134 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -979,26 +979,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf) ice_ptp_flush_tx_tracker(pf, &pf->ptp.port.tx); } -/** - * ice_ptp_read_time - Read the time from the device - * @pf: Board private structure - * @ts: timespec structure to hold the current time value - * @sts: Optional parameter for holding a pair of system timestamps from - * the system clock. Will be ignored if NULL is given. - * - * This function reads the source clock registers and stores them in a timespec. - * However, since the registers are 64 bits of nanoseconds, we must convert the - * result to a timespec before we can return. - */ -static void -ice_ptp_read_time(struct ice_pf *pf, struct timespec64 *ts, - struct ptp_system_timestamp *sts) -{ - u64 time_ns = ice_ptp_read_src_clk_reg(pf, sts); - - *ts = ns_to_timespec64(time_ns); -} - /** * ice_ptp_write_init - Set PHC time to provided value * @pf: Board private structure @@ -1835,15 +1815,10 @@ ice_ptp_gettimex64(struct ptp_clock_info *info, struct timespec64 *ts, struct ptp_system_timestamp *sts) { struct ice_pf *pf = ptp_info_to_pf(info); - struct ice_hw *hw = &pf->hw; + u64 time_ns; - if (!ice_ptp_lock(hw)) { - dev_err(ice_pf_to_dev(pf), "PTP failed to get time\n"); - return -EBUSY; - } - - ice_ptp_read_time(pf, ts, sts); - ice_ptp_unlock(hw); + time_ns = ice_ptp_read_src_clk_reg(pf, sts); + *ts = ns_to_timespec64(time_ns); return 0; }
Reading the time should not block other accesses to the PTP hardware. There isn't a significant risk of reading bad values while another thread is modifying the clock. Removing the hardware lock around the gettime allows multiple application threads to read the clock time with less contention. Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> --- drivers/net/ethernet/intel/ice/ice_ptp.c | 31 +++--------------------- 1 file changed, 3 insertions(+), 28 deletions(-)