* [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading
@ 2024-03-25 23:20 Michal Schmidt
2024-03-25 23:20 ` [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC Michal Schmidt
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Michal Schmidt @ 2024-03-25 23:20 UTC (permalink / raw
To: intel-wired-lan
Cc: Jesse Brandeburg, Tony Nguyen, Richard Cochran, netdev,
Jacob Keller, Jiri Pirko, Arkadiusz Kubalewski, Karol Kolacinski,
Marcin Szycik, Przemek Kitszel, Temerkhanov, Sergey
This series removes the use of the heavy-weight PTP hardware semaphore
in the gettimex64 path. Instead, serialization of access to the time
register is done using a host-side spinlock. The timer hardware is
shared between PFs on the PCI adapter, so the spinlock must be shared
between ice_pf instances too.
Replacing the PTP hardware semaphore entirely with a mutex is also
possible and you can see it done in my git branch[1], but I am not
posting those patches yet to keep the scope of this series limited.
[1] https://gitlab.com/mschmidt2/linux/-/commits/ice-ptp-host-side-lock-10
v4:
- Patch 1: Use named GENMASK macros and FIELD_PREP.
v3:
- Longer variable name ("a" -> "adapter").
- Propagate xarray error in ice_adapter_get with ERR_PTR.
- Added kernel-doc comments for ice_adapter_{get,put}.
v2:
- Patch 1: Rely on xarray's own lock. (Suggested by Jiri Pirko)
- Patch 2: Do not use *_irqsave with ptp_gltsyn_time_lock, as it's used
only in process contexts.
Michal Schmidt (3):
ice: add ice_adapter for shared data across PFs on the same NIC
ice: avoid the PTP hardware semaphore in gettimex64 path
ice: fold ice_ptp_read_time into ice_ptp_gettimex64
drivers/net/ethernet/intel/ice/Makefile | 3 +-
drivers/net/ethernet/intel/ice/ice.h | 2 +
drivers/net/ethernet/intel/ice/ice_adapter.c | 116 +++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_adapter.h | 28 +++++
drivers/net/ethernet/intel/ice/ice_main.c | 8 ++
drivers/net/ethernet/intel/ice/ice_ptp.c | 33 +-----
drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 3 +
7 files changed, 163 insertions(+), 30 deletions(-)
create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.c
create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.h
--
2.43.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC
2024-03-25 23:20 [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Michal Schmidt
@ 2024-03-25 23:20 ` Michal Schmidt
2024-03-29 4:57 ` [Intel-wired-lan] " Pucha, HimasekharX Reddy
2024-03-25 23:20 ` [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path Michal Schmidt
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Michal Schmidt @ 2024-03-25 23:20 UTC (permalink / raw
To: intel-wired-lan
Cc: Jesse Brandeburg, Tony Nguyen, Richard Cochran, netdev,
Jacob Keller, Jiri Pirko, Arkadiusz Kubalewski, Karol Kolacinski,
Marcin Szycik, Przemek Kitszel, Temerkhanov, Sergey
There is a need for synchronization between ice PFs on the same physical
adapter.
Add a "struct ice_adapter" for holding data shared between PFs of the
same multifunction PCI device. The struct is refcounted - each ice_pf
holds a reference to it.
Its first use will be for PTP. I expect it will be useful also to
improve the ugliness that is ice_prot_id_tbl.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
drivers/net/ethernet/intel/ice/Makefile | 3 +-
drivers/net/ethernet/intel/ice/ice.h | 2 +
drivers/net/ethernet/intel/ice/ice_adapter.c | 114 +++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_adapter.h | 22 ++++
drivers/net/ethernet/intel/ice/ice_main.c | 8 ++
5 files changed, 148 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.c
create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.h
diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
index cddd82d4ca0f..4fa09c321440 100644
--- a/drivers/net/ethernet/intel/ice/Makefile
+++ b/drivers/net/ethernet/intel/ice/Makefile
@@ -36,7 +36,8 @@ ice-y := ice_main.o \
ice_repr.o \
ice_tc_lib.o \
ice_fwlog.o \
- ice_debugfs.o
+ ice_debugfs.o \
+ ice_adapter.o
ice-$(CONFIG_PCI_IOV) += \
ice_sriov.o \
ice_virtchnl.o \
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 365c03d1c462..1ffecbdd361a 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -77,6 +77,7 @@
#include "ice_gnss.h"
#include "ice_irq.h"
#include "ice_dpll.h"
+#include "ice_adapter.h"
#define ICE_BAR0 0
#define ICE_REQ_DESC_MULTIPLE 32
@@ -544,6 +545,7 @@ struct ice_agg_node {
struct ice_pf {
struct pci_dev *pdev;
+ struct ice_adapter *adapter;
struct devlink_region *nvm_region;
struct devlink_region *sram_region;
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
new file mode 100644
index 000000000000..f00ab998e853
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: Copyright Red Hat
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/xarray.h>
+#include "ice_adapter.h"
+
+static DEFINE_XARRAY(ice_adapters);
+
+/* PCI bus number is 8 bits. Slot is 5 bits. Domain can have the rest. */
+#define INDEX_FIELD_DOMAIN GENMASK(BITS_PER_LONG - 1, 13)
+#define INDEX_FIELD_BUS GENMASK(12, 5)
+#define INDEX_FIELD_SLOT GENMASK(4, 0)
+
+static unsigned long ice_adapter_index(const struct pci_dev *pdev)
+{
+ unsigned int domain = pci_domain_nr(pdev->bus);
+
+ WARN_ON(domain > FIELD_MAX(INDEX_FIELD_DOMAIN));
+
+ return FIELD_PREP(INDEX_FIELD_DOMAIN, domain) |
+ FIELD_PREP(INDEX_FIELD_BUS, pdev->bus->number) |
+ FIELD_PREP(INDEX_FIELD_SLOT, PCI_SLOT(pdev->devfn));
+}
+
+static struct ice_adapter *ice_adapter_new(void)
+{
+ struct ice_adapter *adapter;
+
+ adapter = kzalloc(sizeof(*adapter), GFP_KERNEL);
+ if (!adapter)
+ return NULL;
+
+ refcount_set(&adapter->refcount, 1);
+
+ return adapter;
+}
+
+static void ice_adapter_free(struct ice_adapter *adapter)
+{
+ kfree(adapter);
+}
+
+DEFINE_FREE(ice_adapter_free, struct ice_adapter*, if (_T) ice_adapter_free(_T))
+
+/**
+ * ice_adapter_get - Get a shared ice_adapter structure.
+ * @pdev: Pointer to the pci_dev whose driver is getting the ice_adapter.
+ *
+ * Gets a pointer to a shared ice_adapter structure. Physical functions (PFs)
+ * of the same multi-function PCI device share one ice_adapter structure.
+ * The ice_adapter is reference-counted. The PF driver must use ice_adapter_put
+ * to release its reference.
+ *
+ * Context: Process, may sleep.
+ * Return: Pointer to ice_adapter on success.
+ * ERR_PTR() on error. -ENOMEM is the only possible error.
+ */
+struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev)
+{
+ struct ice_adapter *ret, __free(ice_adapter_free) *adapter = NULL;
+ unsigned long index = ice_adapter_index(pdev);
+
+ adapter = ice_adapter_new();
+ if (!adapter)
+ return ERR_PTR(-ENOMEM);
+
+ xa_lock(&ice_adapters);
+ ret = __xa_cmpxchg(&ice_adapters, index, NULL, adapter, GFP_KERNEL);
+ if (xa_is_err(ret)) {
+ ret = ERR_PTR(xa_err(ret));
+ goto unlock;
+ }
+ if (ret) {
+ refcount_inc(&ret->refcount);
+ goto unlock;
+ }
+ ret = no_free_ptr(adapter);
+unlock:
+ xa_unlock(&ice_adapters);
+ return ret;
+}
+
+/**
+ * ice_adapter_put - Release a reference to the shared ice_adapter structure.
+ * @pdev: Pointer to the pci_dev whose driver is releasing the ice_adapter.
+ *
+ * Releases the reference to ice_adapter previously obtained with
+ * ice_adapter_get.
+ *
+ * Context: Any.
+ */
+void ice_adapter_put(const struct pci_dev *pdev)
+{
+ unsigned long index = ice_adapter_index(pdev);
+ struct ice_adapter *adapter;
+
+ xa_lock(&ice_adapters);
+ adapter = xa_load(&ice_adapters, index);
+ if (WARN_ON(!adapter))
+ goto unlock;
+
+ if (!refcount_dec_and_test(&adapter->refcount))
+ goto unlock;
+
+ WARN_ON(__xa_erase(&ice_adapters, index) != adapter);
+ ice_adapter_free(adapter);
+unlock:
+ xa_unlock(&ice_adapters);
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
new file mode 100644
index 000000000000..cb5a02eb24c1
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* SPDX-FileCopyrightText: Copyright Red Hat */
+
+#ifndef _ICE_ADAPTER_H_
+#define _ICE_ADAPTER_H_
+
+#include <linux/refcount_types.h>
+
+struct pci_dev;
+
+/**
+ * struct ice_adapter - PCI adapter resources shared across PFs
+ * @refcount: Reference count. struct ice_pf objects hold the references.
+ */
+struct ice_adapter {
+ refcount_t refcount;
+};
+
+struct ice_adapter *ice_adapter_get(const struct pci_dev *pdev);
+void ice_adapter_put(const struct pci_dev *pdev);
+
+#endif /* _ICE_ADAPTER_H */
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 33a164fa325a..7a75d14d8e9d 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5093,6 +5093,7 @@ static int
ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
{
struct device *dev = &pdev->dev;
+ struct ice_adapter *adapter;
struct ice_pf *pf;
struct ice_hw *hw;
int err;
@@ -5145,7 +5146,12 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
pci_set_master(pdev);
+ adapter = ice_adapter_get(pdev);
+ if (IS_ERR(adapter))
+ return PTR_ERR(adapter);
+
pf->pdev = pdev;
+ pf->adapter = adapter;
pci_set_drvdata(pdev, pf);
set_bit(ICE_DOWN, pf->state);
/* Disable service task until DOWN bit is cleared */
@@ -5196,6 +5202,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
err_load:
ice_deinit(pf);
err_init:
+ ice_adapter_put(pdev);
pci_disable_device(pdev);
return err;
}
@@ -5302,6 +5309,7 @@ static void ice_remove(struct pci_dev *pdev)
ice_setup_mc_magic_wake(pf);
ice_set_wake(pf);
+ ice_adapter_put(pdev);
pci_disable_device(pdev);
}
--
2.43.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path
2024-03-25 23:20 [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Michal Schmidt
2024-03-25 23:20 ` [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC Michal Schmidt
@ 2024-03-25 23:20 ` Michal Schmidt
2024-03-29 5:01 ` [Intel-wired-lan] " Pucha, HimasekharX Reddy
2024-03-25 23:20 ` [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64 Michal Schmidt
2024-03-26 9:51 ` [Intel-wired-lan] [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Ivan Vecera
3 siblings, 1 reply; 10+ messages in thread
From: Michal Schmidt @ 2024-03-25 23:20 UTC (permalink / raw
To: intel-wired-lan
Cc: Jesse Brandeburg, Tony Nguyen, Richard Cochran, netdev,
Jacob Keller, Jiri Pirko, Arkadiusz Kubalewski, Karol Kolacinski,
Marcin Szycik, Przemek Kitszel, Temerkhanov, Sergey
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 <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Michal Schmidt <mschmidt@redhat.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 f00ab998e853..52d15ef7f4b1 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -6,6 +6,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"
@@ -35,6 +36,7 @@ static struct ice_adapter *ice_adapter_new(void)
if (!adapter)
return NULL;
+ spin_lock_init(&adapter->ptp_gltsyn_time_lock);
refcount_set(&adapter->refcount, 1);
return adapter;
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..0875f37add78 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)(&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..2b9423a173bb 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)(&pf->adapter->ptp_gltsyn_time_lock);
wr32(hw, GLTSYN_CMD_SYNC, SYNC_EXEC_CMD);
ice_flush(hw);
}
--
2.43.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64
2024-03-25 23:20 [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Michal Schmidt
2024-03-25 23:20 ` [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC Michal Schmidt
2024-03-25 23:20 ` [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path Michal Schmidt
@ 2024-03-25 23:20 ` Michal Schmidt
2024-03-26 14:46 ` Sai Krishna Gajula
` (2 more replies)
2024-03-26 9:51 ` [Intel-wired-lan] [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Ivan Vecera
3 siblings, 3 replies; 10+ messages in thread
From: Michal Schmidt @ 2024-03-25 23:20 UTC (permalink / raw
To: intel-wired-lan
Cc: Jesse Brandeburg, Tony Nguyen, Richard Cochran, netdev,
Jacob Keller, Jiri Pirko, Arkadiusz Kubalewski, Karol Kolacinski,
Marcin Szycik, Przemek Kitszel, Temerkhanov, Sergey
This is a cleanup. It is unnecessary to have this function just to call
another function.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 25 +++---------------------
1 file changed, 3 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 0875f37add78..0f17fc1181d2 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1166,26 +1166,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf)
ice_ptp_mark_tx_tracker_stale(&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
@@ -1926,9 +1906,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);
+ u64 time_ns;
- ice_ptp_read_time(pf, ts, sts);
-
+ time_ns = ice_ptp_read_src_clk_reg(pf, sts);
+ *ts = ns_to_timespec64(time_ns);
return 0;
}
--
2.43.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Intel-wired-lan] [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading
2024-03-25 23:20 [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Michal Schmidt
` (2 preceding siblings ...)
2024-03-25 23:20 ` [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64 Michal Schmidt
@ 2024-03-26 9:51 ` Ivan Vecera
3 siblings, 0 replies; 10+ messages in thread
From: Ivan Vecera @ 2024-03-26 9:51 UTC (permalink / raw
To: Michal Schmidt, intel-wired-lan
Cc: Jiri Pirko, Temerkhanov, Sergey, netdev, Richard Cochran,
Arkadiusz Kubalewski, Karol Kolacinski, Marcin Szycik,
Tony Nguyen, Przemek Kitszel, Jacob Keller
On 26. 03. 24 0:20, Michal Schmidt wrote:
> This series removes the use of the heavy-weight PTP hardware semaphore
> in the gettimex64 path. Instead, serialization of access to the time
> register is done using a host-side spinlock. The timer hardware is
> shared between PFs on the PCI adapter, so the spinlock must be shared
> between ice_pf instances too.
>
> Replacing the PTP hardware semaphore entirely with a mutex is also
> possible and you can see it done in my git branch[1], but I am not
> posting those patches yet to keep the scope of this series limited.
>
> [1] https://gitlab.com/mschmidt2/linux/-/commits/ice-ptp-host-side-lock-10
>
> v4:
> - Patch 1: Use named GENMASK macros and FIELD_PREP.
>
> v3:
> - Longer variable name ("a" -> "adapter").
> - Propagate xarray error in ice_adapter_get with ERR_PTR.
> - Added kernel-doc comments for ice_adapter_{get,put}.
>
> v2:
> - Patch 1: Rely on xarray's own lock. (Suggested by Jiri Pirko)
> - Patch 2: Do not use *_irqsave with ptp_gltsyn_time_lock, as it's used
> only in process contexts.
>
>
> Michal Schmidt (3):
> ice: add ice_adapter for shared data across PFs on the same NIC
> ice: avoid the PTP hardware semaphore in gettimex64 path
> ice: fold ice_ptp_read_time into ice_ptp_gettimex64
>
> drivers/net/ethernet/intel/ice/Makefile | 3 +-
> drivers/net/ethernet/intel/ice/ice.h | 2 +
> drivers/net/ethernet/intel/ice/ice_adapter.c | 116 +++++++++++++++++++
> drivers/net/ethernet/intel/ice/ice_adapter.h | 28 +++++
> drivers/net/ethernet/intel/ice/ice_main.c | 8 ++
> drivers/net/ethernet/intel/ice/ice_ptp.c | 33 +-----
> drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 3 +
> 7 files changed, 163 insertions(+), 30 deletions(-)
> create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.c
> create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.h
>
Reviewed-by: Ivan Vecera <ivecera@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64
2024-03-25 23:20 ` [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64 Michal Schmidt
@ 2024-03-26 14:46 ` Sai Krishna Gajula
2024-03-29 5:03 ` [Intel-wired-lan] " Pucha, HimasekharX Reddy
2024-03-29 8:35 ` Kalesh Anakkur Purayil
2 siblings, 0 replies; 10+ messages in thread
From: Sai Krishna Gajula @ 2024-03-26 14:46 UTC (permalink / raw
To: Michal Schmidt, intel-wired-lan@lists.osuosl.org
Cc: Jesse Brandeburg, Tony Nguyen, Richard Cochran,
netdev@vger.kernel.org, Jacob Keller, Jiri Pirko,
Arkadiusz Kubalewski, Karol Kolacinski, Marcin Szycik,
Przemek Kitszel, Temerkhanov, Sergey
> -----Original Message-----
> From: Michal Schmidt <mschmidt@redhat.com>
> Sent: Tuesday, March 26, 2024 4:51 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>; Tony Nguyen
> <anthony.l.nguyen@intel.com>; Richard Cochran
> <richardcochran@gmail.com>; netdev@vger.kernel.org; Jacob Keller
> <jacob.e.keller@intel.com>; Jiri Pirko <jiri@resnulli.us>; Arkadiusz Kubalewski
> <arkadiusz.kubalewski@intel.com>; Karol Kolacinski
> <karol.kolacinski@intel.com>; Marcin Szycik <marcin.szycik@linux.intel.com>;
> Przemek Kitszel <przemyslaw.kitszel@intel.com>; Temerkhanov, Sergey
> <sergey.temerkhanov@intel.com>
> Subject: [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into
> ice_ptp_gettimex64
>
> This is a cleanup. It is unnecessary to have this function just to call another
> function.
>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 25 +++---------------------
> 1 file changed, 3 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 0875f37add78..0f17fc1181d2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1166,26 +1166,6 @@ static void ice_ptp_reset_cached_phctime(struct
> ice_pf *pf)
> ice_ptp_mark_tx_tracker_stale(&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
> @@ -1926,9 +1906,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);
> + u64 time_ns;
>
> - ice_ptp_read_time(pf, ts, sts);
> -
> + time_ns = ice_ptp_read_src_clk_reg(pf, sts);
> + *ts = ns_to_timespec64(time_ns);
> return 0;
> }
>
> --
> 2.43.2
>
Reviewed-by: Sai Krishna <saikrishnag@marvell.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [Intel-wired-lan] [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC
2024-03-25 23:20 ` [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC Michal Schmidt
@ 2024-03-29 4:57 ` Pucha, HimasekharX Reddy
0 siblings, 0 replies; 10+ messages in thread
From: Pucha, HimasekharX Reddy @ 2024-03-29 4:57 UTC (permalink / raw
To: mschmidt, intel-wired-lan@lists.osuosl.org
Cc: Jiri Pirko, Temerkhanov, Sergey, netdev@vger.kernel.org,
Richard Cochran, Kubalewski, Arkadiusz, Kolacinski, Karol,
Marcin Szycik, Nguyen, Anthony L, Kitszel, Przemyslaw,
Keller, Jacob E
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Tuesday, March 26, 2024 4:51 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Jiri Pirko <jiri@resnulli.us>; Temerkhanov, Sergey <sergey.temerkhanov@intel.com>; netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Marcin Szycik <marcin.szycik@linux.intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC
>
> There is a need for synchronization between ice PFs on the same physical adapter.
>
> Add a "struct ice_adapter" for holding data shared between PFs of the same multifunction PCI device. The struct is refcounted - each ice_pf holds a reference to it.
>
> Its first use will be for PTP. I expect it will be useful also to improve the ugliness that is ice_prot_id_tbl.
>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/Makefile | 3 +-
> drivers/net/ethernet/intel/ice/ice.h | 2 +
> drivers/net/ethernet/intel/ice/ice_adapter.c | 114 +++++++++++++++++++ drivers/net/ethernet/intel/ice/ice_adapter.h | 22 ++++
> drivers/net/ethernet/intel/ice/ice_main.c | 8 ++
> 5 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.c
> create mode 100644 drivers/net/ethernet/intel/ice/ice_adapter.h
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [Intel-wired-lan] [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path
2024-03-25 23:20 ` [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path Michal Schmidt
@ 2024-03-29 5:01 ` Pucha, HimasekharX Reddy
0 siblings, 0 replies; 10+ messages in thread
From: Pucha, HimasekharX Reddy @ 2024-03-29 5:01 UTC (permalink / raw
To: mschmidt, intel-wired-lan@lists.osuosl.org
Cc: Jiri Pirko, Temerkhanov, Sergey, netdev@vger.kernel.org,
Richard Cochran, Kubalewski, Arkadiusz, Kolacinski, Karol,
Marcin Szycik, Nguyen, Anthony L, Kitszel, Przemyslaw,
Keller, Jacob E
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Tuesday, March 26, 2024 4:51 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Jiri Pirko <jiri@resnulli.us>; Temerkhanov, Sergey <sergey.temerkhanov@intel.com>; netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Marcin Szycik <marcin.szycik@linux.intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> 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 <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.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(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [Intel-wired-lan] [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64
2024-03-25 23:20 ` [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64 Michal Schmidt
2024-03-26 14:46 ` Sai Krishna Gajula
@ 2024-03-29 5:03 ` Pucha, HimasekharX Reddy
2024-03-29 8:35 ` Kalesh Anakkur Purayil
2 siblings, 0 replies; 10+ messages in thread
From: Pucha, HimasekharX Reddy @ 2024-03-29 5:03 UTC (permalink / raw
To: mschmidt, intel-wired-lan@lists.osuosl.org
Cc: Jiri Pirko, Temerkhanov, Sergey, netdev@vger.kernel.org,
Richard Cochran, Kubalewski, Arkadiusz, Kolacinski, Karol,
Marcin Szycik, Nguyen, Anthony L, Kitszel, Przemyslaw,
Keller, Jacob E
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Tuesday, March 26, 2024 4:51 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: Jiri Pirko <jiri@resnulli.us>; Temerkhanov, Sergey <sergey.temerkhanov@intel.com>; netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Kolacinski, Karol <karol.kolacinski@intel.com>; Marcin Szycik <marcin.szycik@linux.intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [Intel-wired-lan] [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64
>
> This is a cleanup. It is unnecessary to have this function just to call another function.
>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 25 +++---------------------
> 1 file changed, 3 insertions(+), 22 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64
2024-03-25 23:20 ` [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64 Michal Schmidt
2024-03-26 14:46 ` Sai Krishna Gajula
2024-03-29 5:03 ` [Intel-wired-lan] " Pucha, HimasekharX Reddy
@ 2024-03-29 8:35 ` Kalesh Anakkur Purayil
2 siblings, 0 replies; 10+ messages in thread
From: Kalesh Anakkur Purayil @ 2024-03-29 8:35 UTC (permalink / raw
To: Michal Schmidt
Cc: intel-wired-lan, Jesse Brandeburg, Tony Nguyen, Richard Cochran,
netdev, Jacob Keller, Jiri Pirko, Arkadiusz Kubalewski,
Karol Kolacinski, Marcin Szycik, Przemek Kitszel,
Temerkhanov, Sergey
[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]
On Tue, Mar 26, 2024 at 6:07 AM Michal Schmidt <mschmidt@redhat.com> wrote:
>
> This is a cleanup. It is unnecessary to have this function just to call
> another function.
>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> ---
> drivers/net/ethernet/intel/ice/ice_ptp.c | 25 +++---------------------
> 1 file changed, 3 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 0875f37add78..0f17fc1181d2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1166,26 +1166,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf)
> ice_ptp_mark_tx_tracker_stale(&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
> @@ -1926,9 +1906,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);
> + u64 time_ns;
>
> - ice_ptp_read_time(pf, ts, sts);
> -
> + time_ns = ice_ptp_read_src_clk_reg(pf, sts);
> + *ts = ns_to_timespec64(time_ns);
> return 0;
> }
>
> --
> 2.43.2
>
>
--
Regards,
Kalesh A P
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-03-29 8:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-25 23:20 [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Michal Schmidt
2024-03-25 23:20 ` [PATCH net-next v4 1/3] ice: add ice_adapter for shared data across PFs on the same NIC Michal Schmidt
2024-03-29 4:57 ` [Intel-wired-lan] " Pucha, HimasekharX Reddy
2024-03-25 23:20 ` [PATCH net-next v4 2/3] ice: avoid the PTP hardware semaphore in gettimex64 path Michal Schmidt
2024-03-29 5:01 ` [Intel-wired-lan] " Pucha, HimasekharX Reddy
2024-03-25 23:20 ` [PATCH net-next v4 3/3] ice: fold ice_ptp_read_time into ice_ptp_gettimex64 Michal Schmidt
2024-03-26 14:46 ` Sai Krishna Gajula
2024-03-29 5:03 ` [Intel-wired-lan] " Pucha, HimasekharX Reddy
2024-03-29 8:35 ` Kalesh Anakkur Purayil
2024-03-26 9:51 ` [Intel-wired-lan] [PATCH net-next v4 0/3] ice: lighter locking for PTP time reading Ivan Vecera
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).