[Intel-wired-lan] [PATCH next-queue v1 0/3] igc: Add support for multiple TX tstamp requests

[Intel-wired-lan] [PATCH next-queue v1 0/3] igc: Add support for multiple TX tstamp requests

[Vinicius Costa Gomes]

Hi,

Intel I225 and I226 have 4 sets of registers used to store the result
of transmission timestamp requests, until now only one those sets was
used.

Add support for using those extra registers, so the driver is able to
handle multiple applications doing requests at the same time. This
will become more useful when support for PTP multi-domain is more
common.

The series is divided into three parts:

Patch 1 - The idea is to use spinlocks to handle concurrent access to
      the resources, the original version of the patch was proposed
      here:
      
      https://lore.kernel.org/all/20200728233754.65747-5-andre.guedes@intel.com/
      
      It has received some modifications to make it easier to
      integrate with later patches, and some fixes for issues found
      with lockdep.

Patch 2 - Use the 4 registers sets, the idea is simple, we have a pool
      of registers, and we use the first one from the pool that's
      free. Each element of the pool is pre-allocated with the
      register address from each timestamp register set.

Patch 3 - More of an optimization. Use the ptp_aux_work kthread to do
      the work, and also try to do the work "inline" if the timestamp
      is ready already. Suggested by Vladimir Oltean and Kurt
      Kanzenbach.


Evaluation
----------

To do the evaluation I am using a simple application that sends
packets (and waits for the timestamp to be received before sending the
next packet) and takes two measurements:
  1. from the HW timestamp value and the time the application
  retrieves the timestamps (called "HW to Timestamp";
  2. from just before the sendto() being called in the application to
  the time the application retrieves the timestamp (called "Send to
  Timestamp"). I think this measurement is useful to make sure that
  the total time to send a packet and retrieve its timestamp hasn't
  degraded.

(all tests were done for 1M packets, and times are in nanoseconds)

Before:

HW to Timestamp
	min: 9130
	max: 143183
	percentile 99: 10379
	percentile 99.99: 11510
Send to Timestamp
	min: 18431
	max: 196798
	percentile 99: 19937
	percentile 99.99: 26066

After:

HW to Timestamp
	min: 7933
	max: 31934
	percentile 99: 8690
	percentile 99.99: 10598
Send to Timestamp
	min: 17291
	max: 46327
	percentile 99: 18268
	percentile 99.99: 21575

The minimum times are not that different, but we can see a big
improvement in the 'maximum' time.


Andre Guedes (1):
  igc: Fix race condition in PTP tx code

Vinicius Costa Gomes (2):
  igc: Add support for multiple in-flight TX timestamps
  igc: Use ptp->aux_worker to retrieve TX timestamps

 drivers/net/ethernet/intel/igc/igc.h         |  25 ++-
 drivers/net/ethernet/intel/igc/igc_base.h    |   3 +
 drivers/net/ethernet/intel/igc/igc_defines.h |   7 +
 drivers/net/ethernet/intel/igc/igc_main.c    |  58 ++++-
 drivers/net/ethernet/intel/igc/igc_ptp.c     | 225 +++++++++++++------
 drivers/net/ethernet/intel/igc/igc_regs.h    |  12 +
 6 files changed, 240 insertions(+), 90 deletions(-)

-- 
2.39.2

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

[Intel-wired-lan] [PATCH next-queue v1 1/3] igc: Fix race condition in PTP tx code

[Vinicius Costa Gomes]

From: Andre Guedes <andre.guedes@intel.com>

Currently, the igc driver supports timestamping only one tx packet at a
time. During the transmission flow, the skb that requires hardware
timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
adapter->ptp_tx_skb, and notify the network stack.

While the thread executing the transmission flow (the user process
running in kernel mode) and the thread executing ptp_tx_work don't
access adapter->ptp_tx_skb concurrently, there are two other places
where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
igc_ptp_suspend().

igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
thread which runs periodically so it is possible we have two threads
accessing ptp_tx_skb at the same time. Consider the following scenario:
right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
written yet, this is considered a timeout and adapter->ptp_tx_skb is
cleaned up.

This patch fixes the issue described above by adding the ptp_tx_lock to
protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
Since igc_xmit_frame_ring() called in atomic context by the networking
stack, ptp_tx_lock is defined as a spinlock.

With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
flag doesn't provide much of a use anymore so this patch gets rid of it.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      |  5 +-
 drivers/net/ethernet/intel/igc/igc_main.c |  8 +++-
 drivers/net/ethernet/intel/igc/igc_ptp.c  | 57 +++++++++++++++--------
 3 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index df3e26c0cf01..e804a566bdd3 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -227,6 +227,10 @@ struct igc_adapter {
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
 	struct work_struct ptp_tx_work;
+	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
+	 * ptp_tx_lock.
+	 */
+	spinlock_t ptp_tx_lock;
 	struct sk_buff *ptp_tx_skb;
 	struct hwtstamp_config tstamp_config;
 	unsigned long ptp_tx_start;
@@ -401,7 +405,6 @@ enum igc_state_t {
 	__IGC_TESTING,
 	__IGC_RESETTING,
 	__IGC_DOWN,
-	__IGC_PTP_TX_IN_PROGRESS,
 };
 
 enum igc_tx_flags {
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 2928a6c73692..84c9c6e09054 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1565,14 +1565,16 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 
 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
+		unsigned long flags;
+
+		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
 		/* FIXME: add support for retrieving timestamps from
 		 * the other timer registers before skipping the
 		 * timestamping request.
 		 */
 		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
-		    !test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
-					   &adapter->state)) {
+		    !adapter->ptp_tx_skb) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			tx_flags |= IGC_TX_FLAGS_TSTAMP;
 
@@ -1581,6 +1583,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 		} else {
 			adapter->tx_hwtstamp_skipped++;
 		}
+
+		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 	}
 
 	if (skb_vlan_tag_present(skb)) {
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 4e10ced736db..9247395911c9 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -603,14 +603,15 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
 	return 0;
 }
 
+/* Requires adapter->ptp_tx_lock held by caller. */
 static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
 
 	dev_kfree_skb_any(adapter->ptp_tx_skb);
 	adapter->ptp_tx_skb = NULL;
+	adapter->ptp_tx_start = 0;
 	adapter->tx_hwtstamp_timeouts++;
-	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
 	/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
 	rd32(IGC_TXSTMPH);
 	netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
