From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759288AbcAUKDn (ORCPT ); Thu, 21 Jan 2016 05:03:43 -0500 Received: from mail-wm0-f52.google.com ([74.125.82.52]:36334 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759031AbcAUKDj (ORCPT ); Thu, 21 Jan 2016 05:03:39 -0500 Subject: Re: [RFC V2 2/2] sched: idle: IRQ based next prediction for idle period To: Nicolas Pitre References: <1453305636-22156-1-git-send-email-daniel.lezcano@linaro.org> <1453305636-22156-3-git-send-email-daniel.lezcano@linaro.org> Cc: tglx@linutronix.de, peterz@infradead.org, rafael@kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, vincent.guittot@linaro.org From: Daniel Lezcano Message-ID: <56A0ACF8.3050703@linaro.org> Date: Thu, 21 Jan 2016 11:03:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Nico, On 01/20/2016 06:46 PM, Nicolas Pitre wrote: > On Wed, 20 Jan 2016, Daniel Lezcano wrote: > >> Many IRQs are quiet most of the time, or they tend to come in bursts of >> fairly equal time intervals within each burst. It is therefore possible >> to detect those IRQs with stable intervals and guestimate when the next >> IRQ event is most likely to happen. >> >> Examples of such IRQs may include audio related IRQs where the FIFO size >> and/or DMA descriptor size with the sample rate create stable intervals, >> block devices during large data transfers, etc. Even network streaming >> of multimedia content creates patterns of periodic network interface IRQs >> in some cases. >> >> This patch adds code to track the mean interval and variance for each IRQ >> over a window of time intervals between IRQ events. Those statistics can >> be used to assist cpuidle in selecting the most appropriate sleep state >> by predicting the most likely time for the next interrupt. >> >> Because the stats are gathered in interrupt context, the core computation >> is as light as possible. >> >> Signed-off-by: Daniel Lezcano >> --- [ ... ] >> +struct stats { >> + u64 sum; /* sum of values */ >> + u32 values[STATS_NR_VALUES]; /* array of values */ >> + unsigned char w_ptr; /* current window pointer */ > > Why did you change this from an unsigned int? > > This won't provide any memory space saving given that the structure has > to be padded up to the next 64-bit boundary. Ok, I will change it back to unsigned int. [ ... ] >> + for (i = 0; i < STATS_NR_VALUES; i++) { >> + s64 diff = s->values[i] - mean; >> + variance += (u64)diff * diff; >> + } > > This is completely wrong. Even more wrong than it used to be. I must > have expressed myself badly about this last time. > > To avoid any confusion, here's what the code should be: > > int i; > u64 variance = 0; > > for (i = 0; i < STATS_NR_VALUES; i++) { > s32 diff = s->values[i] - mean; > variance += (s64)diff * diff; > } > > [...] Aah, ok :) [ ... ] >> + if (diff > (1 << 20)) { > > You could use the USEC_PER_SEC constant here. It is already widely used > and would make the code even more obvious. Indeed. [ ... ] >> + /* >> + * There is no point attempting predictions on interrupts more >> + * than 1 second apart. This has no benefit for sleep state >> + * selection and increases the risk of overflowing our variance >> + * computation. Reset all stats in that case. >> + */ > > This comment is wrong. It is relevant in sched_irq_timing_handler() but > not here. Instead this should be something like: > > /* > * This interrupt last triggered more than a second ago. > * It is definitely not predictable for our purpose anymore. > */ Ok. [ ... ] >> + interval = w->stats.values[w->stats.w_ptr]; >> + if ((u64)((interval - mean) * (interval - mean)) > variance) > > s/u64/s64/ please. Noted. Thanks Nico for the review. -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog