On 2022-02-04 16:51:42 [+0100], Jason A. Donenfeld wrote:
> index 455615ac169a..3e54b90a3ff8 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1759,41 +1762,54 @@ u64 get_random_u64(void)
> unsigned long flags;
> struct batched_entropy *batch;
> static void *previous;
> + int next_gen;
>
> warn_unseeded_randomness(&previous);
>
> - batch = raw_cpu_ptr(&batched_entropy_u64);
> - spin_lock_irqsave(&batch->batch_lock, flags);
> - if (batch->position % ARRAY_SIZE(batch->entropy_u64) == 0) {
> + batch = this_cpu_ptr(&batched_entropy_u64);
> + local_lock_irqsave(&batch->lock, flags);
Does this compile and work? From the looks of it, this should be:
local_lock_irqsave(&batched_entropy_u64.lock, flags);
batch = this_cpu_ptr(&batched_entropy_u64);
and we could do s/this_cpu_ptr/raw_cpu_ptr/
Sebastian
On Fri, Feb 4, 2022 at 4:57 PM Sebastian Andrzej Siewior
<[email protected]> wrote:
> On 2022-02-04 16:51:42 [+0100], Jason A. Donenfeld wrote:
> > index 455615ac169a..3e54b90a3ff8 100644
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -1759,41 +1762,54 @@ u64 get_random_u64(void)
> > unsigned long flags;
> > struct batched_entropy *batch;
> > static void *previous;
> > + int next_gen;
> >
> > warn_unseeded_randomness(&previous);
> >
> > - batch = raw_cpu_ptr(&batched_entropy_u64);
> > - spin_lock_irqsave(&batch->batch_lock, flags);
> > - if (batch->position % ARRAY_SIZE(batch->entropy_u64) == 0) {
> > + batch = this_cpu_ptr(&batched_entropy_u64);
> > + local_lock_irqsave(&batch->lock, flags);
>
> Does this compile and work? From the looks of it, this should be:
>
> local_lock_irqsave(&batched_entropy_u64.lock, flags);
> batch = this_cpu_ptr(&batched_entropy_u64);
>
> and we could do s/this_cpu_ptr/raw_cpu_ptr/
Why raw_cpu_ptr? include/linux/percpu-defs.h says about raw_*() operations:
* Operations for contexts where we do not want to do any checks for
* preemptions. Unless strictly necessary, always use [__]this_cpu_*()
* instead.
So when I see a raw_*() percpu thing, I read it as "it is expected
that we can migrate after this point (or we're in some really weird
context where the normal context check doesn't work)". Is that
incorrect?
On Wed, Feb 16, 2022 at 9:01 PM Jann Horn <[email protected]> wrote:
>
> On Fri, Feb 4, 2022 at 4:57 PM Sebastian Andrzej Siewior
> <[email protected]> wrote:
> > On 2022-02-04 16:51:42 [+0100], Jason A. Donenfeld wrote:
> > > index 455615ac169a..3e54b90a3ff8 100644
> > > --- a/drivers/char/random.c
> > > +++ b/drivers/char/random.c
> > > @@ -1759,41 +1762,54 @@ u64 get_random_u64(void)
> > > unsigned long flags;
> > > struct batched_entropy *batch;
> > > static void *previous;
> > > + int next_gen;
> > >
> > > warn_unseeded_randomness(&previous);
> > >
> > > - batch = raw_cpu_ptr(&batched_entropy_u64);
> > > - spin_lock_irqsave(&batch->batch_lock, flags);
> > > - if (batch->position % ARRAY_SIZE(batch->entropy_u64) == 0) {
> > > + batch = this_cpu_ptr(&batched_entropy_u64);
> > > + local_lock_irqsave(&batch->lock, flags);
> >
> > Does this compile and work? From the looks of it, this should be:
> >
> > local_lock_irqsave(&batched_entropy_u64.lock, flags);
> > batch = this_cpu_ptr(&batched_entropy_u64);
> >
> > and we could do s/this_cpu_ptr/raw_cpu_ptr/
>
> Why raw_cpu_ptr? include/linux/percpu-defs.h says about raw_*() operations:
>
> * Operations for contexts where we do not want to do any checks for
> * preemptions. Unless strictly necessary, always use [__]this_cpu_*()
> * instead.
>
> So when I see a raw_*() percpu thing, I read it as "it is expected
> that we can migrate after this point (or we're in some really weird
> context where the normal context check doesn't work)". Is that
> incorrect?
If it says "contexts where we do not want to do any checks for
preemptions", then that would apply here I would think? We're taking a
local lock, which means afterwards there are no preemptions. For
context, the code that got committed after Sebastian's final review
is:
local_lock_irqsave(&batched_entropy_u32.lock, flags);
batch = raw_cpu_ptr(&batched_entropy_u32);
However, I think most other code uses this_cpu_ptr() instead? So not sure.
Jason
On 2022-02-16 21:58:14 [+0100], Jason A. Donenfeld wrote:
> > Why raw_cpu_ptr? include/linux/percpu-defs.h says about raw_*() operations:
> >
> > * Operations for contexts where we do not want to do any checks for
> > * preemptions. Unless strictly necessary, always use [__]this_cpu_*()
> > * instead.
> >
> > So when I see a raw_*() percpu thing, I read it as "it is expected
> > that we can migrate after this point (or we're in some really weird
> > context where the normal context check doesn't work)". Is that
> > incorrect?
>
> If it says "contexts where we do not want to do any checks for
> preemptions", then that would apply here I would think? We're taking a
> local lock, which means afterwards there are no preemptions. For
> context, the code that got committed after Sebastian's final review
> is:
>
> local_lock_irqsave(&batched_entropy_u32.lock, flags);
> batch = raw_cpu_ptr(&batched_entropy_u32);
>
> However, I think most other code uses this_cpu_ptr() instead? So not sure.
It depends what you are looking for.
raw_cpu_ptr(&batched_entropy_u32) give you the pointer to
&batched_entropy_u32 of _this_ CPU - the CPU you are currently running.
this_cpu_ptr() does the same except that it has smp_processor_id() in
it. smp_processor_id() will yell at you (given you enabled
CONFIG_DEBUG_PREEMPT) if the code can migrate to another CPU.
So it will yell at you unless:
- you disable preemption / migration so code can't migrate
- you run on a per-CPU thread i.e. a thread which is bound to a single
CPU and therefore can't migrate to another.
The local_lock_irqsave() acquires a lock / disables interrupt and
therefore can't migrate. So I suggested to use raw_cpu_ptr() as an
optimisation in the debug-case.
If you disable preemption before accessing a per-CPU variable with
this_cpu_ptr() then you _need_ to ensure that nobody is accessing the
variable from in-IRQ context. Nobody will yell at you. But if you use a
local-lock then you get lockdep annotation and lockdep will complain if
you use that variable always in process context and sometimes in-IRQ.
> Jason
Sebastian