@@ -618,20 +619,20 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
 
 void igc_ptp_tx_hang(struct igc_adapter *adapter)
 {
-	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
-					      IGC_PTP_TX_TIMEOUT);
-
-	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
-		return;
-
-	/* If we haven't received a timestamp within the timeout, it is
-	 * reasonable to assume that it will never occur, so we can unlock the
-	 * timestamp bit when this occurs.
-	 */
-	if (timeout) {
-		cancel_work_sync(&adapter->ptp_tx_work);
-		igc_ptp_tx_timeout(adapter);
-	}
+	unsigned long flags;
+
+	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+
+	if (!adapter->ptp_tx_skb)
+		goto unlock;
+
+	if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
+		goto unlock;
+
+	igc_ptp_tx_timeout(adapter);
+
+unlock:
+	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 }
 
 /**
@@ -641,6 +642,8 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter)
  * If we were asked to do hardware stamping and such a time stamp is
  * available, then it must have been for this skb here because we only
  * allow only one such packet into the queue.
+ *
+ * Context: Expects adapter->ptp_tx_lock to be held by caller.
  */
 static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
 {
@@ -682,7 +685,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
 	 * while we're notifying the stack.
 	 */
 	adapter->ptp_tx_skb = NULL;
-	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
+	adapter->ptp_tx_start = 0;
 
 	/* Notify the stack and free the skb after we've unlocked */
 	skb_tstamp_tx(skb, &shhwtstamps);
@@ -701,16 +704,22 @@ static void igc_ptp_tx_work(struct work_struct *work)
 	struct igc_adapter *adapter = container_of(work, struct igc_adapter,
 						   ptp_tx_work);
 	struct igc_hw *hw = &adapter->hw;
+	unsigned long flags;
 	u32 tsynctxctl;
 
-	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
-		return;
+	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+
+	if (!adapter->ptp_tx_skb)
+		goto unlock;
 
 	tsynctxctl = rd32(IGC_TSYNCTXCTL);
 	if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
-		return;
+		goto unlock;
 
 	igc_ptp_tx_hwtstamp(adapter);
+
+unlock:
+	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 }
 
 /**
@@ -960,6 +969,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
 	}
 
 	spin_lock_init(&adapter->tmreg_lock);
+	spin_lock_init(&adapter->ptp_tx_lock);
 	INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
 
 	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
@@ -1017,13 +1027,20 @@ static void igc_ptm_stop(struct igc_adapter *adapter)
  */
 void igc_ptp_suspend(struct igc_adapter *adapter)
 {
+	unsigned long flags;
+
 	if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
 		return;
 
 	cancel_work_sync(&adapter->ptp_tx_work);
+
+	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+
 	dev_kfree_skb_any(adapter->ptp_tx_skb);
 	adapter->ptp_tx_skb = NULL;
-	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
+	adapter->ptp_tx_start = 0;
+
+	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 
 	if (pci_device_is_present(adapter->pdev)) {
 		igc_ptp_time_save(adapter);
-- 
2.39.2

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

[Intel-wired-lan] [PATCH next-queue v1 2/3] igc: Add support for multiple in-flight TX timestamps

[Vinicius Costa Gomes]

Add support for using the four sets of timestamping registers that
i225/i226 have available for TX.

In some workloads, where multiple applications request hardware
transmission timestamps, it was possible that some of those requests
were denied because the only in use register was already occupied.

This is also in preparation to future support for hardware
timestamping in multiple domains. With multiple domains chances of
multiple TX timestamps being requested at the same time increase.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  18 +-
 drivers/net/ethernet/intel/igc/igc_base.h    |   3 +
 drivers/net/ethernet/intel/igc/igc_defines.h |   7 +
 drivers/net/ethernet/intel/igc/igc_main.c    |  45 +++--
 drivers/net/ethernet/intel/igc/igc_ptp.c     | 171 ++++++++++++-------
 drivers/net/ethernet/intel/igc/igc_regs.h    |  12 ++
 6 files changed, 182 insertions(+), 74 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index e804a566bdd3..f0617838a16a 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -35,6 +35,8 @@ void igc_ethtool_set_ops(struct net_device *);
 
 #define MAX_FLEX_FILTER			32
 
+#define IGC_MAX_TX_TSTAMP_REGS		4
+
 enum igc_mac_filter_type {
 	IGC_MAC_FILTER_TYPE_DST = 0,
 	IGC_MAC_FILTER_TYPE_SRC
@@ -67,6 +69,15 @@ struct igc_rx_packet_stats {
 	u64 other_packets;
 };
 
+struct igc_tx_timestamp_request {
+	struct sk_buff *skb;   /* reference to the packet being timestamped */
+	unsigned long start;   /* when the tstamp request started (jiffies) */
+	u32 mask;              /* _TSYNCTXCTL_TXTT_{X} bit for this request */
+	u32 regl;              /* which TXSTMPL_{X} register should be used */
+	u32 regh;              /* which TXSTMPH_{X} register should be used */
+	u32 flags;             /* flags that should be added to the tx_buffer */
+};
+
 struct igc_ring_container {
 	struct igc_ring *ring;          /* pointer to linked list of rings */
 	unsigned int total_bytes;       /* total bytes processed this int */
@@ -231,9 +242,8 @@ struct igc_adapter {
 	 * ptp_tx_lock.
 	 */
 	spinlock_t ptp_tx_lock;
-	struct sk_buff *ptp_tx_skb;
+	struct igc_tx_timestamp_request tx_tstamp[IGC_MAX_TX_TSTAMP_REGS];
 	struct hwtstamp_config tstamp_config;
-	unsigned long ptp_tx_start;
 	unsigned int ptp_flags;
 	/* System time value lock */
 	spinlock_t tmreg_lock;
@@ -416,6 +426,10 @@ enum igc_tx_flags {
 	/* olinfo flags */
 	IGC_TX_FLAGS_IPV4	= 0x10,
 	IGC_TX_FLAGS_CSUM	= 0x20,
+
+	IGC_TX_FLAGS_TSTAMP_1	= 0x100,
+	IGC_TX_FLAGS_TSTAMP_2	= 0x200,
+	IGC_TX_FLAGS_TSTAMP_3	= 0x400,
 };
 
 enum igc_boards {
diff --git a/drivers/net/ethernet/intel/igc/igc_base.h b/drivers/net/ethernet/intel/igc/igc_base.h
index 7a992befca24..a009703da704 100644
--- a/drivers/net/ethernet/intel/igc/igc_base.h
+++ b/drivers/net/ethernet/intel/igc/igc_base.h
@@ -34,6 +34,9 @@ struct igc_adv_tx_context_desc {
 
 /* Adv Transmit Descriptor Config Masks */
 #define IGC_ADVTXD_MAC_TSTAMP	0x00080000 /* IEEE1588 Timestamp packet */
+#define IGC_ADVTXD_TSTAMP_REG_1	0x00010000 /* Select register 1 for timestamp */
+#define IGC_ADVTXD_TSTAMP_REG_2	0x00020000 /* Select register 2 for timestamp */
+#define IGC_ADVTXD_TSTAMP_REG_3	0x00030000 /* Select register 3 for timestamp */
 #define IGC_ADVTXD_DTYP_CTXT	0x00200000 /* Advanced Context Descriptor */
 #define IGC_ADVTXD_DTYP_DATA	0x00300000 /* Advanced Data Descriptor */
 #define IGC_ADVTXD_DCMD_EOP	0x01000000 /* End of Packet */
diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 9dec3563ce3a..967b24fbb87b 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -454,6 +454,9 @@
 
 /* Time Sync Transmit Control bit definitions */
 #define IGC_TSYNCTXCTL_TXTT_0			0x00000001  /* Tx timestamp reg 0 valid */
+#define IGC_TSYNCTXCTL_TXTT_1			0x00000002  /* Tx timestamp reg 1 valid */
+#define IGC_TSYNCTXCTL_TXTT_2			0x00000004  /* Tx timestamp reg 2 valid */
+#define IGC_TSYNCTXCTL_TXTT_3			0x00000008  /* Tx timestamp reg 3 valid */
 #define IGC_TSYNCTXCTL_ENABLED			0x00000010  /* enable Tx timestamping */
 #define IGC_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK	0x0000F000  /* max delay */
 #define IGC_TSYNCTXCTL_SYNC_COMP_ERR		0x20000000  /* sync err */
@@ -461,6 +464,10 @@
 #define IGC_TSYNCTXCTL_START_SYNC		0x80000000  /* initiate sync */
 #define IGC_TSYNCTXCTL_TXSYNSIG			0x00000020  /* Sample TX tstamp in PHY sop */
 
+#define IGC_TSYNCTXCTL_TXTT_ANY ( \
+		IGC_TSYNCTXCTL_TXTT_0 | IGC_TSYNCTXCTL_TXTT_1 | \
+		IGC_TSYNCTXCTL_TXTT_2 | IGC_TSYNCTXCTL_TXTT_3)
+
 /* Timer selection bits */
 #define IGC_AUX_IO_TIMER_SEL_SYSTIM0	(0u << 30) /* Select SYSTIM0 for auxiliary time stamp */
 #define IGC_AUX_IO_TIMER_SEL_SYSTIM1	(1u << 30) /* Select SYSTIM1 for auxiliary time stamp */
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 84c9c6e09054..4861ad0689ed 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1236,10 +1236,21 @@ static u32 igc_tx_cmd_type(struct sk_buff *skb, u32 tx_flags)
 	cmd_type |= IGC_SET_FLAG(tx_flags, IGC_TX_FLAGS_TSO,
 				 (IGC_ADVTXD_DCMD_TSE));
 
-	/* set timestamp bit if present */
+	/* set timestamp bit if present, will select the register set
+	 * based on the _TSTAMP(_X) bit.
+	 */
 	cmd_type |= IGC_SET_FLAG(tx_flags, IGC_TX_FLAGS_TSTAMP,
 				 (IGC_ADVTXD_MAC_TSTAMP));
 
+	cmd_type |= IGC_SET_FLAG(tx_flags, IGC_TX_FLAGS_TSTAMP_1,
+				 (IGC_ADVTXD_TSTAMP_REG_1));
+
+	cmd_type |= IGC_SET_FLAG(tx_flags, IGC_TX_FLAGS_TSTAMP_2,
+				 (IGC_ADVTXD_TSTAMP_REG_2));
+
+	cmd_type |= IGC_SET_FLAG(tx_flags, IGC_TX_FLAGS_TSTAMP_3,
+				 (IGC_ADVTXD_TSTAMP_REG_3));
+
 	/* insert frame checksum */
 	cmd_type ^= IGC_SET_FLAG(skb->no_fcs, 1, IGC_ADVTXD_DCMD_IFCS);
 
@@ -1498,6 +1509,26 @@ static int igc_tso(struct igc_ring *tx_ring,
 	return 1;
 }
 
+static bool igc_request_tx_tstamp(struct igc_adapter *adapter, struct sk_buff *skb, u32 *flags)
+{
+	int i;
+
+	for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) {
+		struct igc_tx_timestamp_request *tstamp = &adapter->tx_tstamp[i];
+
+		if (tstamp->skb)
+			continue;
+
+		tstamp->skb = skb_get(skb);
+		tstamp->start = jiffies;
+		*flags = tstamp->flags;
+
+		return true;
+	}
+
+	return false;
+}
+
 static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 				       struct igc_ring *tx_ring)
 {
@@ -1566,20 +1597,14 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
 		struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
 		unsigned long flags;
+		u32 tstamp_flags;
 
 		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
-		/* FIXME: add support for retrieving timestamps from
-		 * the other timer registers before skipping the
-		 * timestamping request.
-		 */
 		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
-		    !adapter->ptp_tx_skb) {
+		    igc_request_tx_tstamp(adapter, skb, &tstamp_flags)) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-			tx_flags |= IGC_TX_FLAGS_TSTAMP;
-
-			adapter->ptp_tx_skb = skb_get(skb);
-			adapter->ptp_tx_start = jiffies;
+			tx_flags |= IGC_TX_FLAGS_TSTAMP | tstamp_flags;
 		} else {
 			adapter->tx_hwtstamp_skipped++;
 		}
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 9247395911c9..0cb932b52a7b 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -604,92 +604,109 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
 }
 
 /* Requires adapter->ptp_tx_lock held by caller. */
-static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
+static void igc_ptp_tx_timeout(struct igc_adapter *adapter,
+			       struct igc_tx_timestamp_request *tstamp)
 {
 	struct igc_hw *hw = &adapter->hw;
 
-	dev_kfree_skb_any(adapter->ptp_tx_skb);
-	adapter->ptp_tx_skb = NULL;
-	adapter->ptp_tx_start = 0;
+	dev_kfree_skb_any(tstamp->skb);
+	tstamp->skb = NULL;
+	tstamp->start = 0;
 	adapter->tx_hwtstamp_timeouts++;
-	/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
-	rd32(IGC_TXSTMPH);
+
+	/* Reading the high register of one of the timestamp registers
+	 * clears the equivalent bit in the TSYNCTXCTL register.
+	 */
+	rd32(tstamp->regh);
+
 	netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
 }
 
 void igc_ptp_tx_hang(struct igc_adapter *adapter)
 {
+	struct igc_tx_timestamp_request *tstamp;
 	unsigned long flags;
+	int i;
 
 	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
-	if (!adapter->ptp_tx_skb)
-		goto unlock;
+	for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) {
+		tstamp = &adapter->tx_tstamp[i];
 
-	if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
-		goto unlock;
+		if (!tstamp->skb)
+			continue;
 
-	igc_ptp_tx_timeout(adapter);
+		if (time_is_after_jiffies(tstamp->start + IGC_PTP_TX_TIMEOUT))
+			continue;
+
+		igc_ptp_tx_timeout(adapter, tstamp);
+	}
 
-unlock:
 	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 }
 
 /**
  * igc_ptp_tx_hwtstamp - utility function which checks for TX time stamp
  * @adapter: Board private structure
+ * @mask: bitmask of which timestamp registers are ready
  *
- * If we were asked to do hardware stamping and such a time stamp is
- * available, then it must have been for this skb here because we only
- * allow only one such packet into the queue.
+ * Check against the ready mask for which of the timestamp register
+ * sets are ready to be retrieved, then retrieve that and notify the
+ * rest of the stack.
  *
  * Context: Expects adapter->ptp_tx_lock to be held by caller.
  */
-static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
+static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter, u32 mask)
 {
-	struct sk_buff *skb = adapter->ptp_tx_skb;
 	struct skb_shared_hwtstamps shhwtstamps;
 	struct igc_hw *hw = &adapter->hw;
-	int adjust = 0;
-	u64 regval;
+	int i;
 
-	if (WARN_ON_ONCE(!skb))
+	if (!mask)
 		return;
 
-	regval = rd32(IGC_TXSTMPL);
-	regval |= (u64)rd32(IGC_TXSTMPH) << 32;
-	if (igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval))
-		return;
+	for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) {
+		struct igc_tx_timestamp_request *tstamp = &adapter->tx_tstamp[i];
+		struct sk_buff *skb;
+		int adjust = 0;
+		u64 regval;
+
+		if (!(mask & tstamp->mask))
+			continue;
+
+		skb = tstamp->skb;
+		if (!skb)
+			continue;
+
+		regval = rd32(tstamp->regl);
+		regval |= (u64)rd32(tstamp->regh) << 32;
+		if (igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval))
+			return;
+
+		switch (adapter->link_speed) {
+		case SPEED_10:
+			adjust = IGC_I225_TX_LATENCY_10;
+			break;
+		case SPEED_100:
+			adjust = IGC_I225_TX_LATENCY_100;
+			break;
+		case SPEED_1000:
+			adjust = IGC_I225_TX_LATENCY_1000;
+			break;
+		case SPEED_2500:
+			adjust = IGC_I225_TX_LATENCY_2500;
+			break;
+		}
+
+		shhwtstamps.hwtstamp =
+			ktime_add_ns(shhwtstamps.hwtstamp, adjust);
+
+		tstamp->skb = NULL;
+		tstamp->start = 0;
 
