linux-kernel - Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from irqtime_account

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1467993409.13253.31.camel@redhat.com>
Date:	Fri, 08 Jul 2016 11:56:49 -0400
From:	Rik van Riel <riel@...hat.com>
To:	Paolo Bonzini <pbonzini@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>
Cc:	linux-kernel@...r.kernel.org, peterz@...radead.org,
	mingo@...nel.org, fweisbec@...hat.com, wanpeng.li@...mail.com,
	efault@....de, tglx@...utronix.de, rkrcmar@...hat.com
Subject: Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from
 irqtime_account_irq

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, riel@...hat.com wrote:
> > > > From: Rik van Riel <riel@...hat.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 <riel@...hat.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.
Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)