From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751885AbcFUWYT (ORCPT ); Tue, 21 Jun 2016 18:24:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37427 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751519AbcFUWYR (ORCPT ); Tue, 21 Jun 2016 18:24:17 -0400 Message-ID: <1466547814.8637.8.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: Tue, 21 Jun 2016 18:23:34 -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="=-aHLlwKJos+7bizh9vBNo" Mime-Version: 1.0 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 21 Jun 2016 22:23:38 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-aHLlwKJos+7bizh9vBNo 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. >=20 > >=20 > > =C2=A0 int cpu; > > =C2=A0 > > =C2=A0 if (!sched_clock_irqtime) > > =C2=A0 return; > > =C2=A0 > > =C2=A0 cpu =3D smp_processor_id(); > Per this smp_processor_id() usage, preemption is disabled. This code is called from the timer code. Surely preemption is already disabled? Should I change this into raw_smp_processor_id()? > >=20 > > + /* > > + =C2=A0* Softirq context may get interrupted by hardirq context, > > + =C2=A0* on the same CPU. At softirq entry time the amount of > > time > > + =C2=A0* spent in hardirq context is stored. At softirq exit > > time, > > + =C2=A0* the time spent in hardirq context during the softirq is > > + =C2=A0* subtracted. > > + =C2=A0*/ > > + prev_hardirq =3D __this_cpu_read(prev_hardirq_time); > > + prev_softirq_start =3D __this_cpu_read(softirq_start_time); > > + > > + 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() ? Is __this_cpu_read() as fast as per_cpu(,cpu) on all architectures? > >=20 > > + > > + delta =3D now - prev_softirq_start; > > + if (in_serving_softirq()) { > > + /* > > + =C2=A0* Leaving softirq context. Avoid double > > counting by > > + =C2=A0* subtracting hardirq time from this > > interval. > > + =C2=A0*/ > > + s64 hi_delta =3D hardirq_time - > > prev_hardirq; > > + delta -=3D hi_delta; > > + } else { > > + /* Entering softirq context. Note start > > times. */ > > + __this_cpu_write(softirq_start_time, now); > > + __this_cpu_write(prev_hardirq_time, > > hardirq_time); > > + } > > + /* > > + =C2=A0* If a hardirq happened during this calculation, > > it may not > > + =C2=A0* have gotten a consistent snapshot. Try again. > > + =C2=A0*/ > > + } while (hardirq_time !=3D > > READ_ONCE(per_cpu(cpu_hardirq_time, cpu))); > That whole thing is somewhat hard to read; but its far too late for > me > to suggest anything more readable :/ I only had 2 1/2 hours of sleep last night, so I will not try to rewrite it now, but I will see if there is anything I can do to make it more readable tomorrow. If you have any ideas before then, please let me know :) > >=20 > > + irq_time_write_begin(irqtype); > > =C2=A0 /* > > =C2=A0 =C2=A0* We do not account for softirq time from ksoftirqd here. > > =C2=A0 =C2=A0* We want to continue accounting softirq time to > > ksoftirqd thread > > =C2=A0 =C2=A0* in that case, so as not to confuse scheduler with a > > special task > > =C2=A0 =C2=A0* that do not consume any time, but still wants to run. > > =C2=A0 =C2=A0*/ > > + if (irqtype =3D=3D HARDIRQ_OFFSET && hardirq_count()) > > =C2=A0 __this_cpu_add(cpu_hardirq_time, delta); > > + else if (irqtype =3D=3D SOFTIRQ_OFFSET && in_serving_softirq() > > && > > + curr !=3D this_cpu_ksoftirqd()) > > =C2=A0 __this_cpu_add(cpu_softirq_time, delta); > > =C2=A0 > > + irq_time_write_end(irqtype); > Maybe split the whole thing on irqtype at the very start, instead of > the > endless repeated branches? Let me try if I can make things more readable that way. Thanks for the review! Rik --=20 All Rights Reversed. --=-aHLlwKJos+7bizh9vBNo 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 iQEcBAABCAAGBQJXab5mAAoJEM553pKExN6DhWwH/0PQwtEDTp8JPiQf8Kc/fCvw H0abPHnIF/kPqedLBJ1m+CuVxl3UyQlsHQpnINC9T3SPX5V7r21Nrq/SVD5o0kQJ hA8uFy9Cepzt4yuB5SlktwgphLZ0wJZrwTkbUtC64sOP9rI++WaB2OcDo1fFD2wd rv3ZRiV9lZ9SbiYbsJY/r0JKNzrMD1krebwfEWm2ILU05D+ne3aeqxWy3LVuyPcV Nf+s1Hhm6f4uj5VOizUXM2BI58uuSbmQmPK+RGsSLxFqjmvvuIJ4+HngSnSIwgQb BCJsIhBYghxTLdikWlUQRnIjHvzT0xaXGJZYt7XMXZzd471dL3nzlsVUEPoRAm8= =2AjG -----END PGP SIGNATURE----- --=-aHLlwKJos+7bizh9vBNo--