[PATCH] lzma: Fix decompression speed regression

[Christophe Leroy]

Le 07/07/2023 à 11:09, Stefan Roese a écrit :
> [Vous ne recevez pas souvent de courriers de sr at denx.de. Découvrez 
> pourquoi ceci est important à 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hi Simon,
> Hi Christophe,
> 
> On 7/6/23 17:57, Simon Glass wrote:
>> On Wed, 5 Jul 2023 at 09:54, Christophe Leroy
>> <christophe.leroy at csgroup.eu> wrote:
>>>
>>> Uncompressing a 1.7Mbytes FIT image on U-boot 2023.04 takes
>>> approx 7s on a powerpc 8xx.
>>> The same on U-boot 2023.07-rc6 takes approx 28s unless watchdog
>>> is disabled.
>>>
>>> During that decompression, LzmaDec_DecodeReal() calls schedule
>>> 1.6 million times, that is every 4µs in average.
>>>
>>> In the past it used to be a call to WATCHDOG_RESET() which was
>>> just calling hw_watchdog_reset().
>>>
>>> But the combination of commit 29caf9305b6 ("cyclic: Use schedule()
>>> instead of WATCHDOG_RESET()") and commit 26e8ebcd7cb ("watchdog:
>>> mpc8xxx: Make it generic") results in an heavier processing.
>>>
>>> However, there is absolutely no point in calling schedule() that
>>> often.
>>>
>>> By moving and keeping only one call to schedule() in the main
>>> loop the number of calls is reduced to 1.2 million which is still
>>> too much. So add logic to only call schedule every 1024 times.
>>> That leads to a call to schedule approx every 6ms which is still
>>> far enough to entertain the watchdog which has a 1s timeout on
>>> powerpc 8xx.
>>>
>>> powerpc 8xx being one of the slowest targets we have today in
>>> U-boot, and most other watchdogs having a timeout of one minutes
>>> instead of one second like the 8xx, this fix should not have
>>> negative impact on other targets.
>>>
>>> Fixes: 29caf9305b6 ("cyclic: Use schedule() instead of 
>>> WATCHDOG_RESET()")
>>> Signed-off-by: Christophe Leroy <christophe.leroy at csgroup.eu>
>>> ---
>>>   lib/lzma/LzmaDec.c | 18 ++++--------------
>>>   1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> +Stefan Roese I wonder if we need a more general fix?
> 
> IIRC, we already have some code in the watchdog IF making sure, that the
> WDT reset is not called too often. I need to re-check here. And yes, we
> should probably either move this code to the common schedule() function
> or add it here. Best by adding a new Kconfig option, configuring the max
> schedule frequency, e.g. 1000 Hz.

Yes we may add a ratelimitation inside schedule() but if we only do that 
it will only be a plaster. It will still use a huge amount of CPU just 
for reading the time and checking the limit.

A caller shouldn't call schedule() million of times per second, I think 
the root cause need to be fixed anyway. LZMA has this problem, ZLIB has 
not. I didn't check other decompressors.

The best would be that schedule() throw a WARN_ONCE() when that happens 
so that we can identify frantic callers.

Christophe