nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

PATCH In review Vatika Harlalka 4 participants 10 messages Aug 13, 2015 – Sep 11, 2015 1 message not in the archive Scheduler

lore

Share & tools

lore

Vatika Harlalka

author

2015-08-13 09:25 UTC #0 lore raw

cover nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

This patchset is for offloading task_tick() to a remote housekeeping cpu. The larger aim is to stop ticks on nohz_full cpus. For this, extra work must be done by housekeeping cpus. So, task_tick is called from a delayed workqueue for nohz_full cpus and the work is requeued every second for those nohz_full cpus whose ticks are stopped while they are busy. In the rest of the cases it will lead to redundant accounting. To facilitate this, a new function tick_nohz_remote_tick_stopped is added to indicate whether ticks are stopped on a remote cpu.
Tick related code in core.c is moved to tick.c

Vatika Harlalka (2):
nohz_full: Move tick related code to tick.c
nohz_full: Offload task_tick to remote housekeeping cpu

 include/linux/tick.h     |  2 ++
 kernel/sched/Makefile    |  2 +-
 kernel/sched/core.c      | 27 ---------------
 kernel/sched/tick.c      | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/time/tick-sched.c |  5 +++
 5 files changed, 93 insertions(+), 28 deletions(-)
 create mode 100644 kernel/sched/tick.c

Signature · 1 line

2.4.2

Vatika Harlalka

author

2015-08-13 09:25 UTC #1 lore raw

PATCH 1/2 nohz_full: Move tick related code to tick.c

Offloading task_tick off nohz_full cpus introduces tick related code in the scheduler code. It is therefore better to create a new file tick.c and move the existing tick related code there.

Signed-off-by Vatika Harlalka <[email protected]>

---
 kernel/sched/Makefile |  2 +-
 kernel/sched/core.c   | 27 ---------------------------
 kernel/sched/tick.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 28 deletions(-)
 create mode 100644 kernel/sched/tick.c

kernel/sched/Makefile +1 -1

index 46be870..3b31182 100644
@@ -11,7 +11,7 @@ ifneq ($(CONFIG_SCHED_OMIT_FRAME_POINTER),y)
 CFLAGS_core.o := $(PROFILING) -fno-omit-frame-pointer
 endif
 
-obj-y += core.o proc.o clock.o cputime.o
+obj-y += core.o proc.o clock.o cputime.o tick.o
 obj-y += idle_task.o fair.o rt.o deadline.o stop_task.o
 obj-y += wait.o completion.o idle.o
 obj-$(CONFIG_SMP) += cpupri.o cpudeadline.o

kernel/sched/core.c +0 ~~-27~~

index 57bd333..64beceb 100644
@@ -2481,33 +2481,6 @@ unsigned long long task_sched_runtime(struct task_struct *p)
 	return ns;
 }
 
-/*
- * This function gets called by the timer code, with HZ frequency.
- * We call it with interrupts disabled.
- */
-void scheduler_tick(void)
-{
-	int cpu = smp_processor_id();
-	struct rq *rq = cpu_rq(cpu);
-	struct task_struct *curr = rq->curr;
-
-	sched_clock_tick();
-
-	raw_spin_lock(&rq->lock);
-	update_rq_clock(rq);
-	curr->sched_class->task_tick(rq, curr, 0);
-	update_cpu_load_active(rq);
-	raw_spin_unlock(&rq->lock);
-
-	perf_event_task_tick();
-
-#ifdef CONFIG_SMP
-	rq->idle_balance = idle_cpu(cpu);
-	trigger_load_balance(rq);
-#endif
-	rq_last_tick_reset(rq);
-}
-
 #ifdef CONFIG_NO_HZ_FULL
 /**
  * scheduler_tick_max_deferment

kernel/sched/tick.c new file +43 -0

43 lines not on this page · raw message

Signature · 1 line

2.4.2

Peter Zijlstra

maintainer

2015-08-13 12:22 UTC #2 lore raw

In reply to Vatika Harlalka

Quoted 9 lines · Vatika Harlalka

This patchset is for offloading task_tick() to a remote housekeeping cpu. The larger aim is to stop ticks on nohz_full cpus. For this, extra work must be done by housekeeping cpus. So, task_tick is called from a delayed workqueue for nohz_full cpus and the work is requeued every second for those nohz_full cpus whose ticks are stopped while they are busy. In the rest of the cases it will lead to redundant accounting. To facilitate this, a new function tick_nohz_remote_tick_stopped is added to indicate whether ticks are stopped on a remote cpu.
Tick related code in core.c is moved to tick.c

*sigh* of course you didn't read what I've written on this topic..

Frederic Weisbecker

2015-08-13 12:44 UTC #3 lore raw

Quoted 11 lines · Peter Zijlstra

Quoted 9 lines · Vatika Harlalka
This patchset is for offloading task_tick() to a remote housekeeping cpu. The larger aim is to stop ticks on nohz_full cpus. For this, extra work must be done by housekeeping cpus. So, task_tick is called from a delayed workqueue for nohz_full cpus and the work is requeued every second for those nohz_full cpus whose ticks are stopped while they are busy. In the rest of the cases it will lead to redundant accounting. To facilitate this, a new function tick_nohz_remote_tick_stopped is added to indicate whether ticks are stopped on a remote cpu.
Tick related code in core.c is moved to tick.c

Quoted 1 line

*sigh* of course you didn't read what I've written on this topic..

What is it? Note Vatika wrote this after my suggestion, so if there is an issue, I'm likely the responsible :-) But I don't recall you opposed to this solution.

Thanks.

Peter Zijlstra

maintainer

2015-08-13 15:05 UTC #4 lore raw

Quoted 12 lines · Frederic Weisbecker

Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Vatika Harlalka
This patchset is for offloading task_tick() to a remote housekeeping cpu. The larger aim is to stop ticks on nohz_full cpus. For this, extra work must be done by housekeeping cpus. So, task_tick is called from a delayed workqueue for nohz_full cpus and the work is requeued every second for those nohz_full cpus whose ticks are stopped while they are busy. In the rest of the cases it will lead to redundant accounting. To facilitate this, a new function tick_nohz_remote_tick_stopped is added to indicate whether ticks are stopped on a remote cpu.
Tick related code in core.c is moved to tick.c

Quoted 4 lines

Quoted 1 line
*sigh* of course you didn't read what I've written on this topic..
What is it? Note Vatika wrote this after my suggestion, so if there is an issue, I'm likely the responsible :-) But I don't recall you opposed to this solution.

*sigh* of course you _could_ all use Google yourselves.

Re-read: https://patches.linaro.org/28290/

I see nothing like the stuff I asked for in here, on top it creates the stupid tick.c file.

Frederic Weisbecker

2015-08-13 15:36 UTC #5 lore raw

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Frederic Weisbecker
Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Vatika Harlalka
This patchset is for offloading task_tick() to a remote housekeeping cpu. The larger aim is to stop ticks on nohz_full cpus. For this, extra work must be done by housekeeping cpus. So, task_tick is called from a delayed workqueue for nohz_full cpus and the work is requeued every second for those nohz_full cpus whose ticks are stopped while they are busy. In the rest of the cases it will lead to redundant accounting. To facilitate this, a new function tick_nohz_remote_tick_stopped is added to indicate whether ticks are stopped on a remote cpu.
Tick related code in core.c is moved to tick.c

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
*sigh* of course you didn't read what I've written on this topic..
What is it? Note Vatika wrote this after my suggestion, so if there is an issue, I'm likely the responsible :-) But I don't recall you opposed to this solution.
*sigh* of course you _could_ all use Google yourselves.
Re-read: https://patches.linaro.org/28290/

Sorry, there were dozens of threads about this issue and I got a bit confused.

Quoted 3 lines

I see nothing like the stuff I asked for in here, on top it creates the stupid tick.c file.

Right. I initially thought that we should make sched_tick() just work with long delays. Then tglx suggested the offline idea but I lost track about our conversation.

But yeah making that scheduler_tick() working with long delays sound much better. Certainly much more work but that's a natural evolution after all. It should pay in longer term.

We can start with update_cpu_load_active() which only works with HZ frequency updates or nohz idle zero load decay. Now I think that stuff is only used for load balancing. I had hopes this thing could be removed. I think Alex Shin (IIRC) tried but the patchset didn't make it.

Thanks.

Vatika Harlalka

author

2015-08-13 19:22 UTC #6 lore raw

Hi Peter :)

I'm very sorry about this. I should've gone through what was written on the topic before
attempting to write this patch. I'll go through the link and work on what is discussed
to make a new relevant patch.

Thanks
Vatika

[email protected]

2015-08-20 13:22 UTC #7 lore raw

In reply to Frederic Weisbecker

Quoted 49 lines · Peter Zijlstra

Quoted 22 lines · Frederic Weisbecker
Quoted 3 lines · Peter Zijlstra
Quoted 2 lines · Vatika Harlalka
Quoted 1 line
This patchset is for offloading task_tick() to a remote
housekeeping
Quoted 1 line
Quoted 1 line
Quoted 1 line
cpu. The larger aim is to stop ticks on nohz_full cpus. For this,
extra
Quoted 1 line
Quoted 1 line
Quoted 1 line
work must be done by housekeeping cpus. So, task_tick is called
from a
Quoted 1 line
Quoted 1 line
Quoted 1 line
delayed workqueue for nohz_full cpus and the work is requeued
every second
Quoted 1 line
Quoted 1 line
Quoted 1 line
for those nohz_full cpus whose ticks are stopped while they are
busy. In
Quoted 1 line
Quoted 1 line
Quoted 1 line
the rest of the cases it will lead to redundant accounting. To
facilitate
Quoted 1 line
Quoted 1 line
Quoted 1 line
this, a new function tick_nohz_remote_tick_stopped is added to
indicate
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
whether ticks are stopped on a remote cpu.
Tick related code in core.c is moved to tick.c
*sigh* of course you didn't read what I've written on this topic..
Quoted 8 lines
Quoted 1 line
What is it? Note Vatika wrote this after my suggestion, so if there is
an issue,
Quoted 1 line
I'm likely the responsible :-) But I don't recall you opposed to this
solution.
*sigh* of course you _could_ all use Google yourselves.
Re-read: https://patches.linaro.org/28290/
Sorry, there were dozens of threads about this issue and I got a bit confused.
Quoted 3 lines
I see nothing like the stuff I asked for in here, on top it creates the stupid tick.c file.
Right. I initially thought that we should make sched_tick() just work with long delays.
Then tglx suggested the offline idea but I lost track about our conversation.
But yeah making that scheduler_tick() working with long delays sound much better. Certainly
much more work but that's a natural evolution after all. It should pay in longer term.

Quoted 7 lines

We can start with update_cpu_load_active() which only works with HZ frequency updates or
nohz idle zero load decay. Now I think that stuff is only used for load balancing. I had
hopes this thing could be removed. I think Alex Shin (IIRC) tried but the patchset didn't
make it.

I don't think Peter is talking about delays in updating the scheduler stats. Looking at the earlier discussion, it looks like we need to do periodic tick tasks only on demand on the nohz_full cpus. We will perhaps need to do the following(reiterating some points that Peter said earlier) :

1. One of the tasks that scheduler_tick() does is trigger_load_balance(). If we have to get rid of the residual tick, we need to move load balancing on nohz_full cpus into nohz_idle_balance(). In addition to load balancing on the idle cpus, this routine will load balance on the nohz_full cpus as well, when they are running single tasks.

This seems to be a good move because it will avoid pulling more tasks on to the nohz_full cpus, when they are running single tasks, unless needed.

2. In nohz_idle_load_balance(), there needs to be routines similar to update_idle_cpu_load() for nohz_full cpus so that the cpu loads are updated before triggering load balance on them. Lets call this update_nohz_full_cpu_load().
This should include update_curr() and update_cpu_load_active() for nohz_full cpus.

3. When scheduling stats are read, update_curr() and update_cpu_load_active() will
be called remotely.

The above three will ensure that work done during the scheduling tick is always
on demand on the nohz_full cpus; i.e. during load balancing and reading of stats.

Peter, Frederic can you let us know if this is the right approach ?

Regards
Preeti U Murthy

Quoted 3 lines

Thanks.

Frederic Weisbecker

2015-08-21 15:55 UTC #8 lore raw

Quoted 27 lines · On Thu, Aug 20, 2015 at 08:50:55AM -0400, [email protected] wrote:

Quoted 14 lines · Peter Zijlstra
Quoted 2 lines
I see nothing like the stuff I asked for in here, on top it creates the stupid tick.c file.
Right. I initially thought that we should make sched_tick() just work with long delays.
Then tglx suggested the offline idea but I lost track about our conversation.
But yeah making that scheduler_tick() working with long delays sound much better. Certainly
much more work but that's a natural evolution after all. It should pay in longer term.
Quoted 7 lines
We can start with update_cpu_load_active() which only works with HZ frequency updates or
nohz idle zero load decay. Now I think that stuff is only used for load balancing. I had
hopes this thing could be removed. I think Alex Shin (IIRC) tried but the patchset didn't
make it.
I don't think Peter is talking about delays in updating the scheduler stats. Looking at the earlier discussion, it looks like we need to do periodic tick tasks only on demand on the nohz_full cpus. We will perhaps need to do the following(reiterating some points that Peter said earlier) :

Quoted 8 lines

1. One of the tasks that scheduler_tick() does is trigger_load_balance(). If we have to get rid of the residual tick, we need to move load balancing on nohz_full cpus into nohz_idle_balance(). In addition to load balancing on the idle cpus, this routine will load balance on the nohz_full cpus as well, when they are running single tasks.
This seems to be a good move because it will avoid pulling more tasks on to the nohz_full cpus, when they are running single tasks, unless needed.

I suspect trigger_load_balance() is fine because now nohz full CPUs are part of cpu_isolated_map. I believe in that case they are on_null_domains(). If not then we should have a similar ignore check.

Quoted 7 lines

2. In nohz_idle_load_balance(), there needs to be routines similar to update_idle_cpu_load() for nohz_full cpus so that the cpu loads are updated before triggering load balance on them. Lets call this update_nohz_full_cpu_load().
This should include update_curr() and update_cpu_load_active() for nohz_full cpus.

nohz_idle_load_balance() shouldn't happen if trigger_load_balance() is ignored.

Quoted 4 lines

3. When scheduling stats are read, update_curr() and update_cpu_load_active() will
be called remotely.

Concerning update_cpu_load_active(), I wonder if rq->cpu_load[i > 0] are read for CPUs from cpu_isolated_map. I suspect that an isolated CPU shouldn't belong to any struct sched_group.

Now we can force disable SCHED_LB_BIAS otherwise.

Concerning update_curr(), we have yet to find all the places that do remote read. But again, perhaps an isolated cpu isn't subject to sched stats (involved in upodate_curr()) remotely read.

That's all stuff we need to verify.

A message that is not in the archive (sent off-list or not yet received): CACFGFQQ8mBzS-NhfNsp2nao2XpUksxu3t_Jse2zL2EQHLAK_Hg@mail.gmail.com

Vatika Harlalka

author

2015-09-11 17:05 UTC #10 lore raw

Quoted 3 lines

1. If LB_BIAS is false for nohz_full CPUs. This will help us figure out if rq->cpu_load
is read for them.

lb_bias feature is not disabled for full dynticks. rq->cpu_load[] is never used for them.
nohz_full cpus verify the condition on_null_domain(rq)

Quoted 6 lines

2. When a cpu reports scheduling stats for all cpus such as sum_exec_runtime,
does it consider cpu_isolated_map (most likely it will) ? If it does, we need to be
able to remotely update those statistics before reporting them. i.e. call update_curr(rq) on behalf of the nohz_full cpus.

Yeah, cpu stats are reported and updated for all cpus.

The next step would be to offload these on other cpus now for nohz_full cpus running a single task now?

Thanks
Vatika

Quoted 5 lines

--
This is Preeti, signing off.