Re: [tip:perf/urgent] perf/core: Fix lock inversion between perf,trace,cpuhp

Re: [tip:perf/urgent] perf/core: Fix lock inversion between perf,trace,cpuhp

[Peter Zijlstra]

On Wed, Jan 24, 2018 at 02:39:16AM -0800, tip-bot for Peter Zijlstra wrote:
> Commit-ID:  25695067c32e34047b5490436a90af507fda1c2e
> Gitweb:     https://git.kernel.org/tip/25695067c32e34047b5490436a90af507fda1c2e
> Author:     Peter Zijlstra <peterz@infradead.org>
> AuthorDate: Tue, 9 Jan 2018 13:10:30 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Wed, 24 Jan 2018 10:07:23 +0100
> 
> perf/core: Fix lock inversion between perf,trace,cpuhp
> 
> Lockdep gifted us with noticing the following race:
> 
>         perf_trace_init()
>           perf_trace_event_init()
>             perf_trace_event_reg()
>               tp_event->class->reg() := tracepoint_probe_register
>                   trace_point_add_func()
> 
>           perf_event_init_cpu()
> 
>         perf_event_task_disable()
>           mutex_lock(&current->perf_event_mutex)
> 
>         do_exit()
>           task_work_run()
>             __fput()
>               perf_release()
>                 perf_event_release_kernel()
>                   free_event()
>                     _free_event()
>                       event->destroy() := perf_trace_destroy
> 

You seem to have lost part of the Changelog, mine looks like:

        perf_trace_init()
#0        mutex_lock(&event_mutex)
          perf_trace_event_init()
            perf_trace_event_reg()
              tp_event->class->reg() := tracepoint_probe_register
#1              mutex_lock(&tracepoints_mutex)
                  trace_point_add_func()
#2                  static_key_enable()


#2      do_cpu_up()
          perf_event_init_cpu()
#3          mutex_lock(&pmus_lock)
#4          mutex_lock(&ctx->mutex)


        perf_event_task_disable()
          mutex_lock(&current->perf_event_mutex)
#4        ctx = perf_event_ctx_lock()
#5        perf_event_for_each_child()


        do_exit()
          task_work_run()
            __fput()
              perf_release()
                perf_event_release_kernel()
#4                mutex_lock(&ctx->mutex)
#5                mutex_lock(&event->child_mutex)
                  free_event()
                    _free_event()
                      event->destroy() := perf_trace_destroy
#0                      mutex_lock(&event_mutex);



Which shows an actual deadlock.

Re: [tip:perf/urgent] perf/core: Fix lock inversion between perf,trace,cpuhp

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Jan 24, 2018 at 02:39:16AM -0800, tip-bot for Peter Zijlstra wrote:
> > Commit-ID:  25695067c32e34047b5490436a90af507fda1c2e
> > Gitweb:     https://git.kernel.org/tip/25695067c32e34047b5490436a90af507fda1c2e
> > Author:     Peter Zijlstra <peterz@infradead.org>
> > AuthorDate: Tue, 9 Jan 2018 13:10:30 +0100
> > Committer:  Ingo Molnar <mingo@kernel.org>
> > CommitDate: Wed, 24 Jan 2018 10:07:23 +0100
> > 
> > perf/core: Fix lock inversion between perf,trace,cpuhp
> > 
> > Lockdep gifted us with noticing the following race:
> > 
> >         perf_trace_init()
> >           perf_trace_event_init()
> >             perf_trace_event_reg()
> >               tp_event->class->reg() := tracepoint_probe_register
> >                   trace_point_add_func()
> > 
> >           perf_event_init_cpu()
> > 
> >         perf_event_task_disable()
> >           mutex_lock(&current->perf_event_mutex)
> > 
> >         do_exit()
> >           task_work_run()
> >             __fput()
> >               perf_release()
> >                 perf_event_release_kernel()
> >                   free_event()
> >                     _free_event()
> >                       event->destroy() := perf_trace_destroy
> > 
> 
> You seem to have lost part of the Changelog, mine looks like:
> 
>         perf_trace_init()
> #0        mutex_lock(&event_mutex)
>           perf_trace_event_init()
>             perf_trace_event_reg()
>               tp_event->class->reg() := tracepoint_probe_register
> #1              mutex_lock(&tracepoints_mutex)
>                   trace_point_add_func()
> #2                  static_key_enable()
> 
> 
> #2      do_cpu_up()
>           perf_event_init_cpu()
> #3          mutex_lock(&pmus_lock)
> #4          mutex_lock(&ctx->mutex)
> 
> 
>         perf_event_task_disable()
>           mutex_lock(&current->perf_event_mutex)
> #4        ctx = perf_event_ctx_lock()
> #5        perf_event_for_each_child()
> 
> 
>         do_exit()
>           task_work_run()
>             __fput()
>               perf_release()
>                 perf_event_release_kernel()
> #4                mutex_lock(&ctx->mutex)
> #5                mutex_lock(&event->child_mutex)
>                   free_event()
>                     _free_event()
>                       event->destroy() := perf_trace_destroy
> #0                      mutex_lock(&event_mutex);
> 
> 
> 
> Which shows an actual deadlock.

Oh, the '#' was probably interpreted as a comment when I applied it ...

Thanks,

	Ingo

Re: [tip:perf/urgent] perf/core: Fix lock inversion between perf,trace,cpuhp

[Linus Torvalds]

On Wed, Jan 24, 2018 at 7:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Oh, the '#' was probably interpreted as a comment when I applied it ...

Indeed. That's a subtle gotcha when you cut-and-paste things into the
editor when editing a commit message (including the "git rebase -i" or
"git commit --amend" kind of editing).

If you just apply it as an email, the hash-marks at the beginning of
lines are not seen as comments - it's literally just the editing part
that uses them as comments.

I'm afraid this isn't really a git "bug". But it _is_ subtle and unlucky.

Note that only a hash-mark in the first column is seen as a comment
(unlike, say, shell programming), so you can avoid it with spacing. If
you quote code with #ifdef etc, indent it.

Maybe git should have used a different comment scheme, but I have to
admit that I think this is the first time I've heard of this issue
causing problems in practice. I'm sure it's triggered before, and
nobody has noticed (or I wasn't on the cc).

So the '#' thing _normally_ works, but yes, it can cause issues.

                   Linus

Re: [tip:perf/urgent] perf/core: Fix lock inversion between perf,trace,cpuhp

[Steven Rostedt]

On Wed, 24 Jan 2018 08:57:10 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Maybe git should have used a different comment scheme, but I have to
> admit that I think this is the first time I've heard of this issue
> causing problems in practice. I'm sure it's triggered before, and
> nobody has noticed (or I wasn't on the cc).

It does happen a bit in practice. It's nailed me a few times. I just
consider it as the way git works and didn't complain.
 
> 
> So the '#' thing _normally_ works, but yes, it can cause issues.

I just brought this up here:

 http://lkml.kernel.org/r/20180112121148.20778932@gandalf.local.home

And someone replied with a pointer to:

 https://github.com/git/git/blob/master/Documentation/RelNotes/2.0.0.txt

suggesting 

   git config commit.cleanup scissors

Where git looks for:

 # ------------------------ >8 ------------------------

and does not delete comments before it. But this could play havoc when
doing squash though.

-- Steve