From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755258AbcGHNT7 (ORCPT ); Fri, 8 Jul 2016 09:19:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39562 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754746AbcGHNTv (ORCPT ); Fri, 8 Jul 2016 09:19:51 -0400 Message-ID: <1467983987.13253.25.camel@redhat.com> Subject: Re: [PATCH 4/4] irqtime: drop local_irq_save/restore from irqtime_account_irq From: Rik van Riel To: Frederic Weisbecker Cc: linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@kernel.org, pbonzini@redhat.com, fweisbec@redhat.com, wanpeng.li@hotmail.com, efault@gmx.de, tglx@linutronix.de, rkrcmar@redhat.com Date: Fri, 08 Jul 2016 09:19:47 -0400 In-Reply-To: <20160708123010.GD30200@lerouge> References: <1467315350-3152-1-git-send-email-riel@redhat.com> <1467315350-3152-5-git-send-email-riel@redhat.com> <20160708123010.GD30200@lerouge> Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-qFfDeSsOg0P24wHI9JHE" Mime-Version: 1.0 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Fri, 08 Jul 2016 13:19:50 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-qFfDeSsOg0P24wHI9JHE Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2016-07-08 at 14:30 +0200, Frederic Weisbecker wrote: > On Thu, Jun 30, 2016 at 03:35:50PM -0400, riel@redhat.com wrote: > > From: Rik van Riel > >=20 > > 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. > >=20 > > 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. > >=20 > > Signed-off-by: Rik van Riel >=20 > 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. However, I have not gotten numbers yet, and have no problem with this patch being dropped for now. > > --- > > =C2=A0kernel/sched/cputime.c | 72 > > +++++++++++++++++++++++++++++++++++++++++--------- > > =C2=A0kernel/sched/sched.h=C2=A0=C2=A0=C2=A0| 38 +++++++++++++++++++++-= ---- > > =C2=A02 files changed, 90 insertions(+), 20 deletions(-) > >=20 > > diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c > > index a0aefd4c7ea6..b78991fac228 100644 > > --- a/kernel/sched/cputime.c > > +++ b/kernel/sched/cputime.c > > @@ -26,7 +26,9 @@ > > =C2=A0DEFINE_PER_CPU(u64, cpu_hardirq_time); > > =C2=A0DEFINE_PER_CPU(u64, cpu_softirq_time); > > =C2=A0 > > -static DEFINE_PER_CPU(u64, irq_start_time); > > +static DEFINE_PER_CPU(u64, hardirq_start_time); > > +static DEFINE_PER_CPU(u64, softirq_start_time); > > +static DEFINE_PER_CPU(u64, prev_hardirq_time); > > =C2=A0static int sched_clock_irqtime; > > =C2=A0 > > =C2=A0void enable_sched_clock_irqtime(void) > > @@ -41,6 +43,7 @@ void disable_sched_clock_irqtime(void) > > =C2=A0 > > =C2=A0#ifndef CONFIG_64BIT > > =C2=A0DEFINE_PER_CPU(seqcount_t, irq_time_seq); > > +DEFINE_PER_CPU(seqcount_t, softirq_time_seq); > > =C2=A0#endif /* CONFIG_64BIT */ > > =C2=A0 > > =C2=A0/* > > @@ -53,36 +56,79 @@ 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{ > > - unsigned long flags; > > + u64 prev_softirq_start; > > + bool leaving_softirq; > > + u64 prev_hardirq; > > + u64 hardirq_time; > > =C2=A0 s64 delta; > > =C2=A0 int cpu; > > =C2=A0 > > =C2=A0 if (!sched_clock_irqtime) > > =C2=A0 return; > > =C2=A0 > > - local_irq_save(flags); > > - > > =C2=A0 cpu =3D smp_processor_id(); > > - delta =3D sched_clock_cpu(cpu) - > > __this_cpu_read(irq_start_time); > > - __this_cpu_add(irq_start_time, delta); > > =C2=A0 > > - irq_time_write_begin(); > > + /* > > + =C2=A0* Hardirq time accounting is pretty straightforward. If > > not in > > + =C2=A0* hardirq context yet (entering hardirq), set the start > > time. > > + =C2=A0* If already in hardirq context (leaving), account the > > elapsed time. > > + =C2=A0*/ > > + if (irqtype =3D=3D HARDIRQ_OFFSET) { > > + bool leaving_hardirq =3D hardirq_count(); > > + delta =3D sched_clock_cpu(cpu) - > > __this_cpu_read(hardirq_start_time); > > + __this_cpu_add(hardirq_start_time, delta); > > + if (leaving_hardirq) { > > + hardirq_time_write_begin(); > > + __this_cpu_add(cpu_hardirq_time, delta); > > + hardirq_time_write_end(); > > + } >=20 > This doesn't seem to work with nesting hardirqs. >=20 > Thanks. Where does it break? enter hardirq A -> hardirq_start_time =3D now enter hardirq B -> hardirq_start_time =3D now, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0accoun= t already elapsed time leave hardirq B -> account elapsed time, set =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0hardir= q_start_time =3D now leave hardirq A -> account elapsed time What am I missing, except a softirq-style do-while loop to account for hardirq A being interrupted by hardirq B while updating the statistics? --=20 All Rights Reversed. --=-qFfDeSsOg0P24wHI9JHE 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 iQEcBAABCAAGBQJXf6hzAAoJEM553pKExN6DlikH/i1S+FD6A7BVgHKdPcoD15f3 EXqrO7YLJa7H4qXFh4Y23DtQ/Jhxsnkn71oBMlKYa6842gnAiHSrZtN2AgPE8VNg ZGLwSwsztrZ9qpmD9xZvsZpVvfmsaFdnHpisxx2CvXTOP6cnPV/qc+S//uNL+zaa uQBEitBGiGiM8lXlcK1XhGj+GVAo8VHVmzVesTHjvm9tlIdbK0lGaIO5n3p6j+El PCvIEpXFI+sl88RsxwV6GcabbGOEiAYTmbO090ZMliXNMrlwJvjLQOL52QsHkspp QhRANLachhBO62rqdbk9wV3YAr/q5cP3xyvfDCbifGbBt8X40MA4sJviu8U0xiE= =78Ut -----END PGP SIGNATURE----- --=-qFfDeSsOg0P24wHI9JHE--