Re: [RFC PATCH 2/2] sched: idle: IRQ based next prediction for idle period

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 01/08/2016 04:43 PM, Thomas Gleixner wrote:
> On Wed, 6 Jan 2016, Daniel Lezcano wrote:

[ ... ]

>> +	/*
>> +	 * 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.
>
> The above is completely unprotected against interrupts being setup or even
> freed concurrently.
>
> Aside of that, you call that setup function in setup_irq for each action() and
> here you call it only for the first one.

Hi Thomas,

I went through the different comments and almost finished the changes 
but I think the 'register_ops' approach, which happens after some irq 
were setup, introduces some useless complexity and because of the desc 
lock section, the ops can't do memory allocation. Before going further, 
I am wondering if declaring the irq_timings_ops statically (read without 
'register_ops' - hence without a init time dependency) and calling the 
init/free ops from alloc_desc/free_desc wouldn't be cleaner and simpler.

What do you think ?

   -- 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