From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751349AbcFVVzY (ORCPT ); Wed, 22 Jun 2016 17:55:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38702 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751244AbcFVVzX (ORCPT ); Wed, 22 Jun 2016 17:55:23 -0400 Message-ID: <1466632519.15275.29.camel@redhat.com> Subject: Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq From: Rik van Riel To: Peter Zijlstra Cc: linux-kernel@vger.kernel.org, mingo@kernel.org, pbonzini@redhat.com, fweisbec@gmail.com, wanpeng.li@hotmail.com, efault@gmx.de, tglx@linutronix.de, rkrcmar@redhat.com Date: Wed, 22 Jun 2016 17:55:19 -0400 In-Reply-To: <20160621214934.GT30909@twins.programming.kicks-ass.net> References: <1466093167-27653-1-git-send-email-riel@redhat.com> <1466093167-27653-6-git-send-email-riel@redhat.com> <20160621214934.GT30909@twins.programming.kicks-ass.net> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-G5//XU0ig8TDVTB7zXGw" Mime-Version: 1.0 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 22 Jun 2016 21:55:22 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-G5//XU0ig8TDVTB7zXGw Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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: > >=20 > > @@ -53,36 +56,72 @@ DEFINE_PER_CPU(seqcount_t, irq_time_seq); > > =C2=A0 * softirq -> hardirq, hardirq -> softirq > > =C2=A0 * > > =C2=A0 * 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. > > =C2=A0 */ > > =C2=A0void irqtime_account_irq(struct task_struct *curr, int irqtype) > > =C2=A0{ > > + u64 prev_softirq_start; > > + u64 prev_hardirq; > > + u64 hardirq_time; > > + s64 delta =3D 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 =3D in_softirq() also gets rid of the warning, and makes the function a little more readable, so I am doing that. > > + if (irqtype =3D=3D HARDIRQ_OFFSET) { > > + delta =3D sched_clock_cpu(cpu) - > > __this_cpu_read(hardirq_start_time); > > + __this_cpu_add(hardirq_start_time, delta); > > + } else do { > > + u64 now =3D sched_clock_cpu(cpu); > > + hardirq_time =3D 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. =C2=A0Nobody 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. --=20 All rights reversed --=-G5//XU0ig8TDVTB7zXGw Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJXawlHAAoJEM553pKExN6DwNoIALZ0+KEdbD74dLadiYwRHRj2 gU4MEx7lGZ7eAXX/icBSEIQEyshUAjvvzvXIHkw/6xvVlDoxyP9ia1xagaFH0wRZ PuSST0Or99Kd7cQWz0Z0+Zh4V/mpTZZhGvkZ7lyYAFVyO5zjPOI3LbSFcgu1kEGK XHpg2QfSGVwwo76it8jEsjkU1NxmpsRZyLCtBhy6s9BX8z0u8ftgFM0knsIwm2+5 AY/OxPgnhHrWXbo2Vsz9g9G6Q30IhsdHObDwdLL71pfWct4r+ZojEoctK/bCt2mk xJS0ssJI2V9UINNbiSr+2DRS1kHu8go8Xc5BDC7P3M3y8+/XvGf2aeAvziVDNew= =BYUg -----END PGP SIGNATURE----- --=-G5//XU0ig8TDVTB7zXGw--