On Mon, Jun 12, 2023, Linus Torvalds wrote:
> This patch looks completely broken to me.
>
> You now do
>
> if (err == -EAGAIN)
> goto restart;
>
> *within* the RCU-guarded section, and the "goto restart" will guard it again.
What if we require that all guarded sections have explicit scoping? E.g. drop
the current version of guard() and rename scoped_guard() => guard(). And then
figure out macro magic to guard an entire function? E.g. something like
static void perf_pmu_output_stop(struct perf_event *event) fn_guard(rcu)
{
...
}
or just "guard(rcu)" if possible. IIUC, function scopes like that will be possible
once -Wdeclaration-after-statement goes away.
Bugs aside, IMO guards that are buried in the middle of a function and implicitly
scoped to the function are all too easy to overlook. Requiring explicit scoping
would make bugs like this easier to spot since the goto would jump out of scope
(and I assume prematurely release the resource/lock?). As a bonus, annotating
the function itself would also serve as documentation.
The only downside is that the code for function-scoped locks that are acquired
partway through the function would be more verbose and/or cumbersome to write,
but that can be mitigated to some extent, e.g. by moving the locked portion to a
separate helper.
> On Mon, Jun 12, 2023 at 2:39 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -7977,7 +7977,8 @@ static void perf_pmu_output_stop(struct
> > int err, cpu;
> >
> > restart:
> > - rcu_read_lock();
> > + /* cannot have a label in front of a decl */;
> > + guard(rcu)();
> > list_for_each_entry_rcu(iter, &event->rb->event_list, rb_entry) {
> > /*
> > * For per-CPU events, we need to make sure that neither they
> > @@ -7993,12 +7994,9 @@ static void perf_pmu_output_stop(struct
> > continue;
> >
> > err = cpu_function_call(cpu, __perf_pmu_output_stop, event);
> > - if (err == -EAGAIN) {
> > - rcu_read_unlock();
> > + if (err == -EAGAIN)
> > goto restart;
> > - }
> > }
> > - rcu_read_unlock();
> > }
