linux-kernel - Re: [tip:perf/core] perf: Per-pmu-per-cpu contexts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1284134177.402.111.camel@laptop>
Date:	Fri, 10 Sep 2010 17:56:17 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	mingo@...hat.com, hpa@...or.com, paulus@...ba.org,
	linux-kernel@...r.kernel.org, eranian@...glemail.com,
	yanmin_zhang@...ux.intel.com, robert.richter@....com,
	ming.m.lin@...el.com, tglx@...utronix.de, mingo@...e.hu,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	linux-tip-commits@...r.kernel.org
Subject: Re: [tip:perf/core] perf: Per-pmu-per-cpu contexts

On Fri, 2010-09-10 at 16:54 +0200, Frederic Weisbecker wrote:
> On Thu, Sep 09, 2010 at 07:51:53PM +0000, tip-bot for Peter Zijlstra wrote:
> > @@ -3745,18 +3757,20 @@ static void perf_event_task_ctx(struct perf_event_context *ctx,
> >  
> >  static void perf_event_task_event(struct perf_task_event *task_event)
> >  {
> > -	struct perf_cpu_context *cpuctx;
> >  	struct perf_event_context *ctx = task_event->task_ctx;
> > +	struct perf_cpu_context *cpuctx;
> > +	struct pmu *pmu;
> >  
> > -	rcu_read_lock();
> > -	cpuctx = &get_cpu_var(perf_cpu_context);
> > -	perf_event_task_ctx(&cpuctx->ctx, task_event);
> > +	rcu_read_lock_sched();
> > +	list_for_each_entry_rcu(pmu, &pmus, entry) {
> > +		cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
> > +		perf_event_task_ctx(&cpuctx->ctx, task_event);
> > +	}
> >  	if (!ctx)
> >  		ctx = rcu_dereference(current->perf_event_ctxp);
> 
> 
> 
> So, you say below that it works because synchronize_srcu(), that
> waits for qs after touching pmus, implies synchronize_sched(), right?

yep.

> And I guess you picked rcu_read_lock_sched() here because that preempt_disable()
> at the same time.

Mostly because preemption is already disabled there, and sync_srcu() was
implemented using sync_rcu_sched() primitives.

> That looks complicated but I guess that works.

Yeah, similar to the event lists which we protect with a mutex and a
spinlock, hold either to traverse, hold both to modify.

Depending on the situation we need to traverse the pmu list preemptible
-- for example when we need to take the above mentioned mutex (see
perf_event_exit_cpu_context), or non-preemptible, the above. So we need
to guard it using two different flavours of RCU too.

> That said there is also this rcu_dereference(current->perf_event_ctxp).
> Now, this ctx is released after srcu barrier right? So this should
> be srcu_dereference(). But then you seem to actually use rcu_read_lock_sched()
> as it's compatible, so this should be rcu_dereference_sched() ?

The task context is released using call_rcu(), and should be done under
rcu_read_lock(), I guess we should hold both rcu_read_lock() and
rcu_read_lock_sched() there to be correct.

> With the current state, rcu will whine.
> Moreover there seem to be too much game between the different rcu
> flavours here, and that breaks the reviewers parsing.

:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/