Message-ID: <7fc79351-d805-4a2f-94a2-5a3828357e0f@intel.com>
Date: Mon, 26 Feb 2024 13:13:39 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Michal Schmidt <mschmidt@...hat.com>, "Kolacinski, Karol"
	<karol.kolacinski@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, Jesse Brandeburg
	<jesse.brandeburg@...el.com>, Richard Cochran <richardcochran@...il.com>,
	Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>, Karol Kolacinski
	<karol.kolacinski@...el.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/3] ice: avoid the PTP hardware semaphore in
 gettimex64 path



On 2/26/2024 12:11 PM, Michal Schmidt wrote:
> On Mon, Feb 26, 2024 at 8:36 PM Jacob Keller <jacob.e.keller@...el.com> wrote:
>> On 2/26/2024 7:11 AM, Michal Schmidt wrote:
>>> 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.
>>>
>>
>> I believe this is good enough. I don't know the exact timing involved
>> here, but the hardware synchronizes the update to all the PHYs and the
>> MAC and it is supposed to be on the order of nanoseconds.
> 
> Thanks, that's good to know.
> 
>>> My test code can be seen here:
>>> https://gitlab.com/mschmidt2/linux/-/commits/ice-ptp-host-side-lock
>>> It consists of:
>>>  - kernel threads reading the time in a busy loop and looking at the
>>>    deltas between consecutive values, reporting new maxima.
>>>    in the consecutive values;
>>>  - 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.
>>>
>>
>> Nice.
>>
>>
>> At first, I wasn't convinced we actually need cross-adapter spinlock
>> here. I thought all the flows that took hardware semaphore were on the
>> clock owner. Only the clock owner PF will access the GLTSYN_TIME
>> registers, (gettimex is only exposed through the ptp device, which hooks
>> into the clock owner). Similarly, only the clock owner does adjtime,
>> settime, etc.
> 
> Non-owners do not expose a ptp device to userspace, but they still do
> ice_ptp_periodic_work -> ice_ptp_update_cached_phctime ->
> ice_ptp_read_src_clk_reg, where they read GLTSYN_TIME.
> 

I think we (Karol?) have some work to fix this by refactoring so that
the clock owner does this over auxiliary bus for all PFs, rather than
having 4/8 PFs each with their own background thread we have a single
background thread that reads the value once and updates the cache of all
the PFs. It looks like its still only in internal testing however.. the
current kernel code does what you said above.

Either way, we also have the other flows for E822 devices which also
need to execute timer commands (albiet ones which do not directly affect
the main timer).

It is much simpler to reason about correctness if we simply lock every
executed command and the reads of the main timer. This has the advantage
that we also do not need to lock the timer reads while waiting on
firmware to prep all the PHYs, so it solves a major problem we had
before as well.

Thanks!

>> However... It turns out that at least a couple of flows use the
>> semaphore on non-clock owners. Specifically E822 hardware has to
>> initialize the PHY after a link restart, which includes re-doing Vernier
>> calibration. Each PF handles this itself, but does require use of the
>> timer synchronization commands to do it. In this flow, the main timer is
>> not modified but we still use the semaphore and sync registers.
>>
>> I don't think that impacts the E810 card, but we use the same code flow
>> here. The E822 series hardware doesn't use the AdminQ for the PHY
>> messages, so its faster but I think the locking improvement would still
>> be relevant for them as well.
>>
>> Given all this, I think it makes sense to go this route.
>>
>> I'd like to follow-up with possibly replacing the entire HW semaphore
>> with a mutex initialized here. That would remove a bunch of PCIe posted
>> transactions required to acquire the HW semaphore which would be a
>> further improvement here.
> 
> Yes, I agree and I have already been looking into this.
> Thanks,
> Michal

Ok great. If you think you already have patches for this I can go ahead
and wait on this work instead of duplicating effort.
> 
>> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
>>
>>> Signed-off-by: Michal Schmidt <mschmidt@...hat.com>
>>> ---
>>>  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(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
>>> index deb063401238..4b9f5d29811c 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
>>> @@ -5,6 +5,7 @@
>>>  #include <linux/mutex.h>
>>>  #include <linux/pci.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/spinlock.h>
>>>  #include <linux/xarray.h>
>>>  #include "ice_adapter.h"
>>>
>>> @@ -38,6 +39,7 @@ struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
>>>       if (!a)
>>>               return NULL;
>>>
>>> +     spin_lock_init(&a->ptp_gltsyn_time_lock);
>>>       refcount_set(&a->refcount, 1);
>>>
>>>       if (xa_is_err(xa_store(&ice_adapters, index, a, GFP_KERNEL))) {
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
>>> index cb5a02eb24c1..9d11014ec02f 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
>>> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
>>> @@ -4,15 +4,21 @@
>>>  #ifndef _ICE_ADAPTER_H_
>>>  #define _ICE_ADAPTER_H_
>>>
>>> +#include <linux/spinlock_types.h>
>>>  #include <linux/refcount_types.h>
>>>
>>>  struct pci_dev;
>>>
>>>  /**
>>>   * struct ice_adapter - PCI adapter resources shared across PFs
>>> + * @ptp_gltsyn_time_lock: Spinlock protecting access to the GLTSYN_TIME
>>> + *                        register of the PTP clock.
>>>   * @refcount: Reference count. struct ice_pf objects hold the references.
>>>   */
>>>  struct ice_adapter {
>>> +     /* For access to the GLTSYN_TIME register */
>>> +     spinlock_t ptp_gltsyn_time_lock;
>>> +
>>>       refcount_t refcount;
>>>  };
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
>>> index c11eba07283c..b6c7246245c6 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>>> @@ -374,6 +374,7 @@ ice_ptp_read_src_clk_reg(struct ice_pf *pf, struct ptp_system_timestamp *sts)
>>>       u8 tmr_idx;
>>>
>>>       tmr_idx = ice_get_ptp_src_clock_index(hw);
>>> +     guard(spinlock_irqsave)(&pf->adapter->ptp_gltsyn_time_lock);
>>>       /* Read the system timestamp pre PHC read */
>>>       ptp_read_system_prets(sts);
>>>
>>> @@ -1925,15 +1926,8 @@ 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;
>>> -
>>> -     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);
>>>
>>>       return 0;
>>>  }
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>>> index 187ce9b54e1a..a47dbbfadb74 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
>>> @@ -274,6 +274,9 @@ void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
>>>   */
>>>  static void ice_ptp_exec_tmr_cmd(struct ice_hw *hw)
>>>  {
>>> +     struct ice_pf *pf = container_of(hw, struct ice_pf, hw);
>>> +
>>> +     guard(spinlock_irqsave)(&pf->adapter->ptp_gltsyn_time_lock);
>>>       wr32(hw, GLTSYN_CMD_SYNC, SYNC_EXEC_CMD);
>>>       ice_flush(hw);
>>>  }
>>
> 

Powered by blists - more mailing lists