Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1528750 > unrolled thread

[PATCH v3 0/3] Stop sched tick in idle injection task

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] Stop sched tick in idle injection task Jacob Pan <jacob.jun.pan@linux.intel.com> - 2016-11-23 21:20 +0100
    [PATCH v3 3/3] thermal/powerclamp: stop sched tick in forced idle Jacob Pan <jacob.jun.pan@linux.intel.com> - 2016-11-23 21:20 +0100
    Re: [PATCH v3 0/3] Stop sched tick in idle injection task "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-23 22:20 +0100
      Re: [PATCH v3 0/3] Stop sched tick in idle injection task Peter Zijlstra <peterz@infradead.org> - 2016-11-23 22:50 +0100
        Re: [PATCH v3 0/3] Stop sched tick in idle injection task "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-23 23:40 +0100
          Re: [PATCH v3 0/3] Stop sched tick in idle injection task Jacob Pan <jacob.jun.pan@linux.intel.com> - 2016-11-24 01:30 +0100
            Re: [PATCH v3 0/3] Stop sched tick in idle injection task "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-24 01:30 +0100
      Re: [PATCH v3 0/3] Stop sched tick in idle injection task Ingo Molnar <mingo@kernel.org> - 2016-11-24 06:40 +0100

#1528750 — [PATCH v3 0/3] Stop sched tick in idle injection task

Changelog:
v3:	- rearrange idle.c change based on Rafael's suggestion.

v2:
	- moved duration timer from powerclamp driver to play_idle()
	- unexport cpuidle_use_deepest_state
	- indentation fix

Idle injection drivers today use RT threads to run idle loop. There are
efficiency and accounting issues with the current intel_powerclamp.c
and acpi_pad.c. A while ago, I posted CFS based idle injection patch trying
to address them:
https://lkml.org/lkml/2015/11/13/576

Peter proposed another approach with the introduction of a PF_IDLE flag.
This patchset is based on his original posting:
https://lkml.org/lkml/2014/6/4/56

These patches apply on top of the kworker and cpu hotplug state machine
changes made to Intel powerclamp driver.
https://lkml.org/lkml/2016/10/17/362

Similar changes to ACPI PAD driver is developed along with other
enhancements. It will be posted after this patchset is accepted.

Jacob Pan (2):
  cpuidle: allow setting deepest idle
  thermal/powerclamp: stop sched tick in forced idle

Peter Zijlstra (1):
  idle: add support for tasks that inject idle

 drivers/cpuidle/cpuidle.c          |  11 +++
 drivers/thermal/intel_powerclamp.c |  35 +------
 include/linux/cpu.h                |   2 +
 include/linux/cpuidle.h            |   4 +-
 include/linux/sched.h              |   3 +-
 kernel/fork.c                      |   3 +
 kernel/sched/core.c                |   1 +
 kernel/sched/idle.c                | 183 +++++++++++++++++++++++--------------
 8 files changed, 139 insertions(+), 103 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1528754 — [PATCH v3 3/3] thermal/powerclamp: stop sched tick in forced idle

With the introduction of play_idle(), idle injection kthread can
go through the normal idle task processing to get correct accounting
and turn off scheduler tick when possible.

Hrtimer is used to wake up since timeout is most likely to occur
during idle injection.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/thermal/intel_powerclamp.c | 35 +----------------------------------
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/drivers/thermal/intel_powerclamp.c b/drivers/thermal/intel_powerclamp.c
index 745fcec..68dd963 100644
--- a/drivers/thermal/intel_powerclamp.c
+++ b/drivers/thermal/intel_powerclamp.c
@@ -93,7 +93,6 @@ struct powerclamp_worker_data {
 	struct kthread_worker *worker;
 	struct kthread_work balancing_work;
 	struct kthread_delayed_work idle_injection_work;
-	struct timer_list wakeup_timer;
 	unsigned int cpu;
 	unsigned int count;
 	unsigned int guard;
@@ -278,11 +277,6 @@ static u64 pkg_state_counter(void)
 	return count;
 }
 
