mtd: revert "spi-nor: intel: provide a range for poll_timout"

PATCH Applied Luis Alberto Herrera 4 participants 8 messages Jun 10, 2020 – Jul 28, 2020

Subsystems (2)

Memory technology devices (MTD) SPI NOR

lore

Share & tools

lore

Luis Alberto Herrera

author

2020-06-10 22:47 UTC #0 lore raw

PATCH 1/1 mtd: revert "spi-nor: intel: provide a range for poll_timout"

This change reverts aba3a882a178: "mtd: spi-nor: intel: provide a range for poll_timout". That change introduces a performance regression when reading sequentially from flash. Logging calls to intel_spi_read without this change we get:

Start MTD read

[   20.045527] intel_spi_read(from=1800000, len=400000)
[   20.045527] intel_spi_read(from=1800000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)

End MTD read

With this change:

Start MTD read

[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)

End MTD read

Signed-off-by Luis Alberto Herrera <[email protected]>

---
 drivers/mtd/spi-nor/controllers/intel-spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mtd/spi-nor/controllers/intel-spi.c +2 -2

index 61d2a0ad2131..2b89361a0d3a 100644
@@ -292,7 +292,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi)
 	u32 val;
 
 	return readl_poll_timeout(ispi->base + HSFSTS_CTL, val,
-				  !(val & HSFSTS_CTL_SCIP), 40,
+				  !(val & HSFSTS_CTL_SCIP), 0,
 				  INTEL_SPI_TIMEOUT * 1000);
 }

@@ -301,7 +301,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
 	u32 val;
 
 	return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val,
-				  !(val & SSFSTS_CTL_SCIP), 40,
+				  !(val & SSFSTS_CTL_SCIP), 0,
 				  INTEL_SPI_TIMEOUT * 1000);
 }

Signature · 1 line

2.27.0.278.ge193c7cf3a9-goog

Mika Westerberg

Acked-by on v1 1/1

2020-06-11 10:39 UTC #1 lore raw

Quoted 23 lines · Luis Alberto Herrera

This change reverts aba3a882a178: "mtd: spi-nor: intel: provide a range for poll_timout". That change introduces a performance regression when reading sequentially from flash. Logging calls to intel_spi_read without this change we get:
Start MTD read
[   20.045527] intel_spi_read(from=1800000, len=400000)
[   20.045527] intel_spi_read(from=1800000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
End MTD read
With this change:
Start MTD read
[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)

Quoted 3 lines

End MTD read
Signed-off-by Luis Alberto Herrera <[email protected]>

Acked-by Mika Westerberg <[email protected]>

[email protected]

2020-06-26 10:57 UTC #2 lore raw

In reply to Luis Alberto Herrera

Quoted 35 lines · Luis Alberto Herrera

This change reverts aba3a882a178: "mtd: spi-nor: intel: provide a range for poll_timout". That change introduces a performance regression when reading sequentially from flash. Logging calls to intel_spi_read without this change we get:
Start MTD read
[   20.045527] intel_spi_read(from=1800000, len=400000)
[   20.045527] intel_spi_read(from=1800000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
End MTD read
With this change:
Start MTD read
[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)
End MTD read
Signed-off-by Luis Alberto Herrera <[email protected]>
---
 drivers/mtd/spi-nor/controllers/intel-spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/mtd/spi-nor/controllers/intel-spi.c +0 -0
index 61d2a0ad2131..2b89361a0d3a 100644

drivers/mtd/spi-nor/controllers/intel-spi.c static int intel_spi_wait_hw_busy(struct intel_spi *ispi) · line 295

Quoted 5 lines

drivers/mtd/spi-nor/controllers/intel-spi.c +1 -1

@@ -292,7 +292,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi)
        u32 val;
 
        return readl_poll_timeout(ispi->base + HSFSTS_CTL, val,
-                                 !(val & HSFSTS_CTL_SCIP), 40,
+                                 !(val & HSFSTS_CTL_SCIP), 0,

Would 10 us keep the performance as it was before?

Cheers,
ta

Alexander Sverdlin

Tested-by on v1 1/1

2020-07-22 16:37 UTC #3 lore raw

In reply to Luis Alberto Herrera

Hello Luis,

thank you for the patch!

Quoted 16 lines · Luis Alberto Herrera

This change reverts aba3a882a178: "mtd: spi-nor: intel: provide a range for poll_timout". That change introduces a performance regression when reading sequentially from flash. Logging calls to intel_spi_read without this change we get:
Start MTD read
[   20.045527] intel_spi_read(from=1800000, len=400000)
[   20.045527] intel_spi_read(from=1800000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
End MTD read
With this change:

Quoted 8 lines

Start MTD read

[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)

End MTD read

I've performed my testing as well and got the following results:

Vanilla Linux 4.9 (i.e. before the introduction of the offending patch):

dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.91981 s, 1.3 MB/s

Vanilla 4.19 (i.e. with offending patch):

dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 6.70891 s, 781 kB/s

4.19 + revert:

dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.90503 s, 1.3 MB/s

Therefore it looks good from my PoV:

Tested-by Alexander Sverdlin <[email protected]>

Quoted 29 lines

Signed-off-by Luis Alberto Herrera <[email protected]>

Acked-by Mika Westerberg <[email protected]>

---
 drivers/mtd/spi-nor/controllers/intel-spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mtd/spi-nor/controllers/intel-spi.c +2 -2

index 61d2a0ad2131..2b89361a0d3a 100644
@@ -292,7 +292,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi)
 	u32 val;
 
 	return readl_poll_timeout(ispi->base + HSFSTS_CTL, val,
-				  !(val & HSFSTS_CTL_SCIP), 40,
+				  !(val & HSFSTS_CTL_SCIP), 0,
 				  INTEL_SPI_TIMEOUT * 1000);
 }

@@ -301,7 +301,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
 	u32 val;
 
 	return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val,
-				  !(val & SSFSTS_CTL_SCIP), 40,
+				  !(val & SSFSTS_CTL_SCIP), 0,
 				  INTEL_SPI_TIMEOUT * 1000);
 }

Signature · 2 lines

Best regards,
Alexander Sverdlin.

[email protected]

2020-07-22 17:03 UTC #4 lore raw

Hi, Alexander,

Quoted 73 lines · Alexander Sverdlin

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Hello Luis,
thank you for the patch!
Quoted 16 lines · Luis Alberto Herrera
This change reverts aba3a882a178: "mtd: spi-nor: intel: provide a range for poll_timout". That change introduces a performance regression when reading sequentially from flash. Logging calls to intel_spi_read without this change we get:
Start MTD read
[   20.045527] intel_spi_read(from=1800000, len=400000)
[   20.045527] intel_spi_read(from=1800000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
End MTD read
With this change:
Quoted 8 lines
Start MTD read
[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   21.942922] intel_spi_read(from=1c00000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   23.784058] intel_spi_read(from=2000000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)
[   25.625006] intel_spi_read(from=2400000, len=400000)
End MTD read
I've performed my testing as well and got the following results:
Vanilla Linux 4.9 (i.e. before the introduction of the offending patch):
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.91981 s, 1.3 MB/s
Vanilla 4.19 (i.e. with offending patch):
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 6.70891 s, 781 kB/s
4.19 + revert:
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.90503 s, 1.3 MB/s
Therefore it looks good from my PoV:
Tested-by Alexander Sverdlin <[email protected]>
Quoted 13 lines
Signed-off-by Luis Alberto Herrera <[email protected]>
Acked-by Mika Westerberg <[email protected]>
---
 drivers/mtd/spi-nor/controllers/intel-spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/mtd/spi-nor/controllers/intel-spi.c +0 -0
index 61d2a0ad2131..2b89361a0d3a 100644
@@ -292,7 +292,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi)
      u32 val;
 

Quoted 2 lines

drivers/mtd/spi-nor/controllers/intel-spi.c static int intel_spi_wait_hw_busy(struct intel_spi *ispi) · line 295
Quoted 2 lines
drivers/mtd/spi-nor/controllers/intel-spi.c +0 -1
      return readl_poll_timeout(ispi->base + HSFSTS_CTL, val,
-                               !(val & HSFSTS_CTL_SCIP), 40,>> +                               !(val & HSFSTS_CTL_SCIP), 0,

would you put 10 us here

Quoted 9 lines

drivers/mtd/spi-nor/controllers/intel-spi.c static int intel_spi_wait_sw_busy(struct intel_spi *ispi) · line 304

Quoted 9 lines

drivers/mtd/spi-nor/controllers/intel-spi.c +1 -1

                                INTEL_SPI_TIMEOUT * 1000);
 }

@@ -301,7 +301,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
      u32 val;
 
      return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val,
-                               !(val & SSFSTS_CTL_SCIP), 40,
+                               !(val & SSFSTS_CTL_SCIP), 0,

also here, and re-do a test? I'm curios if the performance will be as it was before.

Thanks!

Quoted 9 lines

