Re: [PATCH]new ACPI processor driver to force CPUs idle

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Wed, 2009-06-24 at 12:13 +0800, Shaohua Li wrote:
> This patch supports the processor aggregator device. When OS gets one ACPI
> notification, the driver will idle some number of cpus.
> 
> To make CPU idle, the patch will create power saving thread. Scheduler
> will migrate the thread to preferred CPU. The thread has max priority and
> has SCHED_RR policy, so it can occupy one CPU. To save power, the thread will
> keep calling C-state instruction. Routine power_saving_thread() is the entry
> of the thread.
> 
> To avoid starvation, the thread will sleep 5% time for every second
> (current RT scheduler has threshold to avoid starvation, but if other
> CPUs are idle, the CPU can borrow CPU timer from other, so makes the mechanism
> not work here)
> 
> This approach (to force CPU idle) should hasn't impact to scheduler and tasks
> with affinity still can get chance to run even the tasks run on idled cpu. Any
> comments/suggestions are welcome.

> +static int power_saving_thread(void *data)
> +{
> +	struct sched_param param = {.sched_priority = MAX_RT_PRIO - 1};
> +	int do_sleep;
> +
> +	/*
> +	 * we just create a RT task to do power saving. Scheduler will migrate
> +	 * the task to any CPU.
> +	 */
> +	sched_setscheduler(current, SCHED_RR, &param);
> +

This is crazy and wrong.

1) cpusets can be so configured as to not have the full machine in a
single load-balance domain, eg. the above comment about the scheduler is
false.

2) you're running at MAX_RT_PRIO-1, this will mightily upset the
migration thread and kstopmachine bits.

3) you're going to starve RT processes by being of a higher priority,
even though you might gain enough idle time by simply moving SCHED_OTHER
tasks around.

4) you're introducing 57s latencies to processes that happen to get
scheduled on whatever CPU you end up on, not nice.

NACK