Re: bcm2835-rng: Performance regression since 96cb9d055445

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Stefan Wahren <wahrenst@gmx.net>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
	Florian Fainelli <florian.fainelli@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	bcm-kernel-feedback-list@broadcom.com,
	linux-crypto@vger.kernel.org,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Andrei Coardos <aboutphysycs@gmail.com>,
	Martin Kaiser <martin@kaiser.cx>,
	Linux regressions mailing list <regressions@lists.linux.dev>
Subject: Re: bcm2835-rng: Performance regression since 96cb9d055445
Date: Fri, 25 Aug 2023 13:26:16 +0200	[thread overview]
Message-ID: <ZOiP2H_6pfhKN3fj@zx2c4.com> (raw)
In-Reply-To: <bc97ece5-44a3-4c4e-77da-2db3eb66b128@gmx.net>

Hi Stefan,

On Fri, Aug 25, 2023 at 01:14:55PM +0200, Stefan Wahren wrote:
> Hi,
> 
> i didn't find the time to fix the performance regression in bcm2835-rng
> which affects Raspberry Pi 0 - 3, so report it at least. AFAIK the first
> report about this issue was here [1] and identified the offending commit:
> 
> 96cb9d055445 ("hwrng: bcm2835 - use hwrng_msleep() instead of
>   cpu_relax()")
> 
> #regzbot introduced: 96cb9d055445
> 
> I was able to reproduce this issue with a Raspberry Pi 3 B+ on Linux
> 6.5-rc6 (arm64/defconfig).
> 
> Before:
> time sudo dd if=/dev/hwrng of=/dev/urandom count=1 bs=4096 status=none
> 
> real	3m29,002s
> user	0m0,018s
> sys	0m0,054s

That's not surprising. But also, does it matter? That script has
*always* been wrong. Writing to /dev/urandom like that has *never*
ensured that those bytes are taken into account immediately after. It's
just not how that interface works. So any assumptions based on that are
bogus, and that line effectively does nothing.

Fortunately, however, the kernel itself incorporates hwrng output into
the rng pool, so you don't need to think about doing it yourself.

So go ahead and remove that line from your script.

Now as far as the "regression" goes, we've made an already broken
userspace script take 3 minutes longer than usual, but it still does
eventually complete, so it's not making boot impossible or something.
How this relates to the "don't break userspace" rule might be a matter
of opinion. If you think it does, maybe send a patch to Herbert reducing
that sleep from 1000 to 100 and stating why with my background above,
and see if he agrees it's worth fixing. Or, if removing that line from
your scripts is good enough for you, that's also fine by me.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel