* [PATCH v2 0/2] sched/cputime: sum_exec_runtime fixes for 32-bit cpus
@ 2016-09-05 9:13 Stanislaw Gruszka
2016-09-05 9:13 ` [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read Stanislaw Gruszka
2016-09-05 9:13 ` [PATCH v2 2/2] sched/cputime: Protect other sum_exec_runtime reads on 32 bit cpus Stanislaw Gruszka
0 siblings, 2 replies; 5+ messages in thread
From: Stanislaw Gruszka @ 2016-09-05 9:13 UTC (permalink / raw)
To: linux-kernel
Cc: Giovanni Gherdovich, Linus Torvalds, Mel Gorman, Mike Galbraith,
Paolo Bonzini, Peter Zijlstra, Rik van Riel, Thomas Gleixner,
Wanpeng Li, Ingo Molnar, 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read
2016-09-05 9:13 [PATCH v2 0/2] sched/cputime: sum_exec_runtime fixes for 32-bit cpus Stanislaw Gruszka
@ 2016-09-05 9:13 ` Stanislaw Gruszka
2016-09-05 14:16 ` Stanislaw Gruszka
2016-09-05 9:13 ` [PATCH v2 2/2] sched/cputime: Protect other sum_exec_runtime reads on 32 bit cpus Stanislaw Gruszka
1 sibling, 1 reply; 5+ messages in thread
From: Stanislaw Gruszka @ 2016-09-05 9:13 UTC (permalink / raw)
To: linux-kernel
Cc: Giovanni Gherdovich, Linus Torvalds, Mel Gorman, Mike Galbraith,
Paolo Bonzini, Peter Zijlstra, Rik van Riel, Thomas Gleixner,
Wanpeng Li, Ingo Molnar, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] sched/cputime: Protect other sum_exec_runtime reads on 32 bit cpus
2016-09-05 9:13 [PATCH v2 0/2] sched/cputime: sum_exec_runtime fixes for 32-bit cpus Stanislaw Gruszka
2016-09-05 9:13 ` [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read Stanislaw Gruszka
@ 2016-09-05 9:13 ` Stanislaw Gruszka
1 sibling, 0 replies; 5+ messages in thread
From: Stanislaw Gruszka @ 2016-09-05 9:13 UTC (permalink / raw)
To: linux-kernel
Cc: Giovanni Gherdovich, Linus Torvalds, Mel Gorman, Mike Galbraith,
Paolo Bonzini, Peter Zijlstra, Rik van Riel, Thomas Gleixner,
Wanpeng Li, Ingo Molnar, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read
2016-09-05 9:13 ` [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read Stanislaw Gruszka
@ 2016-09-05 14:16 ` Stanislaw Gruszka
2016-09-05 19:16 ` Peter Zijlstra
0 siblings, 1 reply; 5+ messages in thread
From: Stanislaw Gruszka @ 2016-09-05 14:16 UTC (permalink / raw)
To: linux-kernel
Cc: Giovanni Gherdovich, Linus Torvalds, Mel Gorman, Mike Galbraith,
Paolo Bonzini, Peter Zijlstra, Rik van Riel, Thomas Gleixner,
Wanpeng Li, Ingo Molnar
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read
2016-09-05 14:16 ` Stanislaw Gruszka
@ 2016-09-05 19:16 ` Peter Zijlstra
0 siblings, 0 replies; 5+ messages in thread
From: Peter Zijlstra @ 2016-09-05 19:16 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: linux-kernel, Giovanni Gherdovich, Linus Torvalds, Mel Gorman,
Mike Galbraith, Paolo Bonzini, Rik van Riel, Thomas Gleixner,
Wanpeng Li, Ingo Molnar
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-05 19:16 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-05 9:13 [PATCH v2 0/2] sched/cputime: sum_exec_runtime fixes for 32-bit cpus Stanislaw Gruszka
2016-09-05 9:13 ` [PATCH v2 1/2] sched/cputime: Use only pi_lock to protect sum_exec_runtime read Stanislaw Gruszka
2016-09-05 14:16 ` Stanislaw Gruszka
2016-09-05 19:16 ` Peter Zijlstra
2016-09-05 9:13 ` [PATCH v2 2/2] sched/cputime: Protect other sum_exec_runtime reads on 32 bit cpus Stanislaw Gruszka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).