linux-kernel - Re: [PATCH] mtd: revert "spi-nor: intel: provide a range for poll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <afddc35d-fcef-695c-1522-df871f169a2d@microchip.com>
Date:   Tue, 28 Jul 2020 08:28:22 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <alexander.sverdlin@...ia.com>, <luisalberto@...gle.com>
CC:     <vigneshr@...com>, <bbrezillon@...nel.org>, <richard@....at>,
        <jethro@...tanix.com>, <linux-kernel@...r.kernel.org>,
        <linux-mtd@...ts.infradead.org>, <miquel.raynal@...tlin.com>,
        <mika.westerberg@...ux.intel.com>
Subject: Re: [PATCH] mtd: revert "spi-nor: intel: provide a range for
 poll_timout"

Hi, Mika,

On 7/23/20 12:05 PM, Alexander Sverdlin wrote:
> 
> Hello Tudor,
> 
> On 22/07/2020 19:03, Tudor.Ambarus@...rochip.com wrote:
>> On 7/22/20 7:37 PM, Alexander Sverdlin wrote:
> 
> [...]
> 
>>> 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]

> 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