Re: [PATCH] hwrng: bcm2835: Fix hwrng throughput regression

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Stefan Wahren <wahrenst@gmx.net>
Cc: Olivia Mackall <olivia@selenic.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Florian Fainelli <florian.fainelli@broadcom.com>,
	Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	Mark Brown <broonie@kernel.org>,
	linux-crypto@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	bcm-kernel-feedback-list@broadcom.com
Subject: Re: [PATCH] hwrng: bcm2835: Fix hwrng throughput regression
Date: Sat, 26 Aug 2023 17:48:34 +0200	[thread overview]
Message-ID: <ZOoe0lOR9zpcAw5I@zx2c4.com> (raw)
In-Reply-To: <56b5000c-89d9-865e-035c-5baf730a5304@gmx.net>

On Sat, Aug 26, 2023 at 04:01:58PM +0200, Stefan Wahren wrote:
> Hi Jason,
> 
> Am 26.08.23 um 14:34 schrieb Jason A. Donenfeld:
> > On Sat, Aug 26, 2023 at 01:28:28PM +0200, Stefan Wahren wrote:
> >> The recent RCU stall fix caused a massive throughput regression of the
> >> hwrng on Raspberry Pi 0 - 3. So try to restore a similiar throughput
> >> as before the RCU stall fix.
> >>
> >> Some performance measurements on Raspberry Pi 3B+ (arm64/defconfig):
> >>
> >> sudo dd if=/dev/hwrng of=/dev/null count=1 bs=10000
> >>
> >> cpu_relax              ~138025 Bytes / sec
> >> hwrng_msleep(1000)         ~13 Bytes / sec
> >> usleep_range(100,200)   ~92141 Bytes / sec
> >>
> >> Fixes: 96cb9d055445 ("hwrng: bcm2835 - use hwrng_msleep() instead of cpu_relax()")
> >> Link: https://lore.kernel.org/linux-arm-kernel/bc97ece5-44a3-4c4e-77da-2db3eb66b128@gmx.net/
> >> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> >> ---
> >>   drivers/char/hw_random/bcm2835-rng.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/char/hw_random/bcm2835-rng.c b/drivers/char/hw_random/bcm2835-rng.c
> >> index e98fcac578d6..3f1b6aaa98ee 100644
> >> --- a/drivers/char/hw_random/bcm2835-rng.c
> >> +++ b/drivers/char/hw_random/bcm2835-rng.c
> >> @@ -14,6 +14,7 @@
> >>   #include <linux/printk.h>
> >>   #include <linux/clk.h>
> >>   #include <linux/reset.h>
> >> +#include <linux/delay.h>
> >>
> >>   #define RNG_CTRL	0x0
> >>   #define RNG_STATUS	0x4
> >> @@ -71,7 +72,7 @@ static int bcm2835_rng_read(struct hwrng *rng, void *buf, size_t max,
> >>   	while ((rng_readl(priv, RNG_STATUS) >> 24) == 0) {
> >>   		if (!wait)
> >>   			return 0;
> >> -		hwrng_msleep(rng, 1000);
> >> +		usleep_range(100, 200);
> >
> > I think we still need to use the hwrng_msleep function so that the sleep
> > remains cancelable. Maybe just change the 1000 to 100?
> i found that other hwrng driver like iproc-rng200 (Raspberry Pi 4) also
> use usleep_range().
> 
> Nevertheless here are more numbers:
> 
> usleep_range(200,400) : 47776 bytes / sec
> hwrng_msleep(20) : 715 bytes / sec
> 
> Changing to 100 ms won't be a real gain.

I'm fine with whatever number you want there. Maybe we need a
hwrng_usleep_range() that takes into account rng->dying like
hwrng_msleep() does? (And iproc-rng200 should probably use that too?)

Jason