Thread (14 messages) 14 messages, 4 authors, 2016-11-24

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help