[PATCH] iwlwifi: pcie: limit memory read spin time

[PATCH] iwlwifi: pcie: limit memory read spin time

[Luca Coelho]

From: Johannes Berg <johannes.berg@intel.com>

When we read device memory, we lock a spinlock, write the address we
want to read from the device and then spin in a loop reading the data
in 32-bit quantities from another register.

As the description makes clear, this is rather inefficient, incurring
a PCIe bus transaction for every read. In a typical device today, we
want to read 786k SMEM if it crashes, leading to 192k register reads.
Occasionally, we've seen the whole loop take over 20 seconds and then
triggering the soft lockup detector.

Clearly, it is unreasonable to spin here for such extended periods of
time.

To fix this, break the loop down into an outer and an inner loop, and
break out of the inner loop if more than half a second elapsed. To
avoid too much overhead, check for that only every 128 reads, though
there's no particular reason for that number. Then, unlock and relock
to obtain NIC access again, reprogram the start address and continue.

This will keep (interrupt) latencies on the CPU down to a reasonable
time.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Mordechay Goodstein <mordechay.goodstein@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 .../net/wireless/intel/iwlwifi/pcie/trans.c   | 36 ++++++++++++++-----
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index d2e69ad53b27..2fffbbc8462f 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -2156,18 +2156,36 @@ static int iwl_trans_pcie_read_mem(struct iwl_trans *trans, u32 addr,
 				   void *buf, int dwords)
 {
 	unsigned long flags;
-	int offs, ret = 0;
+	int offs = 0;
 	u32 *vals = buf;
 
-	if (iwl_trans_grab_nic_access(trans, &flags)) {
-		iwl_write32(trans, HBUS_TARG_MEM_RADDR, addr);
-		for (offs = 0; offs < dwords; offs++)
-			vals[offs] = iwl_read32(trans, HBUS_TARG_MEM_RDAT);
-		iwl_trans_release_nic_access(trans, &flags);
-	} else {
-		ret = -EBUSY;
+	while (offs < dwords) {
+		/* limit the time we spin here under lock to 1/2s */
+		ktime_t timeout = ktime_add_us(ktime_get(), 500 * USEC_PER_MSEC);
+
+		if (iwl_trans_grab_nic_access(trans, &flags)) {
+			iwl_write32(trans, HBUS_TARG_MEM_RADDR,
+				    addr + 4 * offs);
+
+			while (offs < dwords) {
+				vals[offs] = iwl_read32(trans,
+							HBUS_TARG_MEM_RDAT);
+				offs++;
+
+				/* calling ktime_get is expensive so
+				 * do it once in 128 reads
+				 */
+				if (offs % 128 == 0 && ktime_after(ktime_get(),
+								   timeout))
+					break;
+			}
+			iwl_trans_release_nic_access(trans, &flags);
+		} else {
+			return -EBUSY;
+		}
 	}
-	return ret;
+
+	return 0;
 }
 
 static int iwl_trans_pcie_write_mem(struct iwl_trans *trans, u32 addr,
-- 
2.28.0

Re: [PATCH] iwlwifi: pcie: limit memory read spin time

[Kalle Valo]

Luca Coelho <luca@coelho.fi> writes:

> From: Johannes Berg <johannes.berg@intel.com>
>
> When we read device memory, we lock a spinlock, write the address we
> want to read from the device and then spin in a loop reading the data
> in 32-bit quantities from another register.
>
> As the description makes clear, this is rather inefficient, incurring
> a PCIe bus transaction for every read. In a typical device today, we
> want to read 786k SMEM if it crashes, leading to 192k register reads.
> Occasionally, we've seen the whole loop take over 20 seconds and then
> triggering the soft lockup detector.
>
> Clearly, it is unreasonable to spin here for such extended periods of
> time.
>
> To fix this, break the loop down into an outer and an inner loop, and
> break out of the inner loop if more than half a second elapsed. To
> avoid too much overhead, check for that only every 128 reads, though
> there's no particular reason for that number. Then, unlock and relock
> to obtain NIC access again, reprogram the start address and continue.
>
> This will keep (interrupt) latencies on the CPU down to a reasonable
> time.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Mordechay Goodstein <mordechay.goodstein@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

I'll queue this to v5.10.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] iwlwifi: pcie: limit memory read spin time

[Kalle Valo]

Luca Coelho <luca@coelho.fi> wrote:

> From: Johannes Berg <johannes.berg@intel.com>
> 
> When we read device memory, we lock a spinlock, write the address we
> want to read from the device and then spin in a loop reading the data
> in 32-bit quantities from another register.
> 
> As the description makes clear, this is rather inefficient, incurring
> a PCIe bus transaction for every read. In a typical device today, we
> want to read 786k SMEM if it crashes, leading to 192k register reads.
> Occasionally, we've seen the whole loop take over 20 seconds and then
> triggering the soft lockup detector.
> 
> Clearly, it is unreasonable to spin here for such extended periods of
> time.
> 
> To fix this, break the loop down into an outer and an inner loop, and
> break out of the inner loop if more than half a second elapsed. To
> avoid too much overhead, check for that only every 128 reads, though
> there's no particular reason for that number. Then, unlock and relock
> to obtain NIC access again, reprogram the start address and continue.
> 
> This will keep (interrupt) latencies on the CPU down to a reasonable
> time.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Mordechay Goodstein <mordechay.goodstein@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>

Patch applied to wireless-drivers.git, thanks.

04516706bb99 iwlwifi: pcie: limit memory read spin time

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/iwlwifi.20201022165103.45878a7e49aa.I3b9b9c5a10002915072312ce75b68ed5b3dc6e14@changeid/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches