From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933273AbcALMl5 (ORCPT ); Tue, 12 Jan 2016 07:41:57 -0500 Received: from mail-wm0-f50.google.com ([74.125.82.50]:37399 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752778AbcALMlz (ORCPT ); Tue, 12 Jan 2016 07:41:55 -0500 Subject: Re: [RFC PATCH 2/2] sched: idle: IRQ based next prediction for idle period To: Thomas Gleixner References: <1452093774-17831-1-git-send-email-daniel.lezcano@linaro.org> <1452093774-17831-3-git-send-email-daniel.lezcano@linaro.org> Cc: peterz@infradead.org, rafael@kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, nicolas.pitre@linaro.org, vincent.guittot@linaro.org From: Daniel Lezcano Message-ID: <5694F490.3040300@linaro.org> Date: Tue, 12 Jan 2016 13:41:52 +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 On 01/08/2016 04:43 PM, Thomas Gleixner wrote: > On Wed, 6 Jan 2016, Daniel Lezcano wrote: >> +/** >> + * sched_irq_timing_free - free_irq callback >> + * >> + * @irq: the irq number to stop tracking >> + * @dev_id: not used at the moment >> + * >> + * This function will remove from the wakeup source prediction table. >> + */ >> +static void sched_irq_timing_free(unsigned int irq, void *dev_id) >> +{ >> + struct irq_desc *desc = irq_to_desc(irq); > > What has this code to fiddle with irq_desc? Nothing! > >> + raw_spin_lock_irqsave(&desc->lock, flags); >> + desc->timings = &irq_timings; >> + raw_spin_unlock_irqrestore(&desc->lock, flags); > > Bah, no. This belongs into the core code. Add that handler - and yes, that's > all you need - to your timing_ops and let the core code do it in a proper way. Ah, yes, you are right. That will be much more cleaner. >> +/** >> + * sched_idle_init - setup the interrupt tracking table >> + * >> + * At init time, some interrupts could have been setup and in the >> + * system life time, some devices could be setup. In order to track >> + * all interrupts we are interested in, we first register a couple of >> + * callback to keep up-to-date the interrupt tracking table and then >> + * we setup the table with the interrupt which were already set up. >> + */ >> +int __init sched_idle_init(void) >> +{ >> + struct irq_desc *desc; >> + unsigned int irq; >> + int ret; >> + >> + /* >> + * Register the setup/free irq callbacks, so new interrupt or >> + * freed interrupt will update their tracking. >> + */ >> + ret = register_irq_timings(&irqt_ops); >> + if (ret) { >> + pr_err("Failed to register timings ops\n"); >> + return ret; >> + } > > So that stuff is installed unconditionally. Is it used unconditionally as > well? Sorry, I am not sure to understand your question. If the kernel is compiled with CONFIG_CPU_IDLE_GOV_SCHED=y, this code is enabled and use the irq timings. The condition comes from the compilation option. Is that your question ? >> + /* >> + * For all the irq already setup, assign the timing callback. >> + * All interrupts with their desc NULL will be discarded. >> + */ >> + for_each_irq_desc(irq, desc) >> + sched_irq_timing_setup(irq, desc->action); > > No, no, no. This belongs into the core code register_irq_timings() function > which installs the handler into the irq descs with the proper protections and > once it has done that enables the static key. Ok. > The above is completely unprotected against interrupts being setup or even > freed concurrently. Ok, I will do the change. > Aside of that, you call that setup function in setup_irq for each action() and > here you call it only for the first one. Ah, right. Thanks for the review. -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog