[Intel-wired-lan] [PATCH net-next 1/2] ice: Check for PTP HW lock more frequently

[Intel-wired-lan] [PATCH net-next 1/2] ice: Check for PTP HW lock more frequently

[Karol Kolacinski]

It was observed that PTP HW semaphore can be held for ~50 ms in worst
case.
SW should wait longer and check more frequently if the HW lock is held.

Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 772b1f566d6e..1f8dd50db524 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -2963,16 +2963,18 @@ bool ice_ptp_lock(struct ice_hw *hw)
 	u32 hw_lock;
 	int i;
 
-#define MAX_TRIES 5
+#define MAX_TRIES 15
 
 	for (i = 0; i < MAX_TRIES; i++) {
 		hw_lock = rd32(hw, PFTSYN_SEM + (PFTSYN_SEM_BYTES * hw->pf_id));
 		hw_lock = hw_lock & PFTSYN_SEM_BUSY_M;
-		if (!hw_lock)
-			break;
+		if (hw_lock) {
+			/* Somebody is holding the lock */
+			usleep_range(5000, 6000);
+			continue;
+		}
 
-		/* Somebody is holding the lock */
-		usleep_range(10000, 20000);
+		break;
 	}
 
 	return !hw_lock;
-- 
2.34.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW semaphore

[Karol Kolacinski]

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(-)

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;
 }
-- 
2.34.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW semaphore

[Tony Nguyen]

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;
>   }
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW semaphore

[Kolacinski, Karol]

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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW semaphore

[Vinicius Costa Gomes]

"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,
-- 
Vinicius
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW semaphore

[Jacob Keller]

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.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW semaphore

[Vinicius Costa Gomes]

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,
-- 
Vinicius
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 2/2] ice: Remove gettime HW semaphore

[G, GurucharanX]

> -----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)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next 1/2] ice: Check for PTP HW lock more frequently

[G, GurucharanX]

> -----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 1/2] ice: Check for PTP HW lock
> more frequently
> 
> It was observed that PTP HW semaphore can be held for ~50 ms in worst
> case.
> SW should wait longer and check more frequently if the HW lock is held.
> 
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 

Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan