Re: [PATCH v3 tip/core/rcu 1/9] rcu: Add call_rcu_tasks()

Peter Zijlstra
2014-08-09T18:34:23Z

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]