Le 30/06/2024 à 20:28, Christophe Leroy a écrit :
Le 30/06/2024 à 16:54, Tom Rini a écrit :
On Sun, Jun 30, 2024 at 10:47:06AM +0200, Christophe Leroy wrote:
Le 27/06/2024 à 21:40, Tom Rini a écrit :
On Thu, Jun 27, 2024 at 01:34:55PM -0600, Tom Rini wrote:
On Thu, Jun 27, 2024 at 10:25:21AM +0200, Christophe Leroy wrote:
Commit 340fdf1303dc ("zlib: Port fix for CVE-2016-9841 to U-Boot") brings a big performance regression in inflate_fast(), which leads to watchdog timer reset on powerpc 8xx.
It looks like that commit does more than what it describe, it especially removed an important optimisation that was doing copies using halfwords instead of bytes. That unexpected change multiplied by almost 4 the time spent in inflate_fast() and increased by 40% the overall time needed to uncompress linux kernel image.
So partially revert that commit but keep post incrementation as it is the initial purpose of said commit.
Fixes: 340fdf1303dc ("zlib: Port fix for CVE-2016-9841 to U-Boot") Signed-off-by: Christophe Leroy christophe.leroy@csgroup.eu
lib/zlib/inffast.c | 51 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 11 deletions(-)
Both this, and my mostly revert lead to CI failures around compression tests: https://source.denx.de/u-boot/u-boot/-/jobs/859329
A full revert however, is fine.
Is it that ? :
FAILED test/py/tests/test_ut.py::test_ut[ut_compression_compression_test_gzip]
It seems like when the optimisation was added by commit cd514aeb996e ("zlib: Optimize decompression"), only the pre-increment implementation was available.
When POSTINC was added by commit e89516f031db ("zlib: split up to match original source tree"), I guess it was not verified because POSTINC is #undef by zlib.h.
With the following change I don't get the FAILED ut_compression_compression_test_gzip CI anymore (https://source.denx.de/u-boot/custodians/u-boot-mpc8xx/-/jobs/859937)
diff --git a/lib/zlib/inffast.c b/lib/zlib/inffast.c index c271d85ea1..5dc0574202 100644 --- a/lib/zlib/inffast.c +++ b/lib/zlib/inffast.c @@ -257,7 +257,7 @@ void inflate_fast(z_streamp strm, unsigned start) sfrom = (unsigned short *)(from - OFF); loops = len >> 1; do - PUP(sout) = get_unaligned(++sfrom); + PUP(sout) = get_unaligned(sfrom++); while (--loops); out = (unsigned char *)sout + OFF; from = (unsigned char *)sfrom + OFF;
Ah, thanks! Testing again now.
That's probably not enough. I thought other failures were unrelated but I gave it a try with a full revert and I get no failure at all with that so there must be other things.
I find that pat16 = *(sout-2+2*OFF) suspicious, sout being a short it means -4 bytes which is not what is expected I think.
That's it. I get all green with the following, see https://source.denx.de/u-boot/custodians/u-boot-mpc8xx/-/pipelines/21391
diff --git a/lib/zlib/inffast.c b/lib/zlib/inffast.c index c271d85ea1..69268caee8 100644 --- a/lib/zlib/inffast.c +++ b/lib/zlib/inffast.c @@ -257,14 +257,14 @@ void inflate_fast(z_streamp strm, unsigned start) sfrom = (unsigned short *)(from - OFF); loops = len >> 1; do - PUP(sout) = get_unaligned(++sfrom); + PUP(sout) = get_unaligned(sfrom++); while (--loops); out = (unsigned char *)sout + OFF; from = (unsigned char *)sfrom + OFF; } else { /* dist == 1 or dist == 2 */ unsigned short pat16;
- pat16 = *(sout-2+2*OFF); + pat16 = *(sout-1+OFF); if (dist == 1) #if defined(__BIG_ENDIAN) pat16 = (pat16 & 0xff) | ((pat16 & 0xff ) << 8);
Christophe