Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1429128

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq riel@redhat.com - 2016-06-16 18:10 +0200
  Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-06-22 00:30 +0200
    Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-06-22 00:40 +0200
    Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Peter Zijlstra <peterz@infradead.org> - 2016-06-22 01:00 +0200
  Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Peter Zijlstra <peterz@infradead.org> - 2016-06-22 00:50 +0200
    Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-06-23 00:00 +0200
      Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Paolo Bonzini <pbonzini@redhat.com> - 2016-06-23 16:00 +0200
        Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from  irqtime_account_irq Rik van Riel <riel@redhat.com> - 2016-06-23 17:30 +0200

csiph-web