drivers/mtd/spi-nor/controllers/intel-spi.c static int intel_spi_wait_hw_busy(struct intel_spi *ispi) · line 298
Quoted 4 lines
drivers/mtd/spi-nor/controllers/intel-spi.c +0 -0
                                INTEL_SPI_TIMEOUT * 1000);
 }
 
--
Best regards,
Alexander Sverdlin.

Alexander Sverdlin

2020-07-23 09:06 UTC #5 lore raw

Hello Tudor,

Quoted 1 line · On 22/07/2020 19:03, [email protected] wrote:

On 7/22/20 7:37 PM, Alexander Sverdlin wrote:

[...]

Quoted 27 lines

Quoted 27 lines
I've performed my testing as well and got the following results:
Vanilla Linux 4.9 (i.e. before the introduction of the offending patch):
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.91981 s, 1.3 MB/s
Vanilla 4.19 (i.e. with offending patch):
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 6.70891 s, 781 kB/s
4.19 + revert:
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.90503 s, 1.3 MB/s
Therefore it looks good from my PoV:
Tested-by Alexander Sverdlin <[email protected]>

[...]

Quoted 7 lines

would you put 10 us here

Quoted 6 lines

                                INTEL_SPI_TIMEOUT * 1000);

}

(quoted diff) +0 -0

@@ -301,7 +301,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
      u32 val;

Quoted 6 lines

Quoted 3 lines
drivers/mtd/spi-nor/controllers/intel-spi.c static int intel_spi_wait_sw_busy(struct intel_spi *ispi) · line 304
Quoted 3 lines
drivers/mtd/spi-nor/controllers/intel-spi.c +1 -1
      return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val,
-                               !(val & SSFSTS_CTL_SCIP), 40,
+                               !(val & SSFSTS_CTL_SCIP), 0,
also here, and re-do a test? I'm curios if the performance will be as it was before.

with 10us it looks like this:

dd if=/dev/flash/by-name/... of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 4.33816 s, 1.2 MB/s

Which means, there is a performance regression and it would depend on the test case, how bad it will be...

Signature · 2 lines

Best regards,
Alexander Sverdlin.

[email protected]

2020-07-28 08:28 UTC #6 lore raw

Hi, Mika,

Quoted 32 lines · Alexander Sverdlin

Hello Tudor,
Quoted 1 line · On 22/07/2020 19:03, [email protected] wrote:
On 7/22/20 7:37 PM, Alexander Sverdlin wrote:
[...]
Quoted 24 lines
Quoted 24 lines
I've performed my testing as well and got the following results:
Vanilla Linux 4.9 (i.e. before the introduction of the offending patch):
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.91981 s, 1.3 MB/s
Vanilla 4.19 (i.e. with offending patch):
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 6.70891 s, 781 kB/s
4.19 + revert:
dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.90503 s, 1.3 MB/s

[cut]

Quoted 10 lines

with 10us it looks like this:
dd if=/dev/flash/by-name/... of=/dev/null bs=4k 1280+0 records in
1280+0 records out
5242880 bytes (5.2 MB, 5.0 MiB) copied, 4.33816 s, 1.2 MB/s
Which means, there is a performance regression and it would depend on the test case, how bad it will be...

We need a bit of a context here. Using a tight-loop for polling and having a 5 secs timeout is fishy. For anything that's expected to complete less than a few usec, it's usually better to poll continuously, but then a timeout of 5s is way too big. Can we shrink the timeout to few msecs?

I'll queue this to spi-nor/next to fix the perf regression, but I would like to continue the discussion and to come up with an incremental patch on top of this one.

Cheers,
ta

Tudor Ambarus

Applied

2020-07-28 09:56 UTC #7 lore raw

In reply to Luis Alberto Herrera

Quoted 12 lines · Luis Alberto Herrera

This change reverts aba3a882a178: "mtd: spi-nor: intel: provide a range for poll_timout". That change introduces a performance regression when reading sequentially from flash. Logging calls to intel_spi_read without this change we get:
Start MTD read
[   20.045527] intel_spi_read(from=1800000, len=400000)
[   20.045527] intel_spi_read(from=1800000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  282.199274] intel_spi_read(from=1c00000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)
[  544.351528] intel_spi_read(from=2000000, len=400000)

Quoted 3 lines

End MTD read
[...]

Applied to spi-nor/next, thanks!

[1/1] mtd: revert "spi-nor: intel: provide a range for poll_timout"

      https://git.kernel.org/mtd/c/e93a977367b2

Best regards,

Signature · 1 line

Tudor Ambarus <[email protected]>