> -----Original Message----- > From: Intel-wired-lan <[email protected]> On Behalf Of > Michal Schmidt > Sent: Tuesday, March 26, 2024 4:51 AM > To: [email protected] > Cc: Jiri Pirko <[email protected]>; Temerkhanov, Sergey > <[email protected]>; [email protected]; Richard Cochran > <[email protected]>; Kubalewski, Arkadiusz > <[email protected]>; Kolacinski, Karol > <[email protected]>; Marcin Szycik <[email protected]>; > Nguyen, Anthony L <[email protected]>; Kitszel, Przemyslaw > <[email protected]>; Keller, Jacob E <[email protected]> > Subject: [Intel-wired-lan] [PATCH net-next v4 2/3] ice: avoid the PTP > hardware semaphore in gettimex64 path > > The PTP hardware semaphore (PFTSYN_SEM) is used to synchronize operations > that program the PTP timers. The operations involve issuing commands to the > sideband queue. The E810 does not have a hardware sideband queue, so the > admin queue is used. The admin queue is slow. > I have observed delays in hundreds of milliseconds waiting for ice_sq_done. > > When phc2sys reads the time from the ice PTP clock and PFTSYN_SEM is held by > a task performing one of the slow operations, ice_ptp_lock can easily time > out. phc2sys gets -EBUSY and the kernel prints: > ice 0000:XX:YY.0: PTP failed to get time These messages appear once every > few seconds, causing log spam. > > The E810 datasheet recommends an algorithm for reading the upper 64 bits of > the GLTSYN_TIME register. It matches what's implemented in > ice_ptp_read_src_clk_reg. It is robust against wrap-around, but not > necessarily against the concurrent setting of the register (with > GLTSYN_CMD_{INIT,ADJ}_TIME commands). Perhaps that's why > ice_ptp_gettimex64 also takes PFTSYN_SEM. > > The race with time setters can be prevented without relying on the PTP > hardware semaphore. Using the "ice_adapter" from the previous patch, we can > have a common spinlock for the PFs that share the clock hardware. > It will protect the reading and writing to the GLTSYN_TIME register. > The writing is performed indirectly, by the hardware, as a result of the > driver writing GLTSYN_CMD_SYNC in ice_ptp_exec_tmr_cmd. I wasn't sure if the > ice_flush there is enough to make sure GLTSYN_TIME has been updated, but it > works well in my testing. > > My test code can be seen here: > https://gitlab.com/mschmidt2/linux/-/commits/ice-ptp-host-side-lock-10 > It consists of: > - kernel threads reading the time in a busy loop and looking at the > deltas between consecutive values, reporting new maxima. > - a shell script that sets the time repeatedly; > - a bpftrace probe to produce a histogram of the measured deltas. > Without the spinlock ptp_gltsyn_time_lock, it is easy to see tearing. > Deltas in the [2G, 4G) range appear in the histograms. > With the spinlock added, there is no tearing and the biggest delta I saw was > in the range [1M, 2M), that is under 2 ms. > > Reviewed-by: Jacob Keller <[email protected]> > Reviewed-by: Przemek Kitszel <[email protected]> > Signed-off-by: Michal Schmidt <[email protected]> > --- > drivers/net/ethernet/intel/ice/ice_adapter.c | 2 ++ > drivers/net/ethernet/intel/ice/ice_adapter.h | 6 ++++++ > drivers/net/ethernet/intel/ice/ice_ptp.c | 8 +------- > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 3 +++ > 4 files changed, 12 insertions(+), 7 deletions(-) >
Tested-by: Pucha Himasekhar Reddy <[email protected]> (A Contingent worker at Intel)
