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

From: Daniel Lezcano
Date: Mon Jan 18 2016 - 08:22:02 EST

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

<> â Open source software for ARM SoCs

Follow Linaro: <> Facebook |
<!/linaroorg> Twitter |
<> Blog