On Fri, Aug 08, 2014 at 01:58:26PM -0700, Paul E. McKenney wrote: > > > And on that, you probably should change rcu_sched_rq() to read: > > > > this_cpu_inc(rcu_sched_data.passed_quiesce); > > > > That avoids touching the per-cpu data offset. > > Hmmm... Interrupts are disabled,
No they are not, __schedule()->rcu_note_context_switch()->rcu_sched_qs()
is only called with preemption disabled.
We only disable IRQs later, where we take the rq->lock.
> so no need to further disable
> interrupts. Storing 1 works fine, no need to increment. If I followed
> the twisty per_cpu passages correctly, my guess is that you would like
> me to do something like this:
>
> __this_cpu_write(rcu_sched_data.passed_quiesce, 1);
>
> Does that work?
Yeah, should be more or less similar, the inc might be encoded shorter
due to not requiring an immediate, but who cares :-)
void rcu_sched_qs(int cpu)
{
if (trace_rcu_grace_period_enabled()) {
if (!__this_cpu_read(rcu_sched_data.passed_quiesce))
trace_rcu_grace_period(...);
}
__this_cpu_write(rcu_sched_data.passed_quiesce, 1);
}
Would further avoid emitting the conditional in the normal case where
the tracepoint is inactive.
Steve does it make sense to have __DO_TRACE() emit __trace_##name() to
avoid the double static_branch thing?
> > And it would be very good if we could avoid the unconditional IRQ flag
> > fiddling in rcu_preempt_note_context_switch(), them expensive, this
> > looks entirely feasibly in the 'normal' case where
> > t->rcu_read_unlock_special doesn't have RCU_READ_UNLOCK_NEED_QS set.
>
> Agreed, but sometimes RCU_READ_UNLOCK_NEED_QS is set.
>
> That said, I should probably revisit RCU_READ_UNLOCK_NEED_QS. A lot has
> changed since I wrote that code.
Sure, but a conditional testing RCU_READ_UNLOCK_NEED_QS is far cheaper
than poking the IRQ flags. That said, its not entirely clear to me why
that needs IRQs disabled at all, then again I didn't look long and I'm
sure its all subtle.
pgpZhrUd88Wwi.pgp
Description: PGP signature
