From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752058AbcFUW7c (ORCPT ); Tue, 21 Jun 2016 18:59:32 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:39616 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751846AbcFUW7a (ORCPT ); Tue, 21 Jun 2016 18:59:30 -0400 Date: Wed, 22 Jun 2016 00:28:32 +0200 From: Peter Zijlstra To: Rik van Riel 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 Subject: Re: [PATCH 5/5] irqtime: drop local_irq_save/restore from irqtime_account_irq Message-ID: <20160621222832.GU30909@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> <1466547814.8637.8.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1466547814.8637.8.camel@redhat.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 21, 2016 at 06:23:34PM -0400, Rik van Riel wrote: > > >   cpu = 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? That's what I said. > > > > > > + /* > > > +  * Softirq context may get interrupted by hardirq context, > > > +  * on the same CPU. At softirq entry time the amount of > > > time > > > +  * spent in hardirq context is stored. At softirq exit > > > time, > > > +  * the time spent in hardirq context during the softirq is > > > +  * subtracted. > > > +  */ > > > + prev_hardirq = __this_cpu_read(prev_hardirq_time); > > > + prev_softirq_start = __this_cpu_read(softirq_start_time); > > > + > > > + if (irqtype == HARDIRQ_OFFSET) { > > > + delta = sched_clock_cpu(cpu) - > > > __this_cpu_read(hardirq_start_time); > > > + __this_cpu_add(hardirq_start_time, delta); > > > + } else do { > > > + u64 now = sched_clock_cpu(cpu); > > > + hardirq_time = 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? Can't be slower. Don't get the argument though; you've used __this_cpu stuff all over the place, and here you use a per_cpu() for no reason. > > > > > > + > > > + delta = now - prev_softirq_start; > > > + if (in_serving_softirq()) { > > > + /* > > > +  * Leaving softirq context. Avoid double > > > counting by > > > +  * subtracting hardirq time from this > > > interval. > > > +  */ > > > + s64 hi_delta = hardirq_time - > > > prev_hardirq; > > > + delta -= hi_delta; > > > + } else { > > > + /* Entering softirq context. Note start > > > times. */ > > > + __this_cpu_write(softirq_start_time, now); > > > + __this_cpu_write(prev_hardirq_time, > > > hardirq_time); > > > + } > > > + /* > > > +  * If a hardirq happened during this calculation, > > > it may not > > > +  * have gotten a consistent snapshot. Try again. > > > +  */ > > > + } while (hardirq_time != > > > 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 :) Heh, step away from the computer ... ;-)