Hi Jason,
On 05/07/2022 17:11, Jason A. Donenfeld wrote:
Hi Holger,
On Tue, Jul 05, 2022 at 04:58:30PM +0200, Holger Dengler wrote:
It is true, that the performance of the instruction is not really
relevant, but only for calls outside of an interrupt context. I did
some ftrace logging for the s390_random_get_seed_long() calls, and -
as you said - there are a few calls per minute. But there was also
some repeating calls in interrupt context. On systems with a huge
interrupt load, this can cause severe performance impacts. I've no
It'd be interesting to know more about this. The way you get
arch_random_get_seed_long() from irq context is:
get_random_{bytes,int,long,u32,u64}()
crng_make_state()
crng_reseed() <-- Rarely
extract_entropy()
arch_get_random_seed_long()
So if an irq user of get_random_xx() is the unlucky one in the minute
span who has to call crng_reseed() then, yea, that'll happen. But I
wonder about this luck aspect. What scenarios are you seeing where
this
happens all the time? Which driver is using random bytes *so* commonly
from irq context? Not that, per say, there's anything wrong with that,
but it could be eyebrow raising, and might point to de facto solutions
that mostly take care of this.
I saw a few calls in interrupt context during my tracing, but I didn't
look to see which ones they were. Let me figure that out in the next
few days and provide more information on that.
One such direction might be making a driver that does such a thing do
it
a little bit less, somehow. Another direction would be preferring
non-irqs to handle crng_reseed(), but not disallowing irqs entirely,
with a patch something like the one below. Or maybe there are other
ideas.
Reduce the number of trng in interrupt context is a possibility, but -
in my opinion - only one single trng instruction call in interrupt
context in one too much.
For the moment, I would propose to drop the buffering but also return
false, if arch_random_get_seed_long() is called in interrupt context.
diff --git a/arch/s390/include/asm/archrandom.h
b/arch/s390/include/asm/archrandom.h
index 2c6e1c6ecbe7..711357bdc464 100644
--- a/arch/s390/include/asm/archrandom.h
+++ b/arch/s390/include/asm/archrandom.h
@@ -32,7 +32,8 @@ static inline bool __must_check
arch_get_random_int(unsigned int *v)
static inline bool __must_check arch_get_random_seed_long(unsigned
long *v)
{
- if (static_branch_likely(&s390_arch_random_available)) {
+ if (static_branch_likely(&s390_arch_random_available) &&
+ !in_interrupt()) {
cpacf_trng(NULL, 0, (u8 *)v, sizeof(*v));
atomic64_add(sizeof(*v), &s390_arch_random_counter);
return true;
(on-top of your commit, without our buffering patch)
But all this is to say that having some more of the "mundane" details
about this might actually help us.
Jason
diff --git a/drivers/char/random.c b/drivers/char/random.c
index e3dd1dd3dd22..81df8cdf2a62 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -270,6 +270,9 @@ static bool crng_has_old_seed(void)
static bool early_boot = true;
unsigned long interval = CRNG_RESEED_INTERVAL;
+ if (in_hardirq())
+ interval += HZ * 10;
+
if (unlikely(READ_ONCE(early_boot))) {
time64_t uptime = ktime_get_seconds();
if (uptime >= CRNG_RESEED_INTERVAL / HZ * 2)
