On Fri, 2016-07-08 at 16:34 +0200, Paolo Bonzini wrote: > > On 08/07/2016 15:19, Rik van Riel wrote: > > On Fri, 2016-07-08 at 14:30 +0200, Frederic Weisbecker wrote: > > > On Thu, Jun 30, 2016 at 03:35:50PM -0400, r...@redhat.com wrote: > > > > From: Rik van Riel <r...@redhat.com> > > > > > > > > Drop local_irq_save/restore from irqtime_account_irq. > > > > Instead, have softirq and hardirq track their time spent > > > > independently, with the softirq code subtracting hardirq > > > > time that happened during the duration of the softirq run. > > > > > > > > The softirq code can be interrupted by hardirq code at > > > > any point in time, but it can check whether it got a > > > > consistent snapshot of the timekeeping variables it wants, > > > > and loop around in the unlikely case that it did not. > > > > > > > > Signed-off-by: Rik van Riel <r...@redhat.com> > > > > > > So the purpose is to get rid of local_irq_save/restore()? > > > Is it really worth such complication? > > > > local_irq_save/restore are quite slow, and look like the > > largest source of overhead in irq time accounting. > > I'm looking at an upstream tree, without your patches applied, > but it seems to me that irqtime_account_irq is always called with > interrupts disabled: > > irqtime_account_irq > -> account_irq_enter_time > -> __irq_enter > -> HARDIRQ_ENTER [1] > -> irq_enter [3] > -> __do_softirq [1] > -> account_irq_exit_time > -> __do_softirq [1] > -> __irq_exit > -> HARDIRQ_EXIT [1] > -> irq_exit [2] > > [1] = does local_irq_disable/enable > [2] = contains WARN_ON_ONCE(!irqs_disabled()) > [3] = calls rcu_irq_enter(), which checks irqs_disabled() > > I don't think your first two patches change this, so perhaps it's > enough > to remove that local_irq-save/restore? Either this, or > ENEEDWEEKEND...
I think you are right! __do_softirq() calls account_irq_enter_time() with irqs already disabled, and also has irqs disabled when it calls account_irq_exit_time() This appears to be true for both ksoftirqd and softirq from irq context. This could simplify my patch series a lot :) -- All Rights Reversed.
signature.asc
Description: This is a digitally signed message part