-	switch (adapter->link_speed) {
-	case SPEED_10:
-		adjust = IGC_I225_TX_LATENCY_10;
-		break;
-	case SPEED_100:
-		adjust = IGC_I225_TX_LATENCY_100;
-		break;
-	case SPEED_1000:
-		adjust = IGC_I225_TX_LATENCY_1000;
-		break;
-	case SPEED_2500:
-		adjust = IGC_I225_TX_LATENCY_2500;
-		break;
+		skb_tstamp_tx(skb, &shhwtstamps);
+		dev_kfree_skb_any(skb);
 	}
-
-	shhwtstamps.hwtstamp =
-		ktime_add_ns(shhwtstamps.hwtstamp, adjust);
-
-	/* Clear the lock early before calling skb_tstamp_tx so that
-	 * applications are not woken up before the lock bit is clear. We use
-	 * a copy of the skb pointer to ensure other threads can't change it
-	 * while we're notifying the stack.
-	 */
-	adapter->ptp_tx_skb = NULL;
-	adapter->ptp_tx_start = 0;
-
-	/* Notify the stack and free the skb after we've unlocked */
-	skb_tstamp_tx(skb, &shhwtstamps);
-	dev_kfree_skb_any(skb);
 }
 
 /**
@@ -709,16 +726,10 @@ static void igc_ptp_tx_work(struct work_struct *work)
 
 	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
-	if (!adapter->ptp_tx_skb)
-		goto unlock;
-
 	tsynctxctl = rd32(IGC_TSYNCTXCTL);
-	if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
-		goto unlock;
 
-	igc_ptp_tx_hwtstamp(adapter);
+	igc_ptp_tx_hwtstamp(adapter, tsynctxctl & IGC_TSYNCTXCTL_TXTT_ANY);
 
-unlock:
 	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 }
 
@@ -931,9 +942,34 @@ static int igc_ptp_getcrosststamp(struct ptp_clock_info *ptp,
 void igc_ptp_init(struct igc_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
+	struct igc_tx_timestamp_request *tstamp;
 	struct igc_hw *hw = &adapter->hw;
 	int i;
 
+	tstamp = &adapter->tx_tstamp[0];
+	tstamp->mask = IGC_TSYNCTXCTL_TXTT_0;
+	tstamp->regl = IGC_TXSTMPL_0;
+	tstamp->regh = IGC_TXSTMPH_0;
+	tstamp->flags = 0;
+
+	tstamp = &adapter->tx_tstamp[1];
+	tstamp->mask = IGC_TSYNCTXCTL_TXTT_1;
+	tstamp->regl = IGC_TXSTMPL_1;
+	tstamp->regh = IGC_TXSTMPH_1;
+	tstamp->flags = IGC_TX_FLAGS_TSTAMP_1;
+
+	tstamp = &adapter->tx_tstamp[2];
+	tstamp->mask = IGC_TSYNCTXCTL_TXTT_2;
+	tstamp->regl = IGC_TXSTMPL_2;
+	tstamp->regh = IGC_TXSTMPH_2;
+	tstamp->flags = IGC_TX_FLAGS_TSTAMP_2;
+
+	tstamp = &adapter->tx_tstamp[3];
+	tstamp->mask = IGC_TSYNCTXCTL_TXTT_3;
+	tstamp->regl = IGC_TXSTMPL_3;
+	tstamp->regh = IGC_TXSTMPH_3;
+	tstamp->flags = IGC_TX_FLAGS_TSTAMP_3;
+
 	switch (hw->mac.type) {
 	case igc_i225:
 		for (i = 0; i < IGC_N_SDP; i++) {
@@ -1018,6 +1054,19 @@ static void igc_ptm_stop(struct igc_adapter *adapter)
 	wr32(IGC_PTM_CTRL, ctrl);
 }
 
+static void igc_tx_tstamp_clear(struct igc_adapter *adapter)
+{
+	int i;
+
+	for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) {
+		struct igc_tx_timestamp_request *tstamp = &adapter->tx_tstamp[i];
+
+		dev_kfree_skb_any(tstamp->skb);
+		tstamp->skb = NULL;
+		tstamp->start = 0;
+	}
+}
+
 /**
  * igc_ptp_suspend - Disable PTP work items and prepare for suspend
  * @adapter: Board private structure
@@ -1036,9 +1085,7 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
 
 	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
-	dev_kfree_skb_any(adapter->ptp_tx_skb);
-	adapter->ptp_tx_skb = NULL;
-	adapter->ptp_tx_start = 0;
+	igc_tx_tstamp_clear(adapter);
 
 	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
 
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index 01c86d36856d..17e0e627679d 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -243,6 +243,18 @@
 #define IGC_SYSTIMR	0x0B6F8  /* System time register Residue */
 #define IGC_TIMINCA	0x0B608  /* Increment attributes register - RW */
 
+/* TX Timestamp Low */
+#define IGC_TXSTMPL_0		0x0B618
+#define IGC_TXSTMPL_1		0x0B698
+#define IGC_TXSTMPL_2		0x0B6B8
+#define IGC_TXSTMPL_3		0x0B6D8
+
+/* TX Timestamp High */
+#define IGC_TXSTMPH_0		0x0B61C
+#define IGC_TXSTMPH_1		0x0B69C
+#define IGC_TXSTMPH_2		0x0B6BC
+#define IGC_TXSTMPH_3		0x0B6DC
+
 #define IGC_TXSTMPL	0x0B618  /* Tx timestamp value Low - RO */
 #define IGC_TXSTMPH	0x0B61C  /* Tx timestamp value High - RO */
 
-- 
2.39.2

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

[Intel-wired-lan] [PATCH next-queue v1 3/3] igc: Use ptp->aux_worker to retrieve TX timestamps

[Vinicius Costa Gomes]

ptp->aux_worker is a kthread and allows the user to set it's priority,
which is useful for workloads that time synchronization is required.

As an optimization, when the interrupt is handled try to retrieve the
timestamps "inline", if they are not, schedule the workload. This
should reduce the delay before the TX timestamp is available to
userspace.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      |  2 +-
 drivers/net/ethernet/intel/igc/igc_main.c |  7 +++++-
 drivers/net/ethernet/intel/igc/igc_ptp.c  | 29 ++++++++++++++++-------
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index f0617838a16a..ce7754cb7e6f 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -237,7 +237,6 @@ struct igc_adapter {
 
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
-	struct work_struct ptp_tx_work;
 	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
 	 * ptp_tx_lock.
 	 */
@@ -651,6 +650,7 @@ int igc_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
 int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
 void igc_ptp_tx_hang(struct igc_adapter *adapter);
 void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts);
+long igc_ptp_tx_work(struct ptp_clock_info *ptp);
 
 #define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring))
 
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 4861ad0689ed..a7775d618867 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -5226,8 +5226,13 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
 	}
 
 	if (tsicr & IGC_TSICR_TXTS) {
+		long delay;
 		/* retrieve hardware timestamp */
-		schedule_work(&adapter->ptp_tx_work);
+		delay = igc_ptp_tx_work(&adapter->ptp_caps);
+		if (delay >= 0) {
+			/* The timestamp is not ready, schedule to check later */
+			ptp_schedule_worker(adapter->ptp_clock, delay);
+		}
 		ack |= IGC_TSICR_TXTS;
 	}
 
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 0cb932b52a7b..34237464f26d 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -713,24 +713,37 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter, u32 mask)
  * igc_ptp_tx_work
  * @work: pointer to work struct
  *
- * This work function polls the TSYNCTXCTL valid bit to determine when a
- * timestamp has been taken for the current stored skb.
+ * This work function polls the TSYNCTXCTL valid bit to determine when
+ * a timestamp has been taken for the current stored skb. Return a
+ * delay in case there's no timestamp ready.
  */
-static void igc_ptp_tx_work(struct work_struct *work)
+long igc_ptp_tx_work(struct ptp_clock_info *ptp)
 {
-	struct igc_adapter *adapter = container_of(work, struct igc_adapter,
-						   ptp_tx_work);
+	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
+						   ptp_caps);
 	struct igc_hw *hw = &adapter->hw;
 	unsigned long flags;
 	u32 tsynctxctl;
+	long delay = -1;
 
 	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
 	tsynctxctl = rd32(IGC_TSYNCTXCTL);
+	tsynctxctl &= IGC_TSYNCTXCTL_TXTT_ANY;
+	if (!tsynctxctl) {
+		/* We got the interrupt but the timestamp is not ready
+		 * still, schedule to check later.
+		 */
+		delay = usecs_to_jiffies(1);
+		goto unlock;
+	}
 
-	igc_ptp_tx_hwtstamp(adapter, tsynctxctl & IGC_TSYNCTXCTL_TXTT_ANY);
+	igc_ptp_tx_hwtstamp(adapter, tsynctxctl);
 
+unlock:
 	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
