Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq

[Rik van Riel]

On Tue, 2016-06-21 at 23:49 +0200, Peter Zijlstra wrote:
> On Thu, Jun 16, 2016 at 12:06:07PM -0400, riel@xxxxxxxxxx wrote:
> > 
> > @@ -53,36 +56,72 @@ DEFINE_PER_CPU(seqcount_t, irq_time_seq);
> > Â * softirq -> hardirq, hardirq -> softirq
> > Â *
> > Â * When exiting hardirq or softirq time, account the elapsed time.
> > + *
> > + * When exiting softirq time, subtract the amount of hardirq time
> > that
> > + * interrupted this softirq run, to avoid double accounting of
> > that time.
> > Â */
> > Âvoid irqtime_account_irq(struct task_struct *curr, int irqtype)
> > Â{
> > +	u64 prev_softirq_start;
> > +	u64 prev_hardirq;
> > +	u64 hardirq_time;
> > +	s64 delta = 0;
> We appear to always assign to delta, so this initialization seems
> superfluous.
> 
> > 
> > Â	int cpu;
> > Â
> > Â	if (!sched_clock_irqtime)
> > Â		return;
> > Â
> > Â	cpu = smp_processor_id();
> Per this smp_processor_id() usage, preemption is disabled.

This code is called from the timer code. Surely preemption
is already disabled?

Should I change this into raw_smp_processor_id()?

> > 
> > +	/*
> > +	Â* Softirq context may get interrupted by hardirq context,
> > +	Â* on the same CPU. At softirq entry time the amount of
> > time
> > +	Â* spent in hardirq context is stored. At softirq exit
> > time,
> > +	Â* the time spent in hardirq context during the softirq is
> > +	Â* subtracted.
> > +	Â*/
> > +	prev_hardirq = __this_cpu_read(prev_hardirq_time);
> > +	prev_softirq_start = __this_cpu_read(softirq_start_time);
> > +
> > +	if (irqtype == HARDIRQ_OFFSET) {
> > +		delta = sched_clock_cpu(cpu) -
> > __this_cpu_read(hardirq_start_time);
> > +		__this_cpu_add(hardirq_start_time, delta);
> > +	} else do {
> > +		u64 now = sched_clock_cpu(cpu);
> > +		hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time,
> > cpu));
> Which makes this per_cpu(,cpu) usage somewhat curious. What's wrong
> with
> __this_cpu_read() ?

Is __this_cpu_read() as fast as per_cpu(,cpu) on all
architectures?

> > 
> > +
> > +		delta = now - prev_softirq_start;
> > +		if (in_serving_softirq()) {
> > +			/*
> > +			Â* Leaving softirq context. Avoid double
> > counting by
> > +			Â* subtracting hardirq time from this
> > interval.
> > +			Â*/
> > +			s64 hi_delta = hardirq_time -
> > prev_hardirq;
> > +			delta -= hi_delta;
> > +		} else {
> > +			/* Entering softirq context. Note start
> > times. */
> > +			__this_cpu_write(softirq_start_time, now);
> > +			__this_cpu_write(prev_hardirq_time,
> > hardirq_time);
> > +		}
> > +		/*
> > +		Â* If a hardirq happened during this calculation,
> > it may not
> > +		Â* have gotten a consistent snapshot. Try again.
> > +		Â*/
> > +	} while (hardirq_time !=
> > READ_ONCE(per_cpu(cpu_hardirq_time, cpu)));
> That whole thing is somewhat hard to read; but its far too late for
> me
> to suggest anything more readable :/

I only had 2 1/2 hours of sleep last night, so I will not
try to rewrite it now, but I will see if there is anything
I can do to make it more readable tomorrow.

If you have any ideas before then, please let me know :)

> > 
> > +	irq_time_write_begin(irqtype);
> > Â	/*
> > Â	Â* We do not account for softirq time from ksoftirqd here.
> > Â	Â* We want to continue accounting softirq time to
> > ksoftirqd thread
> > Â	Â* in that case, so as not to confuse scheduler with a
> > special task
> > Â	Â* that do not consume any time, but still wants to run.
> > Â	Â*/
> > +	if (irqtype == HARDIRQ_OFFSET && hardirq_count())
> > Â		__this_cpu_add(cpu_hardirq_time, delta);
> > +	else if (irqtype == SOFTIRQ_OFFSET && in_serving_softirq()
> > &&
> > +				curr != this_cpu_ksoftirqd())
> > Â		__this_cpu_add(cpu_softirq_time, delta);
> > Â
> > +	irq_time_write_end(irqtype);
> Maybe split the whole thing on irqtype at the very start, instead of
> the
> endless repeated branches?

Let me try if I can make things more readable that way.

Thanks for the review!

Rik
-- 
All Rights Reversed.

Attachment: signature.asc
Description: This is a digitally signed message part