Le 05/07/2024 à 17:47, Tom Rini a écrit :
On Sun, Jun 30, 2024 at 08:49:08PM +0200, Christophe Leroy wrote:
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;
pat16 = *(sout-2+2*OFF);
pat16 = *(sout-1+OFF); if (dist == 1)Now that the release is out, can you please do a new patch to re-introduce the optimization required on your platform? A revert of "bbacdd3ef7762fbdeab43ceea5205d1fd0f25bbd" is required first (and separate) and I'll push that shortly. If you don't have time currently, I can try and v2 your original patch with these changes included, thanks!
I will try to send something tomorrow.