+
+	return delay;
 }
 
 /**
@@ -993,6 +1006,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
 		adapter->ptp_caps.n_per_out = IGC_N_PEROUT;
 		adapter->ptp_caps.n_pins = IGC_N_SDP;
 		adapter->ptp_caps.verify = igc_ptp_verify_pin;
+		adapter->ptp_caps.do_aux_work = igc_ptp_tx_work;
 
 		if (!igc_is_crosststamp_supported(adapter))
 			break;
@@ -1006,7 +1020,6 @@ void igc_ptp_init(struct igc_adapter *adapter)
 
 	spin_lock_init(&adapter->tmreg_lock);
 	spin_lock_init(&adapter->ptp_tx_lock);
-	INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
 
 	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
 	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
@@ -1081,7 +1094,7 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
 	if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
 		return;
 
-	cancel_work_sync(&adapter->ptp_tx_work);
+	ptp_cancel_worker_sync(adapter->ptp_clock);
 
 	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
 
-- 
2.39.2

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

Re: [Intel-wired-lan] [PATCH next-queue v1 1/3] igc: Fix race condition in PTP tx code

[Vladimir Oltean]

Hi Vinicius,

Some small comments, feel free to ignore them.

On Mon, Feb 27, 2023 at 09:45:32PM -0800, Vinicius Costa Gomes wrote:
> From: Andre Guedes <andre.guedes@intel.com>
> 
> Currently, the igc driver supports timestamping only one tx packet at a
> time. During the transmission flow, the skb that requires hardware
> timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
> timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
> scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
> adapter->ptp_tx_skb, and notify the network stack.
> 
> While the thread executing the transmission flow (the user process
> running in kernel mode) and the thread executing ptp_tx_work don't
> access adapter->ptp_tx_skb concurrently, there are two other places
> where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
> igc_ptp_suspend().
> 
> igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
> thread which runs periodically so it is possible we have two threads
> accessing ptp_tx_skb at the same time. Consider the following scenario:
> right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
> igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
> written yet, this is considered a timeout and adapter->ptp_tx_skb is
> cleaned up.
> 
> This patch fixes the issue described above by adding the ptp_tx_lock to
> protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
> Since igc_xmit_frame_ring() called in atomic context by the networking
> stack, ptp_tx_lock is defined as a spinlock.
> 
> With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
> flag doesn't provide much of a use anymore so this patch gets rid of it.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---

Shouldn't this have a Fixes: tag and be sent to the 'net' queue, since
it fixes a bug which can be seen in the real world?

>  drivers/net/ethernet/intel/igc/igc.h      |  5 +-
>  drivers/net/ethernet/intel/igc/igc_main.c |  8 +++-
>  drivers/net/ethernet/intel/igc/igc_ptp.c  | 57 +++++++++++++++--------
>  3 files changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index df3e26c0cf01..e804a566bdd3 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -227,6 +227,10 @@ struct igc_adapter {
>  	struct ptp_clock *ptp_clock;
>  	struct ptp_clock_info ptp_caps;
>  	struct work_struct ptp_tx_work;
> +	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
> +	 * ptp_tx_lock.
> +	 */
> +	spinlock_t ptp_tx_lock;
>  	struct sk_buff *ptp_tx_skb;
>  	struct hwtstamp_config tstamp_config;
>  	unsigned long ptp_tx_start;
> @@ -401,7 +405,6 @@ enum igc_state_t {
>  	__IGC_TESTING,
>  	__IGC_RESETTING,
>  	__IGC_DOWN,
> -	__IGC_PTP_TX_IN_PROGRESS,
>  };
>  
>  enum igc_tx_flags {
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 2928a6c73692..84c9c6e09054 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1565,14 +1565,16 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  
>  	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
>  		struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>  
>  		/* FIXME: add support for retrieving timestamps from
>  		 * the other timer registers before skipping the
>  		 * timestamping request.
>  		 */
>  		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
> -		    !test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
> -					   &adapter->state)) {
> +		    !adapter->ptp_tx_skb) {
>  			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>  			tx_flags |= IGC_TX_FLAGS_TSTAMP;
>  
> @@ -1581,6 +1583,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  		} else {
>  			adapter->tx_hwtstamp_skipped++;

unrelated: when adapter->tstamp_config.tx_type != HWTSTAMP_TX_ON but
skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP was set, it simply means
that the timestamp is "not for you". For example igc is a DSA master for
a PTP-capable switch. Should adapter->tx_hwtstamp_skipped increment in
that case? I mean, timestamps are skipped, but is it worth bumping a
user-visible counter for what is essentially the normal thing to do?

>  		}
> +
> +		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  	}
>  
>  	if (skb_vlan_tag_present(skb)) {
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index 4e10ced736db..9247395911c9 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -603,14 +603,15 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
>  	return 0;
>  }
>  
> +/* Requires adapter->ptp_tx_lock held by caller. */
>  static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
>  {
>  	struct igc_hw *hw = &adapter->hw;
>  
>  	dev_kfree_skb_any(adapter->ptp_tx_skb);
>  	adapter->ptp_tx_skb = NULL;
> +	adapter->ptp_tx_start = 0;

what's the reason for setting this to 0? (here, there and everywhere)

>  	adapter->tx_hwtstamp_timeouts++;
> -	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
>  	/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
>  	rd32(IGC_TXSTMPH);
>  	netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
> @@ -618,20 +619,20 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
>  
>  void igc_ptp_tx_hang(struct igc_adapter *adapter)
>  {
> -	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
> -					      IGC_PTP_TX_TIMEOUT);
> -
> -	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
> -		return;
> -
> -	/* If we haven't received a timestamp within the timeout, it is
> -	 * reasonable to assume that it will never occur, so we can unlock the
> -	 * timestamp bit when this occurs.
> -	 */
> -	if (timeout) {
> -		cancel_work_sync(&adapter->ptp_tx_work);
> -		igc_ptp_tx_timeout(adapter);
> -	}
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
> +
> +	if (!adapter->ptp_tx_skb)
> +		goto unlock;
> +
> +	if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
> +		goto unlock;
> +
> +	igc_ptp_tx_timeout(adapter);
> +
> +unlock:
> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  }
>  
>  /**
> @@ -641,6 +642,8 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter)
>   * If we were asked to do hardware stamping and such a time stamp is
>   * available, then it must have been for this skb here because we only
>   * allow only one such packet into the queue.
> + *
> + * Context: Expects adapter->ptp_tx_lock to be held by caller.
>   */
>  static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
>  {
> @@ -682,7 +685,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
>  	 * while we're notifying the stack.
>  	 */

This comment just became obsolete:

	/* Clear the lock early before calling skb_tstamp_tx so that
	 * applications are not woken up before the lock bit is clear. We use
	 * a copy of the skb pointer to ensure other threads can't change it
	 * while we're notifying the stack.
	 */

it gets removed in one of the later patches.

I suppose this doesn't make a real difference unless this change gets
backported as a fix to stable kernels and the others don't.

>  	adapter->ptp_tx_skb = NULL;
> -	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
> +	adapter->ptp_tx_start = 0;
>  
>  	/* Notify the stack and free the skb after we've unlocked */
>  	skb_tstamp_tx(skb, &shhwtstamps);
> @@ -701,16 +704,22 @@ static void igc_ptp_tx_work(struct work_struct *work)
>  	struct igc_adapter *adapter = container_of(work, struct igc_adapter,
>  						   ptp_tx_work);
>  	struct igc_hw *hw = &adapter->hw;
> +	unsigned long flags;
>  	u32 tsynctxctl;
>  
> -	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
> -		return;
> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
> +
> +	if (!adapter->ptp_tx_skb)
> +		goto unlock;
>  
>  	tsynctxctl = rd32(IGC_TSYNCTXCTL);
>  	if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
> -		return;
> +		goto unlock;
>  
>  	igc_ptp_tx_hwtstamp(adapter);
> +
> +unlock:
> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  }
>  
>  /**
> @@ -960,6 +969,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
>  	}
>  
>  	spin_lock_init(&adapter->tmreg_lock);
> +	spin_lock_init(&adapter->ptp_tx_lock);
>  	INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
>  
>  	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
> @@ -1017,13 +1027,20 @@ static void igc_ptm_stop(struct igc_adapter *adapter)
>   */
>  void igc_ptp_suspend(struct igc_adapter *adapter)
>  {
> +	unsigned long flags;
> +
>  	if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
>  		return;
>  
>  	cancel_work_sync(&adapter->ptp_tx_work);
> +
> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
> +
>  	dev_kfree_skb_any(adapter->ptp_tx_skb);
>  	adapter->ptp_tx_skb = NULL;
> -	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
> +	adapter->ptp_tx_start = 0;
> +
> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  
>  	if (pci_device_is_present(adapter->pdev)) {
>  		igc_ptp_time_save(adapter);
> -- 
> 2.39.2
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH next-queue v1 2/3] igc: Add support for multiple in-flight TX timestamps

[Vladimir Oltean]

On Mon, Feb 27, 2023 at 09:45:33PM -0800, Vinicius Costa Gomes wrote:
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index e804a566bdd3..f0617838a16a 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -35,6 +35,8 @@ void igc_ethtool_set_ops(struct net_device *);
>  
>  #define MAX_FLEX_FILTER			32
>  
> +#define IGC_MAX_TX_TSTAMP_REGS		4
> +
>  enum igc_mac_filter_type {
>  	IGC_MAC_FILTER_TYPE_DST = 0,
>  	IGC_MAC_FILTER_TYPE_SRC
> @@ -67,6 +69,15 @@ struct igc_rx_packet_stats {
>  	u64 other_packets;
>  };
>  
> +struct igc_tx_timestamp_request {
> +	struct sk_buff *skb;   /* reference to the packet being timestamped */
> +	unsigned long start;   /* when the tstamp request started (jiffies) */
> +	u32 mask;              /* _TSYNCTXCTL_TXTT_{X} bit for this request */
> +	u32 regl;              /* which TXSTMPL_{X} register should be used */
> +	u32 regh;              /* which TXSTMPH_{X} register should be used */
> +	u32 flags;             /* flags that should be added to the tx_buffer */
> +};
> +
>  struct igc_ring_container {
>  	struct igc_ring *ring;          /* pointer to linked list of rings */
>  	unsigned int total_bytes;       /* total bytes processed this int */
> @@ -231,9 +242,8 @@ struct igc_adapter {
>  	 * ptp_tx_lock.
>  	 */
>  	spinlock_t ptp_tx_lock;
> -	struct sk_buff *ptp_tx_skb;
> +	struct igc_tx_timestamp_request tx_tstamp[IGC_MAX_TX_TSTAMP_REGS];
>  	struct hwtstamp_config tstamp_config;
> -	unsigned long ptp_tx_start;

This comment added by patch 1/3 already became obsolete:

	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
	 * ptp_tx_lock.
	 */
	spinlock_t ptp_tx_lock;

>  	unsigned int ptp_flags;
>  	/* System time value lock */
>  	spinlock_t tmreg_lock;
> @@ -416,6 +426,10 @@ enum igc_tx_flags {
>  	/* olinfo flags */
>  	IGC_TX_FLAGS_IPV4	= 0x10,
>  	IGC_TX_FLAGS_CSUM	= 0x20,
> +
> +	IGC_TX_FLAGS_TSTAMP_1	= 0x100,
> +	IGC_TX_FLAGS_TSTAMP_2	= 0x200,
> +	IGC_TX_FLAGS_TSTAMP_3	= 0x400,
>  };
>  
>  enum igc_boards {
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index 9247395911c9..0cb932b52a7b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -604,92 +604,109 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
>  }
>  
>  /* Requires adapter->ptp_tx_lock held by caller. */
> -static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
> +static void igc_ptp_tx_timeout(struct igc_adapter *adapter,
> +			       struct igc_tx_timestamp_request *tstamp)
>  {
>  	struct igc_hw *hw = &adapter->hw;
>  
> -	dev_kfree_skb_any(adapter->ptp_tx_skb);
> -	adapter->ptp_tx_skb = NULL;
> -	adapter->ptp_tx_start = 0;
> +	dev_kfree_skb_any(tstamp->skb);
> +	tstamp->skb = NULL;
> +	tstamp->start = 0;
>  	adapter->tx_hwtstamp_timeouts++;
> -	/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
> -	rd32(IGC_TXSTMPH);
> +
> +	/* Reading the high register of one of the timestamp registers
> +	 * clears the equivalent bit in the TSYNCTXCTL register.
> +	 */
> +	rd32(tstamp->regh);
> +
>  	netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
>  }
>  
>  void igc_ptp_tx_hang(struct igc_adapter *adapter)
>  {
> +	struct igc_tx_timestamp_request *tstamp;
>  	unsigned long flags;
> +	int i;
>  
>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);

Since this is called unconditionally and periodically from igc_watchdog_task(),
it will periodically temporarily disable IRQs and therefore also
softirqs, to not race with the data path, right?

Does it make sense to only do this if adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON?
There shouldn't be TX timestamps in flight if that isn't set, and an
_irqsave could be avoided.

I suppose this is more of a comment on patch 1/3 really.

>  
> -	if (!adapter->ptp_tx_skb)
> -		goto unlock;
> +	for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) {
> +		tstamp = &adapter->tx_tstamp[i];
>  
> -	if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
> -		goto unlock;
> +		if (!tstamp->skb)
> +			continue;
>  
> -	igc_ptp_tx_timeout(adapter);
> +		if (time_is_after_jiffies(tstamp->start + IGC_PTP_TX_TIMEOUT))
> +			continue;
> +
> +		igc_ptp_tx_timeout(adapter, tstamp);
> +	}
>  
> -unlock:
>  	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>  }
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH next-queue v1 3/3] igc: Use ptp->aux_worker to retrieve TX timestamps

[Vladimir Oltean]

On Mon, Feb 27, 2023 at 09:45:34PM -0800, Vinicius Costa Gomes wrote:
> ptp->aux_worker is a kthread and allows the user to set it's priority,
> which is useful for workloads that time synchronization is required.
> 
> As an optimization, when the interrupt is handled try to retrieve the
> timestamps "inline", if they are not, schedule the workload. This
> should reduce the delay before the TX timestamp is available to
> userspace.
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  2 +-
>  drivers/net/ethernet/intel/igc/igc_main.c |  7 +++++-
>  drivers/net/ethernet/intel/igc/igc_ptp.c  | 29 ++++++++++++++++-------
>  3 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index f0617838a16a..ce7754cb7e6f 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -237,7 +237,6 @@ struct igc_adapter {
>  
>  	struct ptp_clock *ptp_clock;
>  	struct ptp_clock_info ptp_caps;
> -	struct work_struct ptp_tx_work;
>  	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
>  	 * ptp_tx_lock.
>  	 */
> @@ -651,6 +650,7 @@ int igc_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
>  int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
>  void igc_ptp_tx_hang(struct igc_adapter *adapter);
>  void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts);
> +long igc_ptp_tx_work(struct ptp_clock_info *ptp);
>  
>  #define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring))
>  
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 4861ad0689ed..a7775d618867 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -5226,8 +5226,13 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
>  	}
>  
>  	if (tsicr & IGC_TSICR_TXTS) {
> +		long delay;
>  		/* retrieve hardware timestamp */
> -		schedule_work(&adapter->ptp_tx_work);
> +		delay = igc_ptp_tx_work(&adapter->ptp_caps);
> +		if (delay >= 0) {
> +			/* The timestamp is not ready, schedule to check later */
> +			ptp_schedule_worker(adapter->ptp_clock, delay);
> +		}
>  		ack |= IGC_TSICR_TXTS;
>  	}
>  
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index 0cb932b52a7b..34237464f26d 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -713,24 +713,37 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter, u32 mask)
>   * igc_ptp_tx_work
>   * @work: pointer to work struct
>   *
> - * This work function polls the TSYNCTXCTL valid bit to determine when a
> - * timestamp has been taken for the current stored skb.
> + * This work function polls the TSYNCTXCTL valid bit to determine when
> + * a timestamp has been taken for the current stored skb. Return a
> + * delay in case there's no timestamp ready.
>   */
> -static void igc_ptp_tx_work(struct work_struct *work)
> +long igc_ptp_tx_work(struct ptp_clock_info *ptp)
>  {
> -	struct igc_adapter *adapter = container_of(work, struct igc_adapter,
> -						   ptp_tx_work);
> +	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
> +						   ptp_caps);
>  	struct igc_hw *hw = &adapter->hw;
>  	unsigned long flags;
>  	u32 tsynctxctl;
> +	long delay = -1;

