Messages in this thread | | | Subject | Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq | From | Rik van Riel <> | Date | Wed, 22 Jun 2016 17:55:19 -0400 |
| |
On Tue, 2016-06-21 at 23:49 +0200, Peter Zijlstra wrote: > On Thu, Jun 16, 2016 at 12:06:07PM -0400, riel@redhat.com 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.
It gets rid of a compiler warning, since gcc is not smart enough to know that the result of in_softirq() will be the same throughout the function.
Using a bool leaving_softirq = in_softirq() also gets rid of the warning, and makes the function a little more readable, so I am doing that.
> > + 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() ?
I played around with it a bit, and it seems that __this_cpu_read does not want to nest inside READ_ONCE. Nobody else seems to be doing that, either.
Back to READ_ONCE(per_cpu(,cpu)) it is...
> Maybe split the whole thing on irqtype at the very start, instead of > the > endless repeated branches?
I untangled the whole thing in the next version, which I will post after testing.
-- All rights reversed [unhandled content-type:application/pgp-signature] | |