Two quick comments, both of them "this code is a bit odd" rather than
anything else.
On Tue, 1 Aug 2023 at 12:11, Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
>
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
Why is this file called "tasks.h"?
It's not a header file. It makes no sense. It's full of C code. It's
included in only one place. It's just _weird_.
However, more relevantly:
> + mutex_unlock(&rtp->tasks_gp_mutex);
> set_tasks_gp_state(rtp, RTGS_WAIT_CBS);
Isn't the tasks_gp_mutex the thing that protects the gp state here?
Shouldn't it be after setting?
> rcuwait_wait_event(&rtp->cbs_wait,
> (needgpcb = rcu_tasks_need_gpcb(rtp)),
> TASK_IDLE);
Also, looking at rcu_tasks_need_gpcb() that is now called outside the
lock, it does something quite odd.
At the very top of the function does
for (cpu = 0; cpu < smp_load_acquire(&rtp->percpu_dequeue_lim); cpu++) {
and 'smp_load_acquire()' is all about saying "everything *after* this
load is ordered,
But the way it is done in that loop, it is indeed done at the
beginning of the loop, but then it's done *after* the loop too, so the
last smp_load_acquire seems a bit nonsensical.
If you want to load a value and say "this value is now sensible for
everything that follows", I think you should load it *first*. No?
IOW, wouldn't the whole sequence make more sense as
dequeue_limit = smp_load_acquire(&rtp->percpu_dequeue_lim);
for (cpu = 0; cpu < dequeue_limit; cpu++) {
and say that everything in rcu_tasks_need_gpcb() is ordered wrt the
initial limit on entry?
I dunno. That use of "smp_load_acquire()" just seems odd. Memory
ordering is hard to understand to begin with, but then when you have
things like loops that do the same ordered load multiple times, it
goes from "hard to understand" to positively confusing.
Linus