idk what the coding style is in the igc driver, but this line is longer
than the previous one, and with network drivers, one usually prefers the
"reverse Christmas tree".

>  
>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>  
>  	tsynctxctl = rd32(IGC_TSYNCTXCTL);
> +	tsynctxctl &= IGC_TSYNCTXCTL_TXTT_ANY;
> +	if (!tsynctxctl) {
> +		/* We got the interrupt but the timestamp is not ready
> +		 * still, schedule to check later.
> +		 */

this comment raises many hairs reading it, it sure makes it sound like the
hardware is buggy for raising an interrupt before the data is available.

I had this same question a while ago, could you borrow some of the wording
you used back then to explain in this comment, here, how it is possible to
get IGC_TSICR_TXTS ("Transmit Timestamp") set in the Time Sync Interrupt
Causes register, and for there not to be any TX timestamps available?
https://lore.kernel.org/netdev/20220815222639.346wachaaq5zjwue@skbuf/

> +		delay = usecs_to_jiffies(1);

it will surely take more than 1 us anyway to schedule, and so, setting a
timer for the kthread seems slightly pointless? The ptp_schedule_worker()/
__kthread_queue_delayed_work() API accepts 0 for delay, meaning "immediately".

> +		goto unlock;
> +	}
>  
> -	igc_ptp_tx_hwtstamp(adapter, tsynctxctl & IGC_TSYNCTXCTL_TXTT_ANY);
> +	igc_ptp_tx_hwtstamp(adapter, tsynctxctl);
>  
> +unlock:
>  	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
> +
> +	return delay;

On the same note: from the kthread you can also busy poll for a while;
for TX timestamps (which should be delivered rather quickly) I guess
this is more productive than rescheduling again (with such a short
interval), since it makes the slot available quicker for TX. Though,
I have to say, not quite clear how rescheduling ends up being needed in
the first place....

>  }
>  
>  /**
> @@ -993,6 +1006,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
>  		adapter->ptp_caps.n_per_out = IGC_N_PEROUT;
>  		adapter->ptp_caps.n_pins = IGC_N_SDP;
>  		adapter->ptp_caps.verify = igc_ptp_verify_pin;
> +		adapter->ptp_caps.do_aux_work = igc_ptp_tx_work;
>  
>  		if (!igc_is_crosststamp_supported(adapter))
>  			break;
> @@ -1006,7 +1020,6 @@ void igc_ptp_init(struct igc_adapter *adapter)
>  
>  	spin_lock_init(&adapter->tmreg_lock);
>  	spin_lock_init(&adapter->ptp_tx_lock);
> -	INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
>  
>  	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
>  	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
> @@ -1081,7 +1094,7 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
>  	if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
>  		return;
>  
> -	cancel_work_sync(&adapter->ptp_tx_work);
> +	ptp_cancel_worker_sync(adapter->ptp_clock);
>  
>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>  
> -- 
> 2.39.2
>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH next-queue v1 0/3] igc: Add support for multiple TX tstamp requests

[Vladimir Oltean]

On Mon, Feb 27, 2023 at 09:45:31PM -0800, Vinicius Costa Gomes wrote:
> Patch 3 - More of an optimization. Use the ptp_aux_work kthread to do
>       the work, and also try to do the work "inline" if the timestamp
>       is ready already. Suggested by Vladimir Oltean and Kurt
>       Kanzenbach.
> 
> Evaluation
> ----------
> 
> To do the evaluation I am using a simple application that sends
> packets (and waits for the timestamp to be received before sending the
> next packet) and takes two measurements:

If the application never generates multiple requests in flight, then
this evaluation is only testing patch 3 (and patches 1 and 2 only to the
extent that they don't cause a regression), right?

>   1. from the HW timestamp value and the time the application
>   retrieves the timestamps (called "HW to Timestamp";
>   2. from just before the sendto() being called in the application to
>   the time the application retrieves the timestamp (called "Send to
>   Timestamp"). I think this measurement is useful to make sure that
>   the total time to send a packet and retrieve its timestamp hasn't
>   degraded.
> 
> (all tests were done for 1M packets, and times are in nanoseconds)
> 
> Before:
> 
> HW to Timestamp
> 	min: 9130
> 	max: 143183

what margin of error did phc2sys have here? Tens, hundreds, thousands of
ns, more? Was it a controlled variable? "HW to Timestamp" implies a
comparison of 2 times from 2 different time sources, kept in sync with
each other.

> 	percentile 99: 10379
> 	percentile 99.99: 11510
> Send to Timestamp
> 	min: 18431
> 	max: 196798
> 	percentile 99: 19937
> 	percentile 99.99: 26066
> 
> After:
> 
> HW to Timestamp
> 	min: 7933
> 	max: 31934

so the reduction of the max "HW to Timestamp" from 143 us to 32 us all
the way to user space is mostly due to the inline processing of the TX
timestamp, within the hardirq handler, right? Can you measure how much
it is due to that, and how much due to the PTP kthread (simplest way
would be to keep the kthread, but remove the inline processing)? How
many reschedules of the kthread there are per TX timestamp? Even a
single set of 4 numbers, denoting the maximum numbers of reschedules per
timestamp request, would be useful information.

> 	percentile 99: 8690
> 	percentile 99.99: 10598
> Send to Timestamp
> 	min: 17291
> 	max: 46327
> 	percentile 99: 18268
> 	percentile 99.99: 21575
> 
> The minimum times are not that different

right, probably because the time to do a context switch to user space dominates

> , but we can see a big improvement in the 'maximum' time.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH next-queue v1 1/3] igc: Fix race condition in PTP tx code

[Vinicius Costa Gomes]

Hi Vladimir,

Sorry for the delay, crazy times around here.

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> Hi Vinicius,
>
> Some small comments, feel free to ignore them.
>
> On Mon, Feb 27, 2023 at 09:45:32PM -0800, Vinicius Costa Gomes wrote:
>> From: Andre Guedes <andre.guedes@intel.com>
>> 
>> Currently, the igc driver supports timestamping only one tx packet at a
>> time. During the transmission flow, the skb that requires hardware
>> timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
>> timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
>> scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
>> adapter->ptp_tx_skb, and notify the network stack.
>> 
>> While the thread executing the transmission flow (the user process
>> running in kernel mode) and the thread executing ptp_tx_work don't
>> access adapter->ptp_tx_skb concurrently, there are two other places
>> where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
>> igc_ptp_suspend().
>> 
>> igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
>> thread which runs periodically so it is possible we have two threads
>> accessing ptp_tx_skb at the same time. Consider the following scenario:
>> right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
>> igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
>> written yet, this is considered a timeout and adapter->ptp_tx_skb is
>> cleaned up.
>> 
>> This patch fixes the issue described above by adding the ptp_tx_lock to
>> protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
>> Since igc_xmit_frame_ring() called in atomic context by the networking
>> stack, ptp_tx_lock is defined as a spinlock.
>> 
>> With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
>> flag doesn't provide much of a use anymore so this patch gets rid of it.
>> 
>> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>
> Shouldn't this have a Fixes: tag and be sent to the 'net' queue, since
> it fixes a bug which can be seen in the real world?
>

As I couldn't reproduce the race condition (the "false" timeout) at all
on my systems, at first I chose not to propose this for 'net'.

But on the other hand, yeah, I don't think this patch will have any side
effects and will only protect against this (possible) issue. So yeah,
will add a 'Fixes:' tag and propose it there.

>>  drivers/net/ethernet/intel/igc/igc.h      |  5 +-
>>  drivers/net/ethernet/intel/igc/igc_main.c |  8 +++-
>>  drivers/net/ethernet/intel/igc/igc_ptp.c  | 57 +++++++++++++++--------
>>  3 files changed, 47 insertions(+), 23 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
>> index df3e26c0cf01..e804a566bdd3 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -227,6 +227,10 @@ struct igc_adapter {
>>  	struct ptp_clock *ptp_clock;
>>  	struct ptp_clock_info ptp_caps;
>>  	struct work_struct ptp_tx_work;
>> +	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
>> +	 * ptp_tx_lock.
>> +	 */
>> +	spinlock_t ptp_tx_lock;
>>  	struct sk_buff *ptp_tx_skb;
>>  	struct hwtstamp_config tstamp_config;
>>  	unsigned long ptp_tx_start;
>> @@ -401,7 +405,6 @@ enum igc_state_t {
>>  	__IGC_TESTING,
>>  	__IGC_RESETTING,
>>  	__IGC_DOWN,
>> -	__IGC_PTP_TX_IN_PROGRESS,
>>  };
>>  
>>  enum igc_tx_flags {
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 2928a6c73692..84c9c6e09054 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -1565,14 +1565,16 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>>  
>>  	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
>>  		struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
>> +		unsigned long flags;
>> +
>> +		spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>>  
>>  		/* FIXME: add support for retrieving timestamps from
>>  		 * the other timer registers before skipping the
>>  		 * timestamping request.
>>  		 */
>>  		if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
>> -		    !test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
>> -					   &adapter->state)) {
>> +		    !adapter->ptp_tx_skb) {
>>  			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>>  			tx_flags |= IGC_TX_FLAGS_TSTAMP;
>>  
>> @@ -1581,6 +1583,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>>  		} else {
>>  			adapter->tx_hwtstamp_skipped++;
>
> unrelated: when adapter->tstamp_config.tx_type != HWTSTAMP_TX_ON but
> skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP was set, it simply means
> that the timestamp is "not for you". For example igc is a DSA master for
> a PTP-capable switch. Should adapter->tx_hwtstamp_skipped increment in
> that case? I mean, timestamps are skipped, but is it worth bumping a
> user-visible counter for what is essentially the normal thing to do?
>

Good point, I never considered this case.

Incrementing the counter seems wrong, will separate the conditions for
the "not for me" and for the "this is for me and I am busy" cases. 

>>  		}
>> +
>> +		spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>>  	}
>>  
>>  	if (skb_vlan_tag_present(skb)) {
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
>> index 4e10ced736db..9247395911c9 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
>> @@ -603,14 +603,15 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
>>  	return 0;
>>  }
>>  
>> +/* Requires adapter->ptp_tx_lock held by caller. */
>>  static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
>>  {
>>  	struct igc_hw *hw = &adapter->hw;
>>  
>>  	dev_kfree_skb_any(adapter->ptp_tx_skb);
>>  	adapter->ptp_tx_skb = NULL;
>> +	adapter->ptp_tx_start = 0;
>
> what's the reason for setting this to 0? (here, there and everywhere)
>

Not really needed, something I missed from the early version of the
patch. Will fix.

>>  	adapter->tx_hwtstamp_timeouts++;
>> -	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
>>  	/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
>>  	rd32(IGC_TXSTMPH);
>>  	netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
>> @@ -618,20 +619,20 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
>>  
>>  void igc_ptp_tx_hang(struct igc_adapter *adapter)
>>  {
>> -	bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
>> -					      IGC_PTP_TX_TIMEOUT);
>> -
>> -	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
>> -		return;
>> -
>> -	/* If we haven't received a timestamp within the timeout, it is
>> -	 * reasonable to assume that it will never occur, so we can unlock the
>> -	 * timestamp bit when this occurs.
>> -	 */
>> -	if (timeout) {
>> -		cancel_work_sync(&adapter->ptp_tx_work);
>> -		igc_ptp_tx_timeout(adapter);
>> -	}
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>> +
>> +	if (!adapter->ptp_tx_skb)
>> +		goto unlock;
>> +
>> +	if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
>> +		goto unlock;
>> +
>> +	igc_ptp_tx_timeout(adapter);
>> +
>> +unlock:
>> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>>  }
>>  
>>  /**
>> @@ -641,6 +642,8 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter)
>>   * If we were asked to do hardware stamping and such a time stamp is
>>   * available, then it must have been for this skb here because we only
>>   * allow only one such packet into the queue.
>> + *
>> + * Context: Expects adapter->ptp_tx_lock to be held by caller.
>>   */
>>  static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
>>  {
>> @@ -682,7 +685,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
>>  	 * while we're notifying the stack.
>>  	 */
>
> This comment just became obsolete:
>
> 	/* Clear the lock early before calling skb_tstamp_tx so that
> 	 * applications are not woken up before the lock bit is clear. We use
> 	 * a copy of the skb pointer to ensure other threads can't change it
> 	 * while we're notifying the stack.
> 	 */
>
> it gets removed in one of the later patches.
>
> I suppose this doesn't make a real difference unless this change gets
> backported as a fix to stable kernels and the others don't.
>

Will remove the comment in this patch. Good catch.

>>  	adapter->ptp_tx_skb = NULL;
>> -	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
>> +	adapter->ptp_tx_start = 0;
>>  
>>  	/* Notify the stack and free the skb after we've unlocked */
>>  	skb_tstamp_tx(skb, &shhwtstamps);
>> @@ -701,16 +704,22 @@ static void igc_ptp_tx_work(struct work_struct *work)
>>  	struct igc_adapter *adapter = container_of(work, struct igc_adapter,
>>  						   ptp_tx_work);
>>  	struct igc_hw *hw = &adapter->hw;
>> +	unsigned long flags;
>>  	u32 tsynctxctl;
>>  
>> -	if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
>> -		return;
>> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>> +
>> +	if (!adapter->ptp_tx_skb)
>> +		goto unlock;
>>  
>>  	tsynctxctl = rd32(IGC_TSYNCTXCTL);
>>  	if (WARN_ON_ONCE(!(tsynctxctl & IGC_TSYNCTXCTL_TXTT_0)))
>> -		return;
>> +		goto unlock;
>>  
>>  	igc_ptp_tx_hwtstamp(adapter);
>> +
>> +unlock:
>> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>>  }
>>  
>>  /**
>> @@ -960,6 +969,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
>>  	}
>>  
>>  	spin_lock_init(&adapter->tmreg_lock);
>> +	spin_lock_init(&adapter->ptp_tx_lock);
>>  	INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
>>  
>>  	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
>> @@ -1017,13 +1027,20 @@ static void igc_ptm_stop(struct igc_adapter *adapter)
>>   */
>>  void igc_ptp_suspend(struct igc_adapter *adapter)
>>  {
>> +	unsigned long flags;
>> +
>>  	if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
>>  		return;
>>  
>>  	cancel_work_sync(&adapter->ptp_tx_work);
>> +
>> +	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>> +
>>  	dev_kfree_skb_any(adapter->ptp_tx_skb);
>>  	adapter->ptp_tx_skb = NULL;
>> -	clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
>> +	adapter->ptp_tx_start = 0;
>> +
>> +	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>>  
>>  	if (pci_device_is_present(adapter->pdev)) {
>>  		igc_ptp_time_save(adapter);
>> -- 
>> 2.39.2
>>

-- 
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 next-queue v1 2/3] igc: Add support for multiple in-flight TX timestamps

[Vinicius Costa Gomes]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> On Mon, Feb 27, 2023 at 09:45:33PM -0800, Vinicius Costa Gomes wrote:
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
>> index e804a566bdd3..f0617838a16a 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -35,6 +35,8 @@ void igc_ethtool_set_ops(struct net_device *);
>>  
>>  #define MAX_FLEX_FILTER			32
>>  
>> +#define IGC_MAX_TX_TSTAMP_REGS		4
>> +
>>  enum igc_mac_filter_type {
>>  	IGC_MAC_FILTER_TYPE_DST = 0,
>>  	IGC_MAC_FILTER_TYPE_SRC
>> @@ -67,6 +69,15 @@ struct igc_rx_packet_stats {
>>  	u64 other_packets;
>>  };
>>  
>> +struct igc_tx_timestamp_request {
>> +	struct sk_buff *skb;   /* reference to the packet being timestamped */
>> +	unsigned long start;   /* when the tstamp request started (jiffies) */
>> +	u32 mask;              /* _TSYNCTXCTL_TXTT_{X} bit for this request */
>> +	u32 regl;              /* which TXSTMPL_{X} register should be used */
>> +	u32 regh;              /* which TXSTMPH_{X} register should be used */
>> +	u32 flags;             /* flags that should be added to the tx_buffer */
>> +};
>> +
>>  struct igc_ring_container {
>>  	struct igc_ring *ring;          /* pointer to linked list of rings */
>>  	unsigned int total_bytes;       /* total bytes processed this int */
>> @@ -231,9 +242,8 @@ struct igc_adapter {
>>  	 * ptp_tx_lock.
>>  	 */
>>  	spinlock_t ptp_tx_lock;
>> -	struct sk_buff *ptp_tx_skb;
>> +	struct igc_tx_timestamp_request tx_tstamp[IGC_MAX_TX_TSTAMP_REGS];
>>  	struct hwtstamp_config tstamp_config;
>> -	unsigned long ptp_tx_start;
>
> This comment added by patch 1/3 already became obsolete:
>
> 	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
> 	 * ptp_tx_lock.
> 	 */
> 	spinlock_t ptp_tx_lock;
>

Good catch. Will fix.

>>  	unsigned int ptp_flags;
>>  	/* System time value lock */
>>  	spinlock_t tmreg_lock;
>> @@ -416,6 +426,10 @@ enum igc_tx_flags {
>>  	/* olinfo flags */
>>  	IGC_TX_FLAGS_IPV4	= 0x10,
>>  	IGC_TX_FLAGS_CSUM	= 0x20,
>> +
>> +	IGC_TX_FLAGS_TSTAMP_1	= 0x100,
>> +	IGC_TX_FLAGS_TSTAMP_2	= 0x200,
>> +	IGC_TX_FLAGS_TSTAMP_3	= 0x400,
>>  };
>>  
>>  enum igc_boards {
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
>> index 9247395911c9..0cb932b52a7b 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
>> @@ -604,92 +604,109 @@ static int igc_ptp_set_timestamp_mode(struct igc_adapter *adapter,
>>  }
>>  
>>  /* Requires adapter->ptp_tx_lock held by caller. */
>> -static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
>> +static void igc_ptp_tx_timeout(struct igc_adapter *adapter,
>> +			       struct igc_tx_timestamp_request *tstamp)
>>  {
>>  	struct igc_hw *hw = &adapter->hw;
>>  
>> -	dev_kfree_skb_any(adapter->ptp_tx_skb);
>> -	adapter->ptp_tx_skb = NULL;
>> -	adapter->ptp_tx_start = 0;
>> +	dev_kfree_skb_any(tstamp->skb);
>> +	tstamp->skb = NULL;
>> +	tstamp->start = 0;
>>  	adapter->tx_hwtstamp_timeouts++;
>> -	/* Clear the tx valid bit in TSYNCTXCTL register to enable interrupt. */
>> -	rd32(IGC_TXSTMPH);
>> +
>> +	/* Reading the high register of one of the timestamp registers
>> +	 * clears the equivalent bit in the TSYNCTXCTL register.
>> +	 */
>> +	rd32(tstamp->regh);
>> +
>>  	netdev_warn(adapter->netdev, "Tx timestamp timeout\n");
>>  }
>>  
>>  void igc_ptp_tx_hang(struct igc_adapter *adapter)
>>  {
>> +	struct igc_tx_timestamp_request *tstamp;
>>  	unsigned long flags;
>> +	int i;
>>  
>>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>
> Since this is called unconditionally and periodically from igc_watchdog_task(),
> it will periodically temporarily disable IRQs and therefore also
> softirqs, to not race with the data path, right?
>
> Does it make sense to only do this if adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON?
> There shouldn't be TX timestamps in flight if that isn't set, and an
> _irqsave could be avoided.
>
> I suppose this is more of a comment on patch 1/3 really.
>

That's a good point. Yeah, if noone has enabled hardware timestamps,
then there's no timestamps requests able to timeout. Will take a closer
look and see if I am missing anything, but good idea. Will fix.

>>  
>> -	if (!adapter->ptp_tx_skb)
>> -		goto unlock;
>> +	for (i = 0; i < IGC_MAX_TX_TSTAMP_REGS; i++) {
>> +		tstamp = &adapter->tx_tstamp[i];
>>  
>> -	if (time_is_after_jiffies(adapter->ptp_tx_start + IGC_PTP_TX_TIMEOUT))
>> -		goto unlock;
>> +		if (!tstamp->skb)
>> +			continue;
>>  
>> -	igc_ptp_tx_timeout(adapter);
>> +		if (time_is_after_jiffies(tstamp->start + IGC_PTP_TX_TIMEOUT))
>> +			continue;
>> +
>> +		igc_ptp_tx_timeout(adapter, tstamp);
>> +	}
>>  
>> -unlock:
>>  	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>>  }

-- 
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 next-queue v1 3/3] igc: Use ptp->aux_worker to retrieve TX timestamps

[Vinicius Costa Gomes]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> On Mon, Feb 27, 2023 at 09:45:34PM -0800, Vinicius Costa Gomes wrote:
>> ptp->aux_worker is a kthread and allows the user to set it's priority,
>> which is useful for workloads that time synchronization is required.
>> 
>> As an optimization, when the interrupt is handled try to retrieve the
>> timestamps "inline", if they are not, schedule the workload. This
>> should reduce the delay before the TX timestamp is available to
>> userspace.
>> 
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>>  drivers/net/ethernet/intel/igc/igc.h      |  2 +-
>>  drivers/net/ethernet/intel/igc/igc_main.c |  7 +++++-
>>  drivers/net/ethernet/intel/igc/igc_ptp.c  | 29 ++++++++++++++++-------
>>  3 files changed, 28 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
>> index f0617838a16a..ce7754cb7e6f 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -237,7 +237,6 @@ struct igc_adapter {
>>  
>>  	struct ptp_clock *ptp_clock;
>>  	struct ptp_clock_info ptp_caps;
>> -	struct work_struct ptp_tx_work;
>>  	/* Access to ptp_tx_skb and ptp_tx_start is protected by the
>>  	 * ptp_tx_lock.
>>  	 */
>> @@ -651,6 +650,7 @@ int igc_ptp_set_ts_config(struct net_device *netdev, struct ifreq *ifr);
>>  int igc_ptp_get_ts_config(struct net_device *netdev, struct ifreq *ifr);
>>  void igc_ptp_tx_hang(struct igc_adapter *adapter);
>>  void igc_ptp_read(struct igc_adapter *adapter, struct timespec64 *ts);
>> +long igc_ptp_tx_work(struct ptp_clock_info *ptp);
>>  
>>  #define igc_rx_pg_size(_ring) (PAGE_SIZE << igc_rx_pg_order(_ring))
>>  
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 4861ad0689ed..a7775d618867 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -5226,8 +5226,13 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
>>  	}
>>  
>>  	if (tsicr & IGC_TSICR_TXTS) {
>> +		long delay;
>>  		/* retrieve hardware timestamp */
>> -		schedule_work(&adapter->ptp_tx_work);
>> +		delay = igc_ptp_tx_work(&adapter->ptp_caps);
>> +		if (delay >= 0) {
>> +			/* The timestamp is not ready, schedule to check later */
>> +			ptp_schedule_worker(adapter->ptp_clock, delay);
>> +		}
>>  		ack |= IGC_TSICR_TXTS;
>>  	}
>>  
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
>> index 0cb932b52a7b..34237464f26d 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
>> @@ -713,24 +713,37 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter, u32 mask)
>>   * igc_ptp_tx_work
>>   * @work: pointer to work struct
>>   *
>> - * This work function polls the TSYNCTXCTL valid bit to determine when a
>> - * timestamp has been taken for the current stored skb.
>> + * This work function polls the TSYNCTXCTL valid bit to determine when
>> + * a timestamp has been taken for the current stored skb. Return a
>> + * delay in case there's no timestamp ready.
>>   */
>> -static void igc_ptp_tx_work(struct work_struct *work)
>> +long igc_ptp_tx_work(struct ptp_clock_info *ptp)
>>  {
>> -	struct igc_adapter *adapter = container_of(work, struct igc_adapter,
>> -						   ptp_tx_work);
>> +	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
>> +						   ptp_caps);
>>  	struct igc_hw *hw = &adapter->hw;
>>  	unsigned long flags;
>>  	u32 tsynctxctl;
>> +	long delay = -1;
>
> idk what the coding style is in the igc driver, but this line is longer
> than the previous one, and with network drivers, one usually prefers the
> "reverse Christmas tree".
>

Ugh. My fault. Will fix.

>>  
>>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>>  
>>  	tsynctxctl = rd32(IGC_TSYNCTXCTL);
>> +	tsynctxctl &= IGC_TSYNCTXCTL_TXTT_ANY;
>> +	if (!tsynctxctl) {
>> +		/* We got the interrupt but the timestamp is not ready
>> +		 * still, schedule to check later.
>> +		 */
>
> this comment raises many hairs reading it, it sure makes it sound like the
> hardware is buggy for raising an interrupt before the data is available.
>

Another good point. I will check how many times this condition happens
and see if the hardware folks have any suggestion about what could be
done. I agree that having this re-schedule indeed looks like buggy
hardware.

> I had this same question a while ago, could you borrow some of the wording
> you used back then to explain in this comment, here, how it is possible to
> get IGC_TSICR_TXTS ("Transmit Timestamp") set in the Time Sync Interrupt
> Causes register, and for there not to be any TX timestamps available?
> https://lore.kernel.org/netdev/20220815222639.346wachaaq5zjwue@skbuf/
>

Will improve the comments, if this check still needs to stay.

>> +		delay = usecs_to_jiffies(1);
>
> it will surely take more than 1 us anyway to schedule, and so, setting a
> timer for the kthread seems slightly pointless? The ptp_schedule_worker()/
> __kthread_queue_delayed_work() API accepts 0 for delay, meaning "immediately".
>

The '1' is more to mean "do something else" and then check again. From
looking at code, '0' could mean that it could run just before this
kthread returns to the scheduler. But let's see if the hardware folks
have better suggestions (and if this is indeed necessary).

>> +		goto unlock;
>> +	}
>>  
>> -	igc_ptp_tx_hwtstamp(adapter, tsynctxctl & IGC_TSYNCTXCTL_TXTT_ANY);
>> +	igc_ptp_tx_hwtstamp(adapter, tsynctxctl);
>>  
>> +unlock:
>>  	spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
>> +
>> +	return delay;
>
> On the same note: from the kthread you can also busy poll for a while;
> for TX timestamps (which should be delivered rather quickly) I guess
> this is more productive than rescheduling again (with such a short
> interval), since it makes the slot available quicker for TX. Though,
> I have to say, not quite clear how rescheduling ends up being needed in
> the first place....
>

Again, let's see what my experiments and the HW folks say about this.
Perhaps this is just me being paranoid.

>>  }
>>  
>>  /**
>> @@ -993,6 +1006,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
>>  		adapter->ptp_caps.n_per_out = IGC_N_PEROUT;
>>  		adapter->ptp_caps.n_pins = IGC_N_SDP;
>>  		adapter->ptp_caps.verify = igc_ptp_verify_pin;
>> +		adapter->ptp_caps.do_aux_work = igc_ptp_tx_work;
>>  
>>  		if (!igc_is_crosststamp_supported(adapter))
>>  			break;
>> @@ -1006,7 +1020,6 @@ void igc_ptp_init(struct igc_adapter *adapter)
>>  
>>  	spin_lock_init(&adapter->tmreg_lock);
>>  	spin_lock_init(&adapter->ptp_tx_lock);
>> -	INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
>>  
>>  	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
>>  	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
>> @@ -1081,7 +1094,7 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
>>  	if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
>>  		return;
>>  
>> -	cancel_work_sync(&adapter->ptp_tx_work);
>> +	ptp_cancel_worker_sync(adapter->ptp_clock);
>>  
>>  	spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
>>  
>> -- 
>> 2.39.2
>>


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 next-queue v1 0/3] igc: Add support for multiple TX tstamp requests

[Vinicius Costa Gomes]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> On Mon, Feb 27, 2023 at 09:45:31PM -0800, Vinicius Costa Gomes wrote:
>> Patch 3 - More of an optimization. Use the ptp_aux_work kthread to do
>>       the work, and also try to do the work "inline" if the timestamp
>>       is ready already. Suggested by Vladimir Oltean and Kurt
>>       Kanzenbach.
>> 
>> Evaluation
>> ----------
>> 
>> To do the evaluation I am using a simple application that sends
>> packets (and waits for the timestamp to be received before sending the
>> next packet) and takes two measurements:
>
> If the application never generates multiple requests in flight, then
> this evaluation is only testing patch 3 (and patches 1 and 2 only to the
> extent that they don't cause a regression), right?
>

That's right. I was more interested in not causing a regression. I could
run the same test with two (or more) applications and give some numbers,
but those numbers couldn't be directly compared with the current version
of the code.

But good idea. I will change the application to send "batches" of
packets, so I can configure the number of "in flight" requests.

>>   1. from the HW timestamp value and the time the application
>>   retrieves the timestamps (called "HW to Timestamp";
>>   2. from just before the sendto() being called in the application to
>>   the time the application retrieves the timestamp (called "Send to
>>   Timestamp"). I think this measurement is useful to make sure that
>>   the total time to send a packet and retrieve its timestamp hasn't
>>   degraded.
>> 
>> (all tests were done for 1M packets, and times are in nanoseconds)
>> 
>> Before:
>> 
>> HW to Timestamp
>> 	min: 9130
>> 	max: 143183
>
> what margin of error did phc2sys have here? Tens, hundreds, thousands of
> ns, more? Was it a controlled variable? "HW to Timestamp" implies a
> comparison of 2 times from 2 different time sources, kept in sync with
> each other.
>

Should have provided these numbers, sorry. Yes, I was using phc2sys to
keep those different clocks (CLOCK_TAI and the NIC phc) synchronized,
and the phc2sys measured offset was in the order of tens of nanoseconds,
usually less than 20.

>> 	percentile 99: 10379
>> 	percentile 99.99: 11510
>> Send to Timestamp
>> 	min: 18431
>> 	max: 196798
>> 	percentile 99: 19937
>> 	percentile 99.99: 26066
>> 
>> After:
>> 
>> HW to Timestamp
>> 	min: 7933
>> 	max: 31934
>
> so the reduction of the max "HW to Timestamp" from 143 us to 32 us all
> the way to user space is mostly due to the inline processing of the TX
> timestamp, within the hardirq handler, right? Can you measure how much
> it is due to that, and how much due to the PTP kthread (simplest way
> would be to keep the kthread, but remove the inline processing)? How
> many reschedules of the kthread there are per TX timestamp? Even a
> single set of 4 numbers, denoting the maximum numbers of reschedules per
> timestamp request, would be useful information.
>

I will get these numbers, it will be useful for answering the questions
raised by that other patch.

>> 	percentile 99: 8690
>> 	percentile 99.99: 10598
>> Send to Timestamp
>> 	min: 17291
>> 	max: 46327
>> 	percentile 99: 18268
>> 	percentile 99.99: 21575
>> 
>> The minimum times are not that different
>
> right, probably because the time to do a context switch to user space
> dominates

Yep. Context switches and reading the PCIe registers account for most of
it.

>
>> , but we can see a big improvement in the 'maximum' time.


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 next-queue v1 3/3] igc: Use ptp->aux_worker to retrieve TX timestamps

[Vinicius Costa Gomes]

Hi,

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 27.02.2023 21:45:34, Vinicius Costa Gomes wrote:
>> ptp->aux_worker is a kthread and allows the user to set it's priority,
>> which is useful for workloads that time synchronization is required.
>> 
>> As an optimization, when the interrupt is handled try to retrieve the
>> timestamps "inline", if they are not, schedule the workload. This
>> should reduce the delay before the TX timestamp is available to
>> userspace.
>> 
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>
> Can this optimization also be applied to the igb?
>

The idea behind this optimization applies, but I think that also
introducing something like patch 1/3 (the spinlock) would make the code
easier to reason about.


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 next-queue v1 1/3] igc: Fix race condition in PTP tx code

[Vinicius Costa Gomes]

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 27.02.2023 21:45:32, Vinicius Costa Gomes wrote:
>> From: Andre Guedes <andre.guedes@intel.com>
>> 
>> Currently, the igc driver supports timestamping only one tx packet at a
>> time. During the transmission flow, the skb that requires hardware
>> timestamping is saved in adapter->ptp_tx_skb. Once hardware has the
>> timestamp, an interrupt is delivered, and adapter->ptp_tx_work is
>> scheduled. In igc_ptp_tx_work(), we read the timestamp register, update
>> adapter->ptp_tx_skb, and notify the network stack.
>> 
>> While the thread executing the transmission flow (the user process
>> running in kernel mode) and the thread executing ptp_tx_work don't
>> access adapter->ptp_tx_skb concurrently, there are two other places
>> where adapter->ptp_tx_skb is accessed: igc_ptp_tx_hang() and
>> igc_ptp_suspend().
>> 
>> igc_ptp_tx_hang() is executed by the adapter->watchdog_task worker
>> thread which runs periodically so it is possible we have two threads
>> accessing ptp_tx_skb at the same time. Consider the following scenario:
>> right after __IGC_PTP_TX_IN_PROGRESS is set in igc_xmit_frame_ring(),
>> igc_ptp_tx_hang() is executed. Since adapter->ptp_tx_start hasn't been
>> written yet, this is considered a timeout and adapter->ptp_tx_skb is
>> cleaned up.
>> 
>> This patch fixes the issue described above by adding the ptp_tx_lock to
>> protect access to ptp_tx_skb and ptp_tx_start fields from igc_adapter.
>> Since igc_xmit_frame_ring() called in atomic context by the networking
>> stack, ptp_tx_lock is defined as a spinlock.
>> 
>> With the introduction of the ptp_tx_lock, the __IGC_PTP_TX_IN_PROGRESS
>> flag doesn't provide much of a use anymore so this patch gets rid of it.
>
> Since the igc PTP code is derived from igb, do we need this patch to be
> ported to the igb driver, too?

Yes, that would be good. Will add this to my todo, but I will be glad if
anyone beats me to it.

>
> regards,
> Marc
>
> -- 
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


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 next-queue v1 0/3] igc: Add support for multiple TX tstamp requests

[Miroslav Lichvar]

On Thu, Mar 09, 2023 at 02:57:28PM -0800, Vinicius Costa Gomes wrote:
> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> > If the application never generates multiple requests in flight, then
> > this evaluation is only testing patch 3 (and patches 1 and 2 only to the
> > extent that they don't cause a regression), right?
> >
> 
> That's right. I was more interested in not causing a regression. I could
> run the same test with two (or more) applications and give some numbers,
> but those numbers couldn't be directly compared with the current version
> of the code.
> 
> But good idea. I will change the application to send "batches" of
> packets, so I can configure the number of "in flight" requests.

If you wanted to see the impact on performance and accuracy of
timestamps at the same time, you could use chrony as an NTP server and
generate load with ntpperf. The server can be started as

# chronyd -d local allow 'hwtimestamp eth0' 'clientloglimit 10000000'

ntpperf needs to be started with the -I -H -o options to measure
the difference between server HW TX timestamp and client HW RX
timestamp. The PHC used by ntpperf needs to be synchronized, ideally
via a separate PHC (e.g. by ptp4l+phc2sys).

If the multi-timestamp patches work well, I'd expect the stddev
reported by ntpperf to stay low at a higher rate. See
https://github.com/mlichvar/ntpperf for some examples.

-- 
Miroslav Lichvar

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

Re: [Intel-wired-lan] [PATCH next-queue v1 0/3] igc: Add support for multiple TX tstamp requests

[Vinicius Costa Gomes]

Hi,

Miroslav Lichvar <mlichvar@redhat.com> writes:

> On Thu, Mar 09, 2023 at 02:57:28PM -0800, Vinicius Costa Gomes wrote:
>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
>> > If the application never generates multiple requests in flight, then
>> > this evaluation is only testing patch 3 (and patches 1 and 2 only to the
>> > extent that they don't cause a regression), right?
>> >
>> 
>> That's right. I was more interested in not causing a regression. I could
>> run the same test with two (or more) applications and give some numbers,
>> but those numbers couldn't be directly compared with the current version
>> of the code.
>> 
>> But good idea. I will change the application to send "batches" of
>> packets, so I can configure the number of "in flight" requests.
>
> If you wanted to see the impact on performance and accuracy of
> timestamps at the same time, you could use chrony as an NTP server and
> generate load with ntpperf. The server can be started as
>
> # chronyd -d local allow 'hwtimestamp eth0' 'clientloglimit 10000000'
>
> ntpperf needs to be started with the -I -H -o options to measure
> the difference between server HW TX timestamp and client HW RX
> timestamp. The PHC used by ntpperf needs to be synchronized, ideally
> via a separate PHC (e.g. by ptp4l+phc2sys).
>
> If the multi-timestamp patches work well, I'd expect the stddev
> reported by ntpperf to stay low at a higher rate. See
> https://github.com/mlichvar/ntpperf for some examples.

I was going to start the testing for the second version of the series
just now.

This suggestion sounds great, let's see how it's going to look.


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