Re: [PATCH v3 1/3] idle: add support for tasks that inject idle
From: Ingo Molnar <mingo@kernel.org>
Date: 2016-11-24 04:20:27
Also in:
lkml
* Jacob Pan [off-list ref] wrote:
From: Peter Zijlstra <peterz@infradead.org> Idle injection drivers such as Intel powerclamp and ACPI PAD drivers use realtime tasks to take control of CPU then inject idle. There are two issues with this approach:
'CPU' is capitalized properly here. And here:
#define PF_VCPU 0x00000010 /* I'm a virtual CPU */
But not later in the patch.
quoted hunk ↗ jump to hunk
+++ b/kernel/fork.c@@ -1819,6 +1819,9 @@ static __latent_entropy struct task_struct *copy_process( threadgroup_change_end(current); perf_event_fork(p); + /* do not inherit idle task */ + p->flags &= ~PF_IDLE;
Sloppy formulation: how can the idle task be inherited? Also, capitalize comments please.
+ /* + * If the arch has a polling bit, we maintain an invariant: + * + * Our polling bit is clear if we're not scheduled (i.e. if + * rq->curr != rq->idle). This means that, if rq->idle has + * the polling bit set, then setting need_resched is + * guaranteed to cause the cpu to reschedule. + */
'CPU' is not properly capitalized here.
+ /* In poll mode we reenable interrupts and spin. + * Also if we detected in the wakeup from idle + * path that the tick broadcast device expired + * for us, we don't want to go deep idle as we + * know that the IPI is going to arrive right + * away + */ + if (cpu_idle_force_poll || tick_check_broadcast_expired()) + cpu_idle_poll(); + else + cpuidle_idle_call(); + arch_cpu_idle_exit();
Sigh: that's not standard comment style, nor is it standard coding style. Also, multi-sentence comments should end with a full stop.
+ /* + * We promise to call sched_ttwu_pending and reschedule + * if need_resched is set while polling is set. That + * means that clearing polling needs to be visible + * before doing these things. + */
When referring to functions in comments please spell them as "fn()", i.e. with parentheses.
+ /*
+ * If duration is 0, we will return after a natural wake event occurs. If
+ * duration is none zero, we will go back to sleep if we were woken up earlier.
+ * We also set up a timer to make sure we don't oversleep in deep idle.
+ */
+ if (!duration_ms)
+ do_idle();
+ else {
+ hrtimer_init_on_stack(&timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ timer.function = idle_inject_timer_fn;
+ hrtimer_start(&timer, ms_to_ktime(duration_ms),
+ HRTIMER_MODE_REL_PINNED);
+ end_time = jiffies + msecs_to_jiffies(duration_ms);
+
+ while (time_after(end_time, jiffies))
+ do_idle();
+ }Curly braces should be balanced and nonsensical line breaks should be omitted. Also, comments should be read and grammar should be fixed. Thanks, Ingo