[PATCH] lzma: Fix decompression speed regression

[Stefan Roese]

Hi Christophe,

On 7/7/23 11:27, Christophe Leroy wrote:
> 
> 
> 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.

Agreed. We should do both, change schedule() and also fix the high
frequent callers.

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

Good idea. Let me see if I can come up with such a patch. Will take a
few days though as I'm traveling the next few days.

Thanks,
Stefan