diff mbox series

[v3,56/57] perf: Simplify perf_pmu_output_stop()

Message ID 20230612093541.598260416@infradead.org (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series Scope-based Resource Management | expand

Commit Message

Peter Zijlstra June 12, 2023, 9:08 a.m. UTC 
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Linus Torvalds June 12, 2023, 4:19 p.m. UTC | #1 
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.

So no. Sending out a series of 57 patches that can have these kinds of
bugs in it is not ok. By patch 56 (which just happened to come in
fairly early for me), all sane peoples eyes have glazed over and they
don't react to this kind of breakage any more.

                Linus

On Mon, Jun 12, 2023 at 2:39 AM Peter Zijlstra <peterz@infradead.org> 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();
>  }
Sean Christopherson June 12, 2023, 5:11 p.m. UTC | #2 
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@infradead.org> 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();
> >  }
Linus Torvalds June 12, 2023, 5:43 p.m. UTC | #3 
On Mon, Jun 12, 2023 at 10:12 AM Sean Christopherson <seanjc@google.com> wrote:
>
> 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?

Hmm. I didn't love the excessive scoping, but most of the cases I was
thinking about were for the freeing part, not the locking part.

I agree that explicit scoping might be a good idea for locks as a
rule, but that "entire function" case _is_ a special case, and I don't
see how to do it sanely with a scoping guard.

Unless you accept ugly syntax like

    int fn(..)
    { scoped_guard(rcu)() {
         ...
    }}

which is just a violation of our usual scoping indentation rules.

Of course, at that point, the "scoped" part doesn't actually buy us
anything either, so you'd probably just be better off listing all the
guarding locks, and make it be

    int fn(..)
    { guard(rcu)(); guard(mutex)(&mymutex); {
         ...
    }}

or whatever.

Ugly, ugly.

End result: I think the non-explicitly scoped syntax is pretty much
required for sane use. The scoped version just causes too much
indentation (or forces us to have the above kind of special "we don't
indent this" rules).

> IIUC, function scopes like that will be possible once
> -Wdeclaration-after-statement goes away.

Well, "-Wdeclaration-after-statement" already went away early in
Peter's series, because without that you can't sanely do the normal
"__free()" cleanup thng.

But no, it doesn't help the C syntax case.

If you were to wrap a whole function with a macro, you need to do some
rather ugly things. They are ugly things that we already do: see our
whole "SYSCALL_DEFINEx()" set of macros, so it's not *impossible*, but
it's not possible with normal C syntax (and the normal C
preprocessor).

Of course, one way to do the "whole function scope" is to just do it
in the caller, and not using the cleanup attribute at all.

IOW, we could have things like

   #define WRAP(a,b,c) \
        ({ __typeof__(b) __ret; a; __ret = (b); c; __ret; })

and then you can do things like

   #define guard_fn_mutex(mutex, fn) \
        WRAP(mutex_lock(mutex), fn, mutex_unlock(mutex))

or

   #define rcu_read_action(x) WRAP(rcu_read_lock(), x, rcu_read_unlock())

and now you can easily guard the call-site (or any simple expression
that doesn't include any non-local control flow). Nothing new and
fancy required.

But when you don't want to do the wrapping in the caller, you do want
to have a non-scoping guard at the top of the function, I suspect.

                 Linus
Peter Zijlstra June 12, 2023, 6:55 p.m. UTC | #4 
On Mon, Jun 12, 2023 at 09:19:19AM -0700, 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.

restart:
	guard(rcu)();
	list_for_each_entry_rcu(iter, &head, rb_entry) {
		...
		if (err == -EAGAIN)
			goto restart;
	}

So the restart is *before* the variable exists, eg. it's out-of-scope.

per the last email's guard.c, if changed like so:

void main(void)
{
	int done = 0;
restart:
	lock_guard(spin, moo, &lock);
	for (;!done;) {
		done = 1;
		goto restart;
	}
}

$ gcc -O2 -o guard guard.c && ./guard
spin_lock
spin_unlock
spin_lock
spin_unlock

Which is exactly the expected result.
diff mbox series

Patch

--- 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();
 }
 
 /*