[PATCH] zlib: Fix big performance regression

[Christophe Leroy]

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 at 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.