-static void noop_timer(unsigned long foo)
-{
-	/* empty... just the fact that we get the interrupt wakes us up */
-}
-
 static unsigned int get_compensation(int ratio)
 {
 	unsigned int comp = 0;
@@ -432,7 +426,6 @@ static void clamp_balancing_func(struct kthread_work *work)
 static void clamp_idle_injection_func(struct kthread_work *work)
 {
 	struct powerclamp_worker_data *w_data;
-	unsigned long target_jiffies;
 
 	w_data = container_of(work, struct powerclamp_worker_data,
 			      idle_injection_work.work);
@@ -453,31 +446,7 @@ static void clamp_idle_injection_func(struct kthread_work *work)
 	if (should_skip)
 		goto balance;
 
-	target_jiffies = jiffies + w_data->duration_jiffies;
-	mod_timer(&w_data->wakeup_timer, target_jiffies);
-	if (unlikely(local_softirq_pending()))
-		goto balance;
-	/*
-	 * stop tick sched during idle time, interrupts are still
-	 * allowed. thus jiffies are updated properly.
-	 */
-	preempt_disable();
-	/* mwait until target jiffies is reached */
-	while (time_before(jiffies, target_jiffies)) {
-		unsigned long ecx = 1;
-		unsigned long eax = target_mwait;
-
-		/*
-		 * REVISIT: may call enter_idle() to notify drivers who
-		 * can save power during cpu idle. same for exit_idle()
-		 */
-		local_touch_nmi();
-		stop_critical_timings();
-		mwait_idle_with_hints(eax, ecx);
-		start_critical_timings();
-		atomic_inc(&idle_wakeup_counter);
-	}
-	preempt_enable();
+	play_idle(jiffies_to_msecs(w_data->duration_jiffies));
 
 balance:
 	if (clamping && w_data->clamping && cpu_online(w_data->cpu))
@@ -540,7 +509,6 @@ static void start_power_clamp_worker(unsigned long cpu)
 	w_data->cpu = cpu;
 	w_data->clamping = true;
 	set_bit(cpu, cpu_clamping_mask);
-	setup_timer(&w_data->wakeup_timer, noop_timer, 0);
 	sched_setscheduler(worker->task, SCHED_FIFO, &sparam);
 	kthread_init_work(&w_data->balancing_work, clamp_balancing_func);
 	kthread_init_delayed_work(&w_data->idle_injection_work,
@@ -572,7 +540,6 @@ static void stop_power_clamp_worker(unsigned long cpu)
 	 * a big deal. The balancing work is fast and destroy kthread
 	 * will wait for it.
 	 */
-	del_timer_sync(&w_data->wakeup_timer);
 	clear_bit(w_data->cpu, cpu_clamping_mask);
 	kthread_destroy_worker(w_data->worker);
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1528784

On Wed, Nov 23, 2016 at 9:13 PM, Jacob Pan
<jacob.jun.pan@linux.intel.com> wrote:
> Changelog:
> v3:     - rearrange idle.c change based on Rafael's suggestion.
>
> v2:
>         - moved duration timer from powerclamp driver to play_idle()
>         - unexport cpuidle_use_deepest_state
>         - indentation fix
>
> Idle injection drivers today use RT threads to run idle loop. There are
> efficiency and accounting issues with the current intel_powerclamp.c
> and acpi_pad.c. A while ago, I posted CFS based idle injection patch trying
> to address them:
> https://lkml.org/lkml/2015/11/13/576
>
> Peter proposed another approach with the introduction of a PF_IDLE flag.
> This patchset is based on his original posting:
> https://lkml.org/lkml/2014/6/4/56
>
> These patches apply on top of the kworker and cpu hotplug state machine
> changes made to Intel powerclamp driver.
> https://lkml.org/lkml/2016/10/17/362
>
> Similar changes to ACPI PAD driver is developed along with other
> enhancements. It will be posted after this patchset is accepted.
>
> Jacob Pan (2):
>   cpuidle: allow setting deepest idle
>   thermal/powerclamp: stop sched tick in forced idle
>
> Peter Zijlstra (1):
>   idle: add support for tasks that inject idle
>
>  drivers/cpuidle/cpuidle.c          |  11 +++
>  drivers/thermal/intel_powerclamp.c |  35 +------
>  include/linux/cpu.h                |   2 +
>  include/linux/cpuidle.h            |   4 +-
>  include/linux/sched.h              |   3 +-
>  kernel/fork.c                      |   3 +
>  kernel/sched/core.c                |   1 +
>  kernel/sched/idle.c                | 183 +++++++++++++++++++++++--------------
>  8 files changed, 139 insertions(+), 103 deletions(-)

Any objections anyone?

If not, I'll queue up this series for 4.10.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1528790

On Wed, Nov 23, 2016 at 10:12:46PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 23, 2016 at 9:13 PM, Jacob Pan

> 
> Any objections anyone?
> 
> If not, I'll queue up this series for 4.10.

1h30 is a bit short to expect people to have even seen this, let alone
reviewed.

[toc] | [prev] | [next] | [standalone]

#1528810

On Wed, Nov 23, 2016 at 10:45 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Nov 23, 2016 at 10:12:46PM +0100, Rafael J. Wysocki wrote:
>> On Wed, Nov 23, 2016 at 9:13 PM, Jacob Pan
>
>>
>> Any objections anyone?
>>
>> If not, I'll queue up this series for 4.10.
>
> 1h30 is a bit short to expect people to have even seen this, let alone
> reviewed.

I didn't say I'd queue it up right away. :-)

[toc] | [prev] | [next] | [standalone]

#1528876

On Wed, 23 Nov 2016 23:38:39 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:

> On Wed, Nov 23, 2016 at 10:45 PM, Peter Zijlstra
> <peterz@infradead.org> wrote:
> > On Wed, Nov 23, 2016 at 10:12:46PM +0100, Rafael J. Wysocki wrote:  
> >> On Wed, Nov 23, 2016 at 9:13 PM, Jacob Pan  
> >  
> >>
> >> Any objections anyone?
> >>
> >> If not, I'll queue up this series for 4.10.  
> >
> > 1h30 is a bit short to expect people to have even seen this, let
> > alone reviewed.  
> 
> I didn't say I'd queue it up right away. :-)

The powerclamp change is on top of Petr and Sebastian's kworker and cpu
hp state machine patches. I plan to put all powerclamp patches in one
series after 1 and 2 are accepted.

Thanks,

Jacob

[toc] | [prev] | [next] | [standalone]

#1528877

On Thu, Nov 24, 2016 at 1:25 AM, Jacob Pan
<jacob.jun.pan@linux.intel.com> wrote:
> On Wed, 23 Nov 2016 23:38:39 +0100
> "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>
>> On Wed, Nov 23, 2016 at 10:45 PM, Peter Zijlstra
>> <peterz@infradead.org> wrote:
>> > On Wed, Nov 23, 2016 at 10:12:46PM +0100, Rafael J. Wysocki wrote:
>> >> On Wed, Nov 23, 2016 at 9:13 PM, Jacob Pan
>> >
>> >>
>> >> Any objections anyone?
>> >>
>> >> If not, I'll queue up this series for 4.10.
>> >
>> > 1h30 is a bit short to expect people to have even seen this, let
>> > alone reviewed.
>>
>> I didn't say I'd queue it up right away. :-)
>
> The powerclamp change is on top of Petr and Sebastian's kworker and cpu
> hp state machine patches. I plan to put all powerclamp patches in one
> series after 1 and 2 are accepted.

