lkml.org 
[lkml]   [2017]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH 0/5] perf/tracing/cpuhotplug: Fix locking order
    On Tue, May 16, 2017 at 05:46:06AM -0700, Paul E. McKenney wrote:
    > On Tue, May 16, 2017 at 10:19:23AM +0200, Peter Zijlstra wrote:
    > > On Mon, May 15, 2017 at 11:40:43AM -0700, Paul E. McKenney wrote:
    > >
    > > > Given that you acquire the global pmus_lock when doing the
    > > > get_online_cpus(), and given that CPU hotplug is rare, is it possible
    > > > to momentarily acquire the global pmus_lock in perf_event_init_cpu()
    > > > and perf_event_exit_cpu() and interact directly with that? Then perf
    > > > would presumably leave alone any outgoing CPU that had already executed
    > > > perf_event_exit_cpu(), and also any incoming CPU that had not already
    > > > executed perf_event_init_cpu().
    > > >
    > > > What prevents this approach from working?
    > >
    > > Lack of sleep probably ;-)
    >
    > I know that feeling...
    >
    > > I'd blame the kids, but those have actually been very good lately.
    >
    > I don't get that excuse anymore, all are on their own. So I need
    > to come up with some fresh excuses. ;-)
    >
    > > You're suggesting the below on top, right? I'll run it with lockdep
    > > enabled after I chase some regression..
    >
    > Something like this, yes. Maybe even exactly like this. ;-)

    Ah, one thing I forgot... If you are avoiding use of get_online_cpus(),
    you usually also have to be very careful about how you use things like
    cpu_online() and cpu_is_offline.

    Thanx, Paul

    > > ---
    > > --- a/kernel/events/core.c
    > > +++ b/kernel/events/core.c
    > > @@ -8997,7 +8997,6 @@ int perf_pmu_register(struct pmu *pmu, c
    > > {
    > > int cpu, ret;
    > >
    > > - get_online_cpus();
    > > mutex_lock(&pmus_lock);
    > > ret = -ENOMEM;
    > > pmu->pmu_disable_count = alloc_percpu(int);
    >
    > There is usually also some state check in here somewhere for the CPU
    > being offline from a perf perspective. Such a check might already exist,
    > but I must plead ignorance of perf.
    >
    > > @@ -9093,7 +9092,6 @@ int perf_pmu_register(struct pmu *pmu, c
    > > ret = 0;
    > > unlock:
    > > mutex_unlock(&pmus_lock);
    > > - put_online_cpus();
    > >
    > > return ret;
    > >
    > > @@ -11002,10 +11000,9 @@ static void perf_event_exit_cpu_context(
    > > struct perf_cpu_context *cpuctx;
    > > struct perf_event_context *ctx;
    > > struct pmu *pmu;
    > > - int idx;
    > >
    > > - idx = srcu_read_lock(&pmus_srcu);
    > > - list_for_each_entry_rcu(pmu, &pmus, entry) {
    > > + mutex_lock(&pmus_lock);
    >
    > If the state change checked for by perf_pmu_register() needs to be also
    > guarded by ctx->mutex, this looks right to me.
    >
    > Just for completeness, the other style is to maintain separate per-CPU
    > state, in which case you would instead acquire pmus_lock, mark this
    > CPU off-limits to more perf_pmu_register() usage, release pmus_lock,
    > then clean up any old usage.
    >
    > The approach you have here seems to work best when the cleanup
    > and initialization naturally mark the CPU as off limits and ready,
    > respectively. The other style seems to work best when you need a separate
    > indication of which CPUs are off limits and usable.
    >
    > RCU is an example of the other style, with the rcu_node structure's
    > ->qsmaskinitnext mask serving to mark which CPUs usable. One reason
    > that the other style works so well for RCU is that a CPU coming online
    > has no effect on the current grace period, so rcu_cpu_starting() just
    > sets the CPU's bit in ->qsmaskinitnext, which takes effect only once
    > the the next grace period starts.
    >
    > It is quite possible that many of the other use cases instead need to
    > use something like what you have here. I suspect that the common case
    > is that a CPU appearing or disappearing must have some immediate effect.
    >
    > > + list_for_each_entry(pmu, &pmus, entry) {
    > > cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
    > > ctx = &cpuctx->ctx;
    > >
    > > @@ -11014,7 +11011,7 @@ static void perf_event_exit_cpu_context(
    > > cpuctx->online = 0;
    > > mutex_unlock(&ctx->mutex);
    > > }
    > > - srcu_read_unlock(&pmus_srcu, idx);
    > > + mutex_unlock(&pmus_lock);
    > > }
    > > #else
    > >
    > > @@ -11027,12 +11024,11 @@ int perf_event_init_cpu(unsigned int cpu
    > > struct perf_cpu_context *cpuctx;
    > > struct perf_event_context *ctx;
    > > struct pmu *pmu;
    > > - int idx;
    > >
    > > perf_swevent_init_cpu(cpu);
    > >
    > > - idx = srcu_read_lock(&pmus_srcu);
    > > - list_for_each_entry_rcu(pmu, &pmus, entry) {
    > > + mutex_lock(&pmus_lock);
    > > + list_for_each_entry(pmu, &pmus, entry) {
    > > cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
    > > ctx = &cpuctx->ctx;
    > >
    > > @@ -11040,7 +11036,7 @@ int perf_event_init_cpu(unsigned int cpu
    > > cpuctx->online = 1;
    > > mutex_unlock(&ctx->mutex);
    > > }
    > > - srcu_read_unlock(&pmus_srcu, idx);
    > > + mutex_unlock(&pmus_lock);
    >
    > And same here.
    >
    > Again for completeness, the other style would be to mark this CPU
    > as ready for perf usage at the very end, protected by pmus_lock.
    >
    > Thanx, Paul
    >
    > > return 0;
    > > }
    > >

    \
     
     \ /
      Last update: 2017-05-16 16:28    [W:2.992 / U:0.364 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site