* [Intel-wired-lan] [PATCH iwl-net v4 1/4] igc: Fix race condition in PTP tx code
2023-06-07 21:32 [Intel-wired-lan] [PATCH iwl-net v4 0/4] igc: TX timestamping fixes Vinicius Costa Gomes
@ 2023-06-07 21:32 ` Vinicius Costa Gomes
2023-06-19 7:29 ` naamax.meir
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 2/4] igc: Check if hardware TX timestamping is enabled earlier Vinicius Costa Gomes
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Vinicius Costa Gomes @ 2023-06-07 21:32 UTC (permalink / raw)
To: intel-wired-lan; +Cc: Andre Guedes, vladimir.oltean, kurt, anthony.l.nguyen
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, and the irq safe variants
of lock/unlock are used.
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.
Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
drivers/net/ethernet/intel/igc/igc.h | 5 +-
drivers/net/ethernet/intel/igc/igc_main.c | 9 ++--
drivers/net/ethernet/intel/igc/igc_ptp.c | 61 ++++++++++++-----------
3 files changed, 43 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 34aebf00a512..7da0657ea48f 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -229,6 +229,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 are 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 1c4676882082..a61afa69975e 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1583,9 +1583,10 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
* 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)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
+ if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON && !adapter->ptp_tx_skb) {
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
tx_flags |= IGC_TX_FLAGS_TSTAMP;
@@ -1594,6 +1595,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..56128e55f5c0 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -603,6 +603,7 @@ 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;
@@ -610,7 +611,6 @@ static void igc_ptp_tx_timeout(struct igc_adapter *adapter)
dev_kfree_skb_any(adapter->ptp_tx_skb);
adapter->ptp_tx_skb = NULL;
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 +618,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 +641,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)
{
@@ -676,13 +678,7 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
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;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
/* Notify the stack and free the skb after we've unlocked */
skb_tstamp_tx(skb, &shhwtstamps);
@@ -693,24 +689,33 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
* 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 checks the TSYNCTXCTL valid bit to determine when
+ * a timestamp has been taken for the current stored skb.
*/
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;
+ tsynctxctl &= IGC_TSYNCTXCTL_TXTT_0;
+ if (!tsynctxctl) {
+ WARN_ONCE(1, "Received a TSTAMP interrupt but no TSTAMP is ready.\n");
+ goto unlock;
+ }
igc_ptp_tx_hwtstamp(adapter);
+
+unlock:
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
}
/**
@@ -959,6 +964,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
return;
}
+ spin_lock_init(&adapter->ptp_tx_lock);
spin_lock_init(&adapter->tmreg_lock);
INIT_WORK(&adapter->ptp_tx_work, igc_ptp_tx_work);
@@ -1023,7 +1029,6 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
cancel_work_sync(&adapter->ptp_tx_work);
dev_kfree_skb_any(adapter->ptp_tx_skb);
adapter->ptp_tx_skb = NULL;
- clear_bit_unlock(__IGC_PTP_TX_IN_PROGRESS, &adapter->state);
if (pci_device_is_present(adapter->pdev)) {
igc_ptp_time_save(adapter);
--
2.40.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v4 1/4] igc: Fix race condition in PTP tx code
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 1/4] igc: Fix race condition in PTP tx code Vinicius Costa Gomes
@ 2023-06-19 7:29 ` naamax.meir
0 siblings, 0 replies; 9+ messages in thread
From: naamax.meir @ 2023-06-19 7:29 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: vladimir.oltean, Andre Guedes, kurt, anthony.l.nguyen
On 6/8/2023 00:32, Vinicius Costa Gomes wrote:
> 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, and the irq safe variants
> of lock/unlock are used.
>
> 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.
>
> Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> drivers/net/ethernet/intel/igc/igc.h | 5 +-
> drivers/net/ethernet/intel/igc/igc_main.c | 9 ++--
> drivers/net/ethernet/intel/igc/igc_ptp.c | 61 ++++++++++++-----------
> 3 files changed, 43 insertions(+), 32 deletions(-)
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [PATCH iwl-net v4 2/4] igc: Check if hardware TX timestamping is enabled earlier
2023-06-07 21:32 [Intel-wired-lan] [PATCH iwl-net v4 0/4] igc: TX timestamping fixes Vinicius Costa Gomes
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 1/4] igc: Fix race condition in PTP tx code Vinicius Costa Gomes
@ 2023-06-07 21:32 ` Vinicius Costa Gomes
2023-06-20 6:54 ` naamax.meir
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 3/4] igc: Retrieve TX timestamp during interrupt handling Vinicius Costa Gomes
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 4/4] igc: Work around HW bug causing missing timestamps Vinicius Costa Gomes
3 siblings, 1 reply; 9+ messages in thread
From: Vinicius Costa Gomes @ 2023-06-07 21:32 UTC (permalink / raw)
To: intel-wired-lan; +Cc: vladimir.oltean, kurt, anthony.l.nguyen
Before requesting a packet transmission to be hardware timestamped,
check if the user has TX timestamping enabled. Fixes an issue that if
a packet was internally forwarded to the NIC, and it had the
SKBTX_HW_TSTAMP flag set, the driver would mark that timestamp as
skipped.
In reality, that timestamp was "not for us", as TX timestamp could
never be enabled in the NIC.
Checking if the TX timestamping is enabled earlier has a secondary
effect that when TX timestamping is disabled, there's no need to check
for timestamp timeouts.
We should only take care to free any pending timestamp when TX
timestamping is disabled, as that skb would never be released
otherwise.
Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
drivers/net/ethernet/intel/igc/igc.h | 1 +
drivers/net/ethernet/intel/igc/igc_main.c | 5 +--
drivers/net/ethernet/intel/igc/igc_ptp.c | 42 +++++++++++++++++++++--
3 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 7da0657ea48f..66fb67c17e4f 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -581,6 +581,7 @@ enum igc_ring_flags_t {
IGC_RING_FLAG_TX_CTX_IDX,
IGC_RING_FLAG_TX_DETECT_HANG,
IGC_RING_FLAG_AF_XDP_ZC,
+ IGC_RING_FLAG_TX_HWTSTAMP,
};
#define ring_uses_large_buffer(ring) \
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index a61afa69975e..d3cfb8e97ac8 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1578,7 +1578,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
}
}
- if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
+ if (unlikely(test_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags) &&
+ skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
/* FIXME: add support for retrieving timestamps from
* the other timer registers before skipping the
* timestamping request.
@@ -1586,7 +1587,7 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
unsigned long flags;
spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
- if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON && !adapter->ptp_tx_skb) {
+ if (!adapter->ptp_tx_skb) {
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
tx_flags |= IGC_TX_FLAGS_TSTAMP;
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index 56128e55f5c0..42f622ceb64b 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -536,9 +536,36 @@ static void igc_ptp_enable_rx_timestamp(struct igc_adapter *adapter)
wr32(IGC_TSYNCRXCTL, val);
}
+static void igc_ptp_clear_tx_tstamp(struct igc_adapter *adapter)
+{
+ unsigned long flags;
+
+ 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;
+
+ spin_unlock_irqrestore(&adapter->ptp_tx_lock, flags);
+}
+
static void igc_ptp_disable_tx_timestamp(struct igc_adapter *adapter)
{
struct igc_hw *hw = &adapter->hw;
+ int i;
+
+ /* Clear the flags first to avoid new packets to be enqueued
+ * for TX timestamping.
+ */
+ for (i = 0; i < adapter->num_tx_queues; i++) {
+ struct igc_ring *tx_ring = adapter->tx_ring[i];
+
+ clear_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags);
+ }
+
+ /* Now we can clean the pending TX timestamp requests. */
+ igc_ptp_clear_tx_tstamp(adapter);
wr32(IGC_TSYNCTXCTL, 0);
}
@@ -546,12 +573,23 @@ static void igc_ptp_disable_tx_timestamp(struct igc_adapter *adapter)
static void igc_ptp_enable_tx_timestamp(struct igc_adapter *adapter)
{
struct igc_hw *hw = &adapter->hw;
+ int i;
wr32(IGC_TSYNCTXCTL, IGC_TSYNCTXCTL_ENABLED | IGC_TSYNCTXCTL_TXSYNSIG);
/* Read TXSTMP registers to discard any timestamp previously stored. */
rd32(IGC_TXSTMPL);
rd32(IGC_TXSTMPH);
+
+ /* The hardware is ready to accept TX timestamp requests,
+ * notify the transmit path.
+ */
+ for (i = 0; i < adapter->num_tx_queues; i++) {
+ struct igc_ring *tx_ring = adapter->tx_ring[i];
+
+ set_bit(IGC_RING_FLAG_TX_HWTSTAMP, &tx_ring->flags);
+ }
+
}
/**
@@ -1026,9 +1064,7 @@ void igc_ptp_suspend(struct igc_adapter *adapter)
if (!(adapter->ptp_flags & IGC_PTP_ENABLED))
return;
- cancel_work_sync(&adapter->ptp_tx_work);
- dev_kfree_skb_any(adapter->ptp_tx_skb);
- adapter->ptp_tx_skb = NULL;
+ igc_ptp_clear_tx_tstamp(adapter);
if (pci_device_is_present(adapter->pdev)) {
igc_ptp_time_save(adapter);
--
2.40.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v4 2/4] igc: Check if hardware TX timestamping is enabled earlier
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 2/4] igc: Check if hardware TX timestamping is enabled earlier Vinicius Costa Gomes
@ 2023-06-20 6:54 ` naamax.meir
0 siblings, 0 replies; 9+ messages in thread
From: naamax.meir @ 2023-06-20 6:54 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: vladimir.oltean, kurt, anthony.l.nguyen
On 6/8/2023 00:32, Vinicius Costa Gomes wrote:
> Before requesting a packet transmission to be hardware timestamped,
> check if the user has TX timestamping enabled. Fixes an issue that if
> a packet was internally forwarded to the NIC, and it had the
> SKBTX_HW_TSTAMP flag set, the driver would mark that timestamp as
> skipped.
>
> In reality, that timestamp was "not for us", as TX timestamp could
> never be enabled in the NIC.
>
> Checking if the TX timestamping is enabled earlier has a secondary
> effect that when TX timestamping is disabled, there's no need to check
> for timestamp timeouts.
>
> We should only take care to free any pending timestamp when TX
> timestamping is disabled, as that skb would never be released
> otherwise.
>
> Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
> Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
> drivers/net/ethernet/intel/igc/igc.h | 1 +
> drivers/net/ethernet/intel/igc/igc_main.c | 5 +--
> drivers/net/ethernet/intel/igc/igc_ptp.c | 42 +++++++++++++++++++++--
> 3 files changed, 43 insertions(+), 5 deletions(-)
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [PATCH iwl-net v4 3/4] igc: Retrieve TX timestamp during interrupt handling
2023-06-07 21:32 [Intel-wired-lan] [PATCH iwl-net v4 0/4] igc: TX timestamping fixes Vinicius Costa Gomes
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 1/4] igc: Fix race condition in PTP tx code Vinicius Costa Gomes
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 2/4] igc: Check if hardware TX timestamping is enabled earlier Vinicius Costa Gomes
@ 2023-06-07 21:32 ` Vinicius Costa Gomes
2023-06-20 19:02 ` naamax.meir
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 4/4] igc: Work around HW bug causing missing timestamps Vinicius Costa Gomes
3 siblings, 1 reply; 9+ messages in thread
From: Vinicius Costa Gomes @ 2023-06-07 21:32 UTC (permalink / raw)
To: intel-wired-lan; +Cc: vladimir.oltean, kurt, anthony.l.nguyen
When the interrupt is handled, the TXTT_0 bit in the TSYNCTXCTL
register should already be set and the timestamp value already loaded
in the appropriate register.
This simplifies the handling, and reduces the latency for retrieving
the TX timestamp, which increase the amount of TX timestamps that can
be handled in a given time period.
As the "work" function doesn't run in a workqueue anymore, rename it
to something more sensible, a event handler.
Using ntpperf[1] we can see the following performance improvements:
Before:
$ sudo ./ntpperf -i enp3s0 -m 10:22:22:22:22:21 -d 192.168.1.3 -s 172.18.0.0/16 -I -H -o -37
| responses | TX timestamp offset (ns)
rate clients | lost invalid basic xleave | min mean max stddev
1000 100 0.00% 0.00% 0.00% 100.00% -56 +9 +52 19
1500 150 0.00% 0.00% 0.00% 100.00% -40 +30 +75 22
2250 225 0.00% 0.00% 0.00% 100.00% -11 +29 +72 15
3375 337 0.00% 0.00% 0.00% 100.00% -18 +40 +88 22
5062 506 0.00% 0.00% 0.00% 100.00% -19 +23 +77 15
7593 759 0.00% 0.00% 0.00% 100.00% +7 +47 +5168 43
11389 1138 0.00% 0.00% 0.00% 100.00% -11 +41 +5240 39
17083 1708 0.00% 0.00% 0.00% 100.00% +19 +60 +5288 50
25624 2562 0.00% 0.00% 0.00% 100.00% +1 +56 +5368 58
38436 3843 0.00% 0.00% 0.00% 100.00% -84 +12 +8847 66
57654 5765 0.00% 0.00% 100.00% 0.00%
86481 8648 0.00% 0.00% 100.00% 0.00%
129721 12972 0.00% 0.00% 100.00% 0.00%
194581 16384 0.00% 0.00% 100.00% 0.00%
291871 16384 27.35% 0.00% 72.65% 0.00%
437806 16384 50.05% 0.00% 49.95% 0.00%
After:
$ sudo ./ntpperf -i enp3s0 -m 10:22:22:22:22:21 -d 192.168.1.3 -s 172.18.0.0/16 -I -H -o -37
| responses | TX timestamp offset (ns)
rate clients | lost invalid basic xleave | min mean max stddev
1000 100 0.00% 0.00% 0.00% 100.00% -44 +0 +61 19
1500 150 0.00% 0.00% 0.00% 100.00% -6 +39 +81 16
2250 225 0.00% 0.00% 0.00% 100.00% -22 +25 +69 15
3375 337 0.00% 0.00% 0.00% 100.00% -28 +15 +56 14
5062 506 0.00% 0.00% 0.00% 100.00% +7 +78 +143 27
7593 759 0.00% 0.00% 0.00% 100.00% -54 +24 +144 47
11389 1138 0.00% 0.00% 0.00% 100.00% -90 -33 +28 21
17083 1708 0.00% 0.00% 0.00% 100.00% -50 -2 +35 14
25624 2562 0.00% 0.00% 0.00% 100.00% -62 +7 +66 23
38436 3843 0.00% 0.00% 0.00% 100.00% -33 +30 +5395 36
57654 5765 0.00% 0.00% 100.00% 0.00%
86481 8648 0.00% 0.00% 100.00% 0.00%
129721 12972 0.00% 0.00% 100.00% 0.00%
194581 16384 19.50% 0.00% 80.50% 0.00%
291871 16384 35.81% 0.00% 64.19% 0.00%
437806 16384 55.40% 0.00% 44.60% 0.00%
[1] https://github.com/mlichvar/ntpperf
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
drivers/net/ethernet/intel/igc/igc.h | 2 +-
drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
drivers/net/ethernet/intel/igc/igc_ptp.c | 15 +++++----------
3 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 66fb67c17e4f..a00738bf6b19 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -228,7 +228,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 are protected by the
* ptp_tx_lock.
*/
@@ -638,6 +637,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);
+void igc_ptp_tx_tstamp_event(struct igc_adapter *adapter);
#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 d3cfb8e97ac8..3ee77d523fce 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -5216,7 +5216,7 @@ static void igc_tsync_interrupt(struct igc_adapter *adapter)
if (tsicr & IGC_TSICR_TXTS) {
/* retrieve hardware timestamp */
- schedule_work(&adapter->ptp_tx_work);
+ igc_ptp_tx_tstamp_event(adapter);
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 42f622ceb64b..cf963a12a92f 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -540,8 +540,6 @@ static void igc_ptp_clear_tx_tstamp(struct igc_adapter *adapter)
{
unsigned long flags;
- cancel_work_sync(&adapter->ptp_tx_work);
-
spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
dev_kfree_skb_any(adapter->ptp_tx_skb);
@@ -724,16 +722,14 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
}
/**
- * igc_ptp_tx_work
- * @work: pointer to work struct
+ * igc_ptp_tx_tstamp_event
+ * @adapter: board private structure
*
- * This work function checks the TSYNCTXCTL valid bit to determine when
- * a timestamp has been taken for the current stored skb.
+ * Called when a TX timestamp interrupt happens to retrieve the
+ * timestamp and send it up to the socket.
*/
-static void igc_ptp_tx_work(struct work_struct *work)
+void igc_ptp_tx_tstamp_event(struct igc_adapter *adapter)
{
- struct igc_adapter *adapter = container_of(work, struct igc_adapter,
- ptp_tx_work);
struct igc_hw *hw = &adapter->hw;
unsigned long flags;
u32 tsynctxctl;
@@ -1004,7 +1000,6 @@ void igc_ptp_init(struct igc_adapter *adapter)
spin_lock_init(&adapter->ptp_tx_lock);
spin_lock_init(&adapter->tmreg_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;
--
2.40.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v4 3/4] igc: Retrieve TX timestamp during interrupt handling
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 3/4] igc: Retrieve TX timestamp during interrupt handling Vinicius Costa Gomes
@ 2023-06-20 19:02 ` naamax.meir
0 siblings, 0 replies; 9+ messages in thread
From: naamax.meir @ 2023-06-20 19:02 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: vladimir.oltean, kurt, anthony.l.nguyen
On 6/8/2023 00:32, Vinicius Costa Gomes wrote:
> When the interrupt is handled, the TXTT_0 bit in the TSYNCTXCTL
> register should already be set and the timestamp value already loaded
> in the appropriate register.
>
> This simplifies the handling, and reduces the latency for retrieving
> the TX timestamp, which increase the amount of TX timestamps that can
> be handled in a given time period.
>
> As the "work" function doesn't run in a workqueue anymore, rename it
> to something more sensible, a event handler.
>
> Using ntpperf[1] we can see the following performance improvements:
>
> Before:
>
> $ sudo ./ntpperf -i enp3s0 -m 10:22:22:22:22:21 -d 192.168.1.3 -s 172.18.0.0/16 -I -H -o -37
> | responses | TX timestamp offset (ns)
> rate clients | lost invalid basic xleave | min mean max stddev
> 1000 100 0.00% 0.00% 0.00% 100.00% -56 +9 +52 19
> 1500 150 0.00% 0.00% 0.00% 100.00% -40 +30 +75 22
> 2250 225 0.00% 0.00% 0.00% 100.00% -11 +29 +72 15
> 3375 337 0.00% 0.00% 0.00% 100.00% -18 +40 +88 22
> 5062 506 0.00% 0.00% 0.00% 100.00% -19 +23 +77 15
> 7593 759 0.00% 0.00% 0.00% 100.00% +7 +47 +5168 43
> 11389 1138 0.00% 0.00% 0.00% 100.00% -11 +41 +5240 39
> 17083 1708 0.00% 0.00% 0.00% 100.00% +19 +60 +5288 50
> 25624 2562 0.00% 0.00% 0.00% 100.00% +1 +56 +5368 58
> 38436 3843 0.00% 0.00% 0.00% 100.00% -84 +12 +8847 66
> 57654 5765 0.00% 0.00% 100.00% 0.00%
> 86481 8648 0.00% 0.00% 100.00% 0.00%
> 129721 12972 0.00% 0.00% 100.00% 0.00%
> 194581 16384 0.00% 0.00% 100.00% 0.00%
> 291871 16384 27.35% 0.00% 72.65% 0.00%
> 437806 16384 50.05% 0.00% 49.95% 0.00%
>
> After:
>
> $ sudo ./ntpperf -i enp3s0 -m 10:22:22:22:22:21 -d 192.168.1.3 -s 172.18.0.0/16 -I -H -o -37
> | responses | TX timestamp offset (ns)
> rate clients | lost invalid basic xleave | min mean max stddev
> 1000 100 0.00% 0.00% 0.00% 100.00% -44 +0 +61 19
> 1500 150 0.00% 0.00% 0.00% 100.00% -6 +39 +81 16
> 2250 225 0.00% 0.00% 0.00% 100.00% -22 +25 +69 15
> 3375 337 0.00% 0.00% 0.00% 100.00% -28 +15 +56 14
> 5062 506 0.00% 0.00% 0.00% 100.00% +7 +78 +143 27
> 7593 759 0.00% 0.00% 0.00% 100.00% -54 +24 +144 47
> 11389 1138 0.00% 0.00% 0.00% 100.00% -90 -33 +28 21
> 17083 1708 0.00% 0.00% 0.00% 100.00% -50 -2 +35 14
> 25624 2562 0.00% 0.00% 0.00% 100.00% -62 +7 +66 23
> 38436 3843 0.00% 0.00% 0.00% 100.00% -33 +30 +5395 36
> 57654 5765 0.00% 0.00% 100.00% 0.00%
> 86481 8648 0.00% 0.00% 100.00% 0.00%
> 129721 12972 0.00% 0.00% 100.00% 0.00%
> 194581 16384 19.50% 0.00% 80.50% 0.00%
> 291871 16384 35.81% 0.00% 64.19% 0.00%
> 437806 16384 55.40% 0.00% 44.60% 0.00%
>
> [1] https://github.com/mlichvar/ntpperf
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> drivers/net/ethernet/intel/igc/igc.h | 2 +-
> drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
> drivers/net/ethernet/intel/igc/igc_ptp.c | 15 +++++----------
> 3 files changed, 7 insertions(+), 12 deletions(-)
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [PATCH iwl-net v4 4/4] igc: Work around HW bug causing missing timestamps
2023-06-07 21:32 [Intel-wired-lan] [PATCH iwl-net v4 0/4] igc: TX timestamping fixes Vinicius Costa Gomes
` (2 preceding siblings ...)
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 3/4] igc: Retrieve TX timestamp during interrupt handling Vinicius Costa Gomes
@ 2023-06-07 21:32 ` Vinicius Costa Gomes
2023-06-22 8:59 ` naamax.meir
3 siblings, 1 reply; 9+ messages in thread
From: Vinicius Costa Gomes @ 2023-06-07 21:32 UTC (permalink / raw)
To: intel-wired-lan; +Cc: vladimir.oltean, kurt, anthony.l.nguyen
There's an hardware issue that can cause missing timestamps. The bug
is that the interrupt is only cleared if the IGC_TXSTMPH_0 register is
read.
The bug can cause a race condition if a timestamp is captured at the
wrong time, and we will miss that timestamp. To reduce the time window
that the problem is able to happen, in case no timestamp was ready, we
read the "previous" value of the timestamp registers, and we compare
with the "current" one, if it didn't change we can be reasonably sure
that no timestamp was captured. If they are different, we use the new
value as the captured timestamp.
The HW bug is not easy to reproduce, got to reproduce it when smashing
the NIC with timestamping requests from multiple applications (e.g.
multiple ntpperf instances + ptp4l), after 10s of minutes.
This workaround has more impact when multiple timestamp registers are
used, and the IGC_TXSTMPH_0 register always need to be read, so the
interrupt is cleared.
Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
drivers/net/ethernet/intel/igc/igc_ptp.c | 48 ++++++++++++++++++------
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index cf963a12a92f..32ef112f8291 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -685,14 +685,49 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
struct sk_buff *skb = adapter->ptp_tx_skb;
struct skb_shared_hwtstamps shhwtstamps;
struct igc_hw *hw = &adapter->hw;
+ u32 tsynctxctl;
int adjust = 0;
u64 regval;
if (WARN_ON_ONCE(!skb))
return;
- regval = rd32(IGC_TXSTMPL);
- regval |= (u64)rd32(IGC_TXSTMPH) << 32;
+ tsynctxctl = rd32(IGC_TSYNCTXCTL);
+ tsynctxctl &= IGC_TSYNCTXCTL_TXTT_0;
+ if (tsynctxctl) {
+ regval = rd32(IGC_TXSTMPL);
+ regval |= (u64)rd32(IGC_TXSTMPH) << 32;
+ } else {
+ /* There's a bug in the hardware that could cause
+ * missing interrupts for TX timestamping. The issue
+ * is that for new interrupts to be triggered, the
+ * IGC_TXSTMPH_0 register must be read.
+ *
+ * To avoid discarding a valid timestamp that just
+ * happened at the "wrong" time, we need to confirm
+ * that there was no timestamp captured, we do that by
+ * assuming that no two timestamps in sequence have
+ * the same nanosecond value.
+ *
+ * So, we read the "low" register, read the "high"
+ * register (to latch a new timestamp) and read the
+ * "low" register again, if "old" and "new" versions
+ * of the "low" register are different, a valid
+ * timestamp was captured, we can read the "high"
+ * register again.
+ */
+ u32 txstmpl_old, txstmpl_new;
+
+ txstmpl_old = rd32(IGC_TXSTMPL);
+ rd32(IGC_TXSTMPH);
+ txstmpl_new = rd32(IGC_TXSTMPL);
+
+ if (txstmpl_old == txstmpl_new)
+ return;
+
+ regval = txstmpl_new;
+ regval |= (u64)rd32(IGC_TXSTMPH) << 32;
+ }
if (igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval))
return;
@@ -730,22 +765,13 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
*/
void igc_ptp_tx_tstamp_event(struct igc_adapter *adapter)
{
- struct igc_hw *hw = &adapter->hw;
unsigned long flags;
- u32 tsynctxctl;
spin_lock_irqsave(&adapter->ptp_tx_lock, flags);
if (!adapter->ptp_tx_skb)
goto unlock;
- tsynctxctl = rd32(IGC_TSYNCTXCTL);
- tsynctxctl &= IGC_TSYNCTXCTL_TXTT_0;
- if (!tsynctxctl) {
- WARN_ONCE(1, "Received a TSTAMP interrupt but no TSTAMP is ready.\n");
- goto unlock;
- }
-
igc_ptp_tx_hwtstamp(adapter);
unlock:
--
2.40.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v4 4/4] igc: Work around HW bug causing missing timestamps
2023-06-07 21:32 ` [Intel-wired-lan] [PATCH iwl-net v4 4/4] igc: Work around HW bug causing missing timestamps Vinicius Costa Gomes
@ 2023-06-22 8:59 ` naamax.meir
0 siblings, 0 replies; 9+ messages in thread
From: naamax.meir @ 2023-06-22 8:59 UTC (permalink / raw)
To: Vinicius Costa Gomes, intel-wired-lan
Cc: vladimir.oltean, kurt, anthony.l.nguyen
On 6/8/2023 00:32, Vinicius Costa Gomes wrote:
> There's an hardware issue that can cause missing timestamps. The bug
> is that the interrupt is only cleared if the IGC_TXSTMPH_0 register is
> read.
>
> The bug can cause a race condition if a timestamp is captured at the
> wrong time, and we will miss that timestamp. To reduce the time window
> that the problem is able to happen, in case no timestamp was ready, we
> read the "previous" value of the timestamp registers, and we compare
> with the "current" one, if it didn't change we can be reasonably sure
> that no timestamp was captured. If they are different, we use the new
> value as the captured timestamp.
>
> The HW bug is not easy to reproduce, got to reproduce it when smashing
> the NIC with timestamping requests from multiple applications (e.g.
> multiple ntpperf instances + ptp4l), after 10s of minutes.
>
> This workaround has more impact when multiple timestamp registers are
> used, and the IGC_TXSTMPH_0 register always need to be read, so the
> interrupt is cleared.
>
> Fixes: 2c344ae24501 ("igc: Add support for TX timestamping")
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
> drivers/net/ethernet/intel/igc/igc_ptp.c | 48 ++++++++++++++++++------
> 1 file changed, 37 insertions(+), 11 deletions(-)
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 9+ messages in thread