OK

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1529002

* Rafael J. Wysocki <rafael@kernel.org> wrote:

> On Wed, Nov 23, 2016 at 9:13 PM, Jacob Pan
> <jacob.jun.pan@linux.intel.com> wrote:
> > Changelog:
> > v3:     - rearrange idle.c change based on Rafael's suggestion.
> >
> > v2:
> >         - moved duration timer from powerclamp driver to play_idle()
> >         - unexport cpuidle_use_deepest_state
> >         - indentation fix
> >
> > Idle injection drivers today use RT threads to run idle loop. There are
> > efficiency and accounting issues with the current intel_powerclamp.c
> > and acpi_pad.c. A while ago, I posted CFS based idle injection patch trying
> > to address them:
> > https://lkml.org/lkml/2015/11/13/576
> >
> > Peter proposed another approach with the introduction of a PF_IDLE flag.
> > This patchset is based on his original posting:
> > https://lkml.org/lkml/2014/6/4/56
> >
> > These patches apply on top of the kworker and cpu hotplug state machine
> > changes made to Intel powerclamp driver.
> > https://lkml.org/lkml/2016/10/17/362
> >
> > Similar changes to ACPI PAD driver is developed along with other
> > enhancements. It will be posted after this patchset is accepted.
> >
> > Jacob Pan (2):
> >   cpuidle: allow setting deepest idle
> >   thermal/powerclamp: stop sched tick in forced idle
> >
> > Peter Zijlstra (1):
> >   idle: add support for tasks that inject idle
> >
> >  drivers/cpuidle/cpuidle.c          |  11 +++
> >  drivers/thermal/intel_powerclamp.c |  35 +------
> >  include/linux/cpu.h                |   2 +
> >  include/linux/cpuidle.h            |   4 +-
> >  include/linux/sched.h              |   3 +-
> >  kernel/fork.c                      |   3 +
> >  kernel/sched/core.c                |   1 +
> >  kernel/sched/idle.c                | 183 +++++++++++++++++++++++--------------
> >  8 files changed, 139 insertions(+), 103 deletions(-)
> 
> Any objections anyone?
> 
> If not, I'll queue up this series for 4.10.

NAK. The first patch alone has a dozen of trivial problems - very sloppy.

Guys, please rework, double check and actually _READ_ the fine patches you are 
promoting and then in a couple of days I'll review this series again to see 
whether the NAK can be lifted ...

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web