Le 07/07/2023 à 11:09, Stefan Roese a écrit : > [Vous ne recevez pas souvent de courriers de s...@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.le...@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.le...@csgroup.eu> >>> --- >>> lib/lzma/LzmaDec.c | 18 ++++-------------- >>> 1 file changed, 4 insertions(+), 14 deletions(-) >> >> Reviewed-by: Simon Glass <s...@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
