[PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[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

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

-- 
2.4.2

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

[Vatika Harlalka]

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 <vatikaharlalka@gmail.com>
---
 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

diff --git a/kernel/sched/Makefile b/kernel/sched/Makefile
index 46be870..3b31182 100644
--- a/kernel/sched/Makefile
+++ b/kernel/sched/Makefile
@@ -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
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 57bd333..64beceb 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -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
diff --git a/kernel/sched/tick.c b/kernel/sched/tick.c
new file mode 100644
index 0000000..3bc32c7
--- /dev/null
+++ b/kernel/sched/tick.c
@@ -0,0 +1,43 @@
+#include <linux/smp.h>
+#include <linux/cpumask.h>
+#include <linux/tick.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/percpu.h>
+#include <linux/perf_event.h>
+#include <linux/workqueue.h>
+#include <linux/jiffies.h>
+#include "sched.h"
+
+/*
+ * 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;
+
+	raw_spin_lock(&rq->lock);
+	curr = rq->curr;
+	raw_spin_unlock(&rq->lock);
+
+	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);
+}
+
-- 
2.4.2

Re: [PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[Peter Zijlstra]

On Thu, Aug 13, 2015 at 02:55:36PM +0530, Vatika Harlalka wrote:
> 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..

Re: [PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[Frederic Weisbecker]

On Thu, Aug 13, 2015 at 02:22:23PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 13, 2015 at 02:55:36PM +0530, Vatika Harlalka wrote:
> > 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..

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.

Re: [PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[Peter Zijlstra]

On Thu, Aug 13, 2015 at 02:44:02PM +0200, Frederic Weisbecker wrote:
> On Thu, Aug 13, 2015 at 02:22:23PM +0200, Peter Zijlstra wrote:
> > On Thu, Aug 13, 2015 at 02:55:36PM +0530, Vatika Harlalka wrote:
> > > 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..
> 
> 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.

Re: [PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[Frederic Weisbecker]

On Thu, Aug 13, 2015 at 05:05:45PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 13, 2015 at 02:44:02PM +0200, Frederic Weisbecker wrote:
> > On Thu, Aug 13, 2015 at 02:22:23PM +0200, Peter Zijlstra wrote:
> > > On Thu, Aug 13, 2015 at 02:55:36PM +0530, Vatika Harlalka wrote:
> > > > 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..
> > 
> > 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.

> 
> 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.

Re: [PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[Vatika Harlalka]

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

Re: [PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[preetium]

> On Thu, Aug 13, 2015 at 05:05:45PM +0200, Peter Zijlstra wrote:
>> On Thu, Aug 13, 2015 at 02:44:02PM +0200, Frederic Weisbecker wrote:
>> > On Thu, Aug 13, 2015 at 02:22:23PM +0200, Peter Zijlstra wrote:
>> > > On Thu, Aug 13, 2015 at 02:55:36PM +0530, Vatika Harlalka wrote:
>> > > > 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..
>> >
>> > 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.
>
>>
>> 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.

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

>
> Thanks.
>

Re: [PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[Frederic Weisbecker]

On Thu, Aug 20, 2015 at 08:50:55AM -0400, preetium@andrew.cmu.edu wrote:
> > On Thu, Aug 13, 2015 at 05:05:45PM +0200, Peter Zijlstra wrote:
> >> 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.
> 
> 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.

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.

> 
> 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.

> 
> 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.

Re: [PATCH 0/2] nohz_full: Offload task_tick to remote housekeeping cpus for nohz_full cpus

[Vatika Harlalka]

> 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)

> 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

>
>
>
> --
> This is Preeti, signing off.