[PATCH v2 0/2] sched/cputime: sum_exec_runtime fixes for 32-bit cpus

[PATCH v2 0/2] sched/cputime: sum_exec_runtime fixes for 32-bit cpus

[Stanislaw Gruszka]

Optimize read_sum_exec_runtime() on 32-bit cpus by taking only p->pi_lock
and use above function to protect reads of 64bit sum_exec_runtime on
other places on 32 bit architectures.

Stanislaw Gruszka (2):
  sched/cputime: Use only pi_lock to protect sum_exec_runtime read
  sched/cputime: Protect other sum_exec_runtime reads on 32 bit cpus

 fs/proc/base.c                 |  2 +-
 include/linux/sched.h          | 18 ++++++++++++++++++
 kernel/delayacct.c             |  2 +-
 kernel/exit.c                  |  2 +-
 kernel/sched/cputime.c         | 22 +---------------------
 kernel/time/posix-cpu-timers.c |  4 ++--
 6 files changed, 24 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read

[Stanislaw Gruszka]

Currently we protect 64bit sum_exec_runtime read on 32bit cpus using
task_rq_lock() which internally takes t->pi_lock and rq->lock. Taking
rq->lock is not needed in this case.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 kernel/sched/cputime.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index b93c72d..5535774 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -315,12 +315,11 @@ static inline u64 read_sum_exec_runtime(struct task_struct *t)
 static u64 read_sum_exec_runtime(struct task_struct *t)
 {
 	u64 ns;
-	struct rq_flags rf;
-	struct rq *rq;
+	unsigned long flags;
 
-	rq = task_rq_lock(t, &rf);
+	raw_spin_lock_irqsave(&t->pi_lock, flags);
 	ns = t->se.sum_exec_runtime;
-	task_rq_unlock(rq, t, &rf);
+	raw_spin_unlock_irqrestore(&t->pi_lock, flags);
 
 	return ns;
 }
-- 
1.8.3.1

[PATCH v2 2/2] sched/cputime: Protect other sum_exec_runtime reads on 32 bit cpus

[Stanislaw Gruszka]

We protect 64bit sum_exec_runtime variable against inconsistency on
32 bit architectures when reading it on thread_group_cputime(), but we
also read it on other places not protected.

Patch changes that, except file kernel/sched/debug.c , where we also
read some other u64 variables (vruntime, exec_start) without protection.
But since this is for debug purpose and inconsistency is very unlike,
we can ignore that.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 fs/proc/base.c                 |  2 +-
 include/linux/sched.h          | 19 +++++++++++++++++++
 kernel/delayacct.c             |  2 +-
 kernel/exit.c                  |  2 +-
 kernel/sched/cputime.c         | 21 +--------------------
 kernel/time/posix-cpu-timers.c |  4 ++--
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index e9ff186..8118d97 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -505,7 +505,7 @@ static int proc_pid_schedstat(struct seq_file *m, struct pid_namespace *ns,
 		seq_printf(m, "0 0 0\n");
 	else
 		seq_printf(m, "%llu %llu %lu\n",
-		   (unsigned long long)task->se.sum_exec_runtime,
+		   (unsigned long long)read_sum_exec_runtime(task),
 		   (unsigned long long)task->sched_info.run_delay,
 		   task->sched_info.pcount);
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index eb64fcd..267fc98 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2508,6 +2508,25 @@ static inline void disable_sched_clock_irqtime(void) {}
 extern unsigned long long
 task_sched_runtime(struct task_struct *task);
 
+#ifdef CONFIG_64BIT
+static inline u64 read_sum_exec_runtime(struct task_struct *t)
+{
+	return t->se.sum_exec_runtime;
+}
+#else
+static inline u64 read_sum_exec_runtime(struct task_struct *t)
+{
+	u64 ns;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&t->pi_lock, flags);
+	ns = t->se.sum_exec_runtime;
+	raw_spin_unlock_irqrestore(&t->pi_lock, flags);
+
+	return ns;
+}
+#endif
+
 /* sched_exec is called by processes performing an exec */
 #ifdef CONFIG_SMP
 extern void sched_exec(void);
diff --git a/kernel/delayacct.c b/kernel/delayacct.c
index 435c14a..023b2ca 100644
--- a/kernel/delayacct.c
+++ b/kernel/delayacct.c
@@ -104,7 +104,7 @@ int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
 	 */
 	t1 = tsk->sched_info.pcount;
 	t2 = tsk->sched_info.run_delay;
-	t3 = tsk->se.sum_exec_runtime;
+	t3 = read_sum_exec_runtime(tsk);
 
 	d->cpu_count += t1;
 
diff --git a/kernel/exit.c b/kernel/exit.c
index 2f974ae..a46f96f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -134,7 +134,7 @@ static void __exit_signal(struct task_struct *tsk)
 	sig->inblock += task_io_get_inblock(tsk);
 	sig->oublock += task_io_get_oublock(tsk);
 	task_io_accounting_add(&sig->ioac, &tsk->ioac);
-	sig->sum_sched_runtime += tsk->se.sum_exec_runtime;
+	sig->sum_sched_runtime += read_sum_exec_runtime(tsk);
 	sig->nr_threads--;
 	__unhash_process(tsk, group_dead);
 	write_sequnlock(&sig->stats_lock);
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 5535774..4d080f2 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -306,25 +306,6 @@ static inline cputime_t account_other_time(cputime_t max)
 	return accounted;
 }
 
-#ifdef CONFIG_64BIT
-static inline u64 read_sum_exec_runtime(struct task_struct *t)
-{
-	return t->se.sum_exec_runtime;
-}
-#else
-static u64 read_sum_exec_runtime(struct task_struct *t)
-{
-	u64 ns;
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&t->pi_lock, flags);
-	ns = t->se.sum_exec_runtime;
-	raw_spin_unlock_irqrestore(&t->pi_lock, flags);
-
-	return ns;
-}
-#endif
-
 /*
  * Accumulate raw cputime values of dead tasks (sig->[us]time) and live
  * tasks (sum on group iteration) belonging to @tsk's group.
@@ -701,7 +682,7 @@ out:
 void task_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime_t *st)
 {
 	struct task_cputime cputime = {
-		.sum_exec_runtime = p->se.sum_exec_runtime,
+		.sum_exec_runtime = read_sum_exec_runtime(p),
 	};
 
 	task_cputime(p, &cputime.utime, &cputime.stime);
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 39008d7..a2d753b 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -848,7 +848,7 @@ static void check_thread_timers(struct task_struct *tsk,
 	tsk_expires->virt_exp = expires_to_cputime(expires);
 
 	tsk_expires->sched_exp = check_timers_list(++timers, firing,
-						   tsk->se.sum_exec_runtime);
+						   read_sum_exec_runtime(tsk));
 
 	/*
 	 * Check for the special case thread timers.
@@ -1115,7 +1115,7 @@ static inline int fastpath_timer_check(struct task_struct *tsk)
 		struct task_cputime task_sample;
 
 		task_cputime(tsk, &task_sample.utime, &task_sample.stime);
-		task_sample.sum_exec_runtime = tsk->se.sum_exec_runtime;
+		task_sample.sum_exec_runtime = read_sum_exec_runtime(tsk);
 		if (task_cputime_expired(&task_sample, &tsk->cputime_expires))
 			return 1;
 	}
-- 
1.8.3.1

Re: [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read

[Stanislaw Gruszka]

On Mon, Sep 05, 2016 at 11:13:01AM +0200, Stanislaw Gruszka wrote:
> Currently we protect 64bit sum_exec_runtime read on 32bit cpus using
> task_rq_lock() which internally takes t->pi_lock and rq->lock. Taking
> rq->lock is not needed in this case.

I looked more at kernel/sched/ code and now I'm not sure about this.
I assumed that update_curr() is called with rq->curr->pi_lock, but
looks like it can be called with some other task->pi_lock not
necessary the rq->curr, hence looks that we need rq->lock to assure
protection

Stanislaw

> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  kernel/sched/cputime.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index b93c72d..5535774 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -315,12 +315,11 @@ static inline u64 read_sum_exec_runtime(struct task_struct *t)
>  static u64 read_sum_exec_runtime(struct task_struct *t)
>  {
>  	u64 ns;
> -	struct rq_flags rf;
> -	struct rq *rq;
> +	unsigned long flags;
>  
> -	rq = task_rq_lock(t, &rf);
> +	raw_spin_lock_irqsave(&t->pi_lock, flags);
>  	ns = t->se.sum_exec_runtime;
> -	task_rq_unlock(rq, t, &rf);
> +	raw_spin_unlock_irqrestore(&t->pi_lock, flags);
>  
>  	return ns;
>  }
> -- 
> 1.8.3.1
> 

Re: [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 04:16:57PM +0200, Stanislaw Gruszka wrote:
> On Mon, Sep 05, 2016 at 11:13:01AM +0200, Stanislaw Gruszka wrote:
> > Currently we protect 64bit sum_exec_runtime read on 32bit cpus using
> > task_rq_lock() which internally takes t->pi_lock and rq->lock. Taking
> > rq->lock is not needed in this case.
> 
> I looked more at kernel/sched/ code and now I'm not sure about this.
> I assumed that update_curr() is called with rq->curr->pi_lock, but
> looks like it can be called with some other task->pi_lock not
> necessary the rq->curr, hence looks that we need rq->lock to assure
> protection

Correct, rq->lock needs be held for this.

update_curr() is called from places like the tick and schedule(),
neither of which hold pi_lock.