lkml.org 
[lkml]   [2016]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 2/2] sched: idle: IRQ based next prediction for idle period
From
Date
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

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

\
 
 \ /
  Last update: 2016-01-12 14:21    [W:0.131 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site