| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20130215140429.GA4322@enea.se>
Date: Fri, 15 Feb 2013 15:04:29 +0100
From: Mats Liljegren <mats.liljegren@...a.com>
To: Kevin Hilman <khilman@...aro.org>
CC: Frederic Weisbecker <fweisbec@...il.com>,
Ingo Molnar <mingo@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Li Zhong <zhong@...ux.vnet.ibm.com>,
Namhyung Kim <namhyung.kim@....com>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Paul Gortmaker <paul.gortmaker@...driver.com>,
Peter Zijlstra <peterz@...radead.org>,
Steven Rostedt <rostedt@...dmis.org>,
Thomas Gleixner <tglx@...utronix.de>,
Sedat Dilek <sedat.dilek@...il.com>,
Gleb Natapov <gleb@...hat.com>,
Marcelo Tosatti <mtosatti@...hat.com>,
Tony Luck <tony.luck@...el.com>,
Fenghua Yu <fenghua.yu@...el.com>
Subject: Re: [GIT PULL] cputime: Full dynticks task/cputime accounting v7
Kevin Hilman wrote:
> Frederic Weisbecker <fweisbec@...il.com> writes:
>
> > Ingo,
> >
> > Please pull the new full dynticks cputime accounting code that
> > can be found at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
> > tags/full-dynticks-cputime-for-mingo
> >
> > My last concern is the dependency on CONFIG_64BIT. We rely on cputime_t
> > being u64 for reasonable nanosec granularity implementation. And therefore
> > we need a single instruction fetch to read kernel cpustat for atomicity
> > requirement against concurrent incrementation, which only 64 bit archs
> > can provide.
>
> Actually, moderately recent 32-bit ARMs can do atomic 64-bit load/stores
> too.
>
> Also, is it just kernel_cpustat increments that need protection? or do
> the various reads of the task_struct's cputime fields also need
> protection (hmm, thinking twice, maybe those are already sufficiently
> protected by the vtime_seqlock?)
>
> > It's probably no big deal to solve this issue. What we need is simply some
> > atomic accessors.
>
> What about using the atomic64_* accessors? Those would just use the
> native loads/stores on arches that have them, otherwise
> CONFIG_GENERIC_ATOMIC64 provides some fallbacks.
>
> To give it a try, below is a quick patch to convert kernel_cpustat to
> atomic64. I only got as far as compile testing and basic boot testing
> on a 32-bit ARM platform, but let me know if this is the right
> direction.
>
> > There is just no emergency though as this new option depends on the context
> > tracking subsystem that only x86-64 (and soon ppc64) implements yet. And
> > this set is complex enough already. I think we can deal with that later.
>
> I've started working on the ARM version of the context_tracker, so
> "later" is coming quickly and I will do what I can to help this along.
>
> Kevin
>
>
>
> >From 9881fc4b86a63244018a4bdb5a383f3b862a0b8b Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@...aro.org>
> Date: Thu, 14 Feb 2013 17:46:08 -0800
> Subject: [PATCH] kernel_cpustat: convert to atomic 64-bit accessors
>
> WIP: only compile tested and basic boot tested on 32-bit ARM platform
> ---
> fs/proc/stat.c | 36 ++++++++++++++++++------------------
> fs/proc/uptime.c | 2 +-
> include/linux/kernel_stat.h | 2 +-
> kernel/sched/cputime.c | 22 +++++++++++-----------
> 4 files changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/fs/proc/stat.c b/fs/proc/stat.c
> index e296572..2a4c723 100644
> --- a/fs/proc/stat.c
> +++ b/fs/proc/stat.c
> @@ -52,7 +52,7 @@ static u64 get_idle_time(int cpu)
>
> if (idle_time == -1ULL)
> /* !NO_HZ or cpu offline so we can rely on cpustat.idle */
> - idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
> + idle = atomic64_read(&kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE]);
> else
> idle = usecs_to_cputime64(idle_time);
>
> @@ -68,7 +68,7 @@ static u64 get_iowait_time(int cpu)
>
> if (iowait_time == -1ULL)
> /* !NO_HZ or cpu offline so we can rely on cpustat.iowait */
> - iowait = kcpustat_cpu(cpu).cpustat[CPUTIME_IOWAIT];
> + iowait = atomic64_read(&kcpustat_cpu(cpu).cpustat[CPUTIME_IOWAIT]);
> else
> iowait = usecs_to_cputime64(iowait_time);
>
> @@ -95,16 +95,16 @@ static int show_stat(struct seq_file *p, void *v)
> jif = boottime.tv_sec;
>
> for_each_possible_cpu(i) {
> - user += kcpustat_cpu(i).cpustat[CPUTIME_USER];
> - nice += kcpustat_cpu(i).cpustat[CPUTIME_NICE];
> - system += kcpustat_cpu(i).cpustat[CPUTIME_SYSTEM];
> + user += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_USER]);
> + nice += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_NICE]);
> + system += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_SYSTEM]);
> idle += get_idle_time(i);
> iowait += get_iowait_time(i);
> - irq += kcpustat_cpu(i).cpustat[CPUTIME_IRQ];
> - softirq += kcpustat_cpu(i).cpustat[CPUTIME_SOFTIRQ];
> - steal += kcpustat_cpu(i).cpustat[CPUTIME_STEAL];
> - guest += kcpustat_cpu(i).cpustat[CPUTIME_GUEST];
> - guest_nice += kcpustat_cpu(i).cpustat[CPUTIME_GUEST_NICE];
> + irq += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_IRQ]);
> + softirq += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_SOFTIRQ]);
> + steal += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_STEAL]);
> + guest += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_GUEST]);
> + guest_nice += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_GUEST_NICE]);
> sum += kstat_cpu_irqs_sum(i);
> sum += arch_irq_stat_cpu(i);
>
> @@ -132,16 +132,16 @@ static int show_stat(struct seq_file *p, void *v)
>
> for_each_online_cpu(i) {
> /* Copy values here to work around gcc-2.95.3, gcc-2.96 */
> - user = kcpustat_cpu(i).cpustat[CPUTIME_USER];
> - nice = kcpustat_cpu(i).cpustat[CPUTIME_NICE];
> - system = kcpustat_cpu(i).cpustat[CPUTIME_SYSTEM];
> + user = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_USER]);
> + nice = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_NICE]);
> + system = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_SYSTEM]);
> idle = get_idle_time(i);
> iowait = get_iowait_time(i);
> - irq = kcpustat_cpu(i).cpustat[CPUTIME_IRQ];
> - softirq = kcpustat_cpu(i).cpustat[CPUTIME_SOFTIRQ];
> - steal = kcpustat_cpu(i).cpustat[CPUTIME_STEAL];
> - guest = kcpustat_cpu(i).cpustat[CPUTIME_GUEST];
> - guest_nice = kcpustat_cpu(i).cpustat[CPUTIME_GUEST_NICE];
> + irq = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_IRQ]);
> + softirq = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_SOFTIRQ]);
> + steal = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_STEAL]);
> + guest = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_GUEST]);
> + guest_nice = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_GUEST_NICE]);
> seq_printf(p, "cpu%d", i);
> seq_put_decimal_ull(p, ' ', cputime64_to_clock_t(user));
> seq_put_decimal_ull(p, ' ', cputime64_to_clock_t(nice));
> diff --git a/fs/proc/uptime.c b/fs/proc/uptime.c
> index 9610ac7..10c0f6e 100644
> --- a/fs/proc/uptime.c
> +++ b/fs/proc/uptime.c
> @@ -18,7 +18,7 @@ static int uptime_proc_show(struct seq_file *m, void *v)
>
> idletime = 0;
> for_each_possible_cpu(i)
> - idletime += (__force u64) kcpustat_cpu(i).cpustat[CPUTIME_IDLE];
> + idletime += atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_IDLE]);
>
> do_posix_clock_monotonic_gettime(&uptime);
> monotonic_to_bootbased(&uptime);
> diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
> index ed5f6ed..45b9f71 100644
> --- a/include/linux/kernel_stat.h
> +++ b/include/linux/kernel_stat.h
> @@ -32,7 +32,7 @@ enum cpu_usage_stat {
> };
>
> struct kernel_cpustat {
> - u64 cpustat[NR_STATS];
> + atomic64_t cpustat[NR_STATS];
> };
>
> struct kernel_stat {
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index ccff275..c1ac2b2 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -125,7 +125,7 @@ static inline void task_group_account_field(struct task_struct *p, int index,
> * is the only cgroup, then nothing else should be necessary.
> *
> */
> - __get_cpu_var(kernel_cpustat).cpustat[index] += tmp;
> + atomic64_add(tmp, &__get_cpu_var(kernel_cpustat).cpustat[index]);
>
> #ifdef CONFIG_CGROUP_CPUACCT
> if (unlikely(!cpuacct_subsys.active))
> @@ -176,7 +176,7 @@ void account_user_time(struct task_struct *p, cputime_t cputime,
> static void account_guest_time(struct task_struct *p, cputime_t cputime,
> cputime_t cputime_scaled)
> {
> - u64 *cpustat = kcpustat_this_cpu->cpustat;
> + atomic64_t *cpustat = kcpustat_this_cpu->cpustat;
>
> /* Add guest time to process. */
> p->utime += cputime;
> @@ -186,11 +186,11 @@ static void account_guest_time(struct task_struct *p, cputime_t cputime,
>
> /* Add guest time to cpustat. */
> if (TASK_NICE(p) > 0) {
> - cpustat[CPUTIME_NICE] += (__force u64) cputime;
> - cpustat[CPUTIME_GUEST_NICE] += (__force u64) cputime;
> + atomic64_add((__force u64) cputime, &cpustat[CPUTIME_NICE]);
> + atomic64_add((__force u64) cputime, &cpustat[CPUTIME_GUEST_NICE]);
> } else {
> - cpustat[CPUTIME_USER] += (__force u64) cputime;
> - cpustat[CPUTIME_GUEST] += (__force u64) cputime;
> + atomic64_add((__force u64) cputime, &cpustat[CPUTIME_USER]);
> + atomic64_add((__force u64) cputime, &cpustat[CPUTIME_GUEST]);
> }
> }
>
> @@ -250,9 +250,9 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
> */
> void account_steal_time(cputime_t cputime)
> {
> - u64 *cpustat = kcpustat_this_cpu->cpustat;
> + atomic64_t *cpustat = kcpustat_this_cpu->cpustat;
>
> - cpustat[CPUTIME_STEAL] += (__force u64) cputime;
> + atomic64_add((__force u64) cputime, &cpustat[CPUTIME_STEAL]);
> }
>
> /*
> @@ -261,13 +261,13 @@ void account_steal_time(cputime_t cputime)
> */
> void account_idle_time(cputime_t cputime)
> {
> - u64 *cpustat = kcpustat_this_cpu->cpustat;
> + atomic64_t *cpustat = kcpustat_this_cpu->cpustat;
> struct rq *rq = this_rq();
>
> if (atomic_read(&rq->nr_iowait) > 0)
> - cpustat[CPUTIME_IOWAIT] += (__force u64) cputime;
> + atomic64_add((__force u64) cputime, &cpustat[CPUTIME_IOWAIT]);
> else
> - cpustat[CPUTIME_IDLE] += (__force u64) cputime;
> + atomic64_add((__force u64) cputime, &cpustat[CPUTIME_IDLE]);
> }
>
> static __always_inline bool steal_account_process_tick(void)
> --
> 1.8.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
I had to do some changes to get it to compile. You can check my patch below
and see whether I got it all wrong...
/Mats
>From e3da56bdb36a3efb5a9869ded20f346039f381f2 Mon Sep 17 00:00:00 2001
From: Mats Liljegren <mats.liljegren@...a.com>
Date: Fri, 15 Feb 2013 13:46:29 +0100
Subject: [PATCH] Added atomic operators to make it compile
---
kernel/sched/core.c | 10 +++++-----
kernel/sched/cputime.c | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2fad439..5415e85 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8168,8 +8168,8 @@ static int cpuacct_stats_show(struct cgroup *cgrp, struct cftype *cft,
for_each_online_cpu(cpu) {
struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat, cpu);
- val += kcpustat->cpustat[CPUTIME_USER];
- val += kcpustat->cpustat[CPUTIME_NICE];
+ val += atomic64_read(&kcpustat->cpustat[CPUTIME_USER]);
+ val += atomic64_read(&kcpustat->cpustat[CPUTIME_NICE]);
}
val = cputime64_to_clock_t(val);
cb->fill(cb, cpuacct_stat_desc[CPUACCT_STAT_USER], val);
@@ -8177,9 +8177,9 @@ static int cpuacct_stats_show(struct cgroup *cgrp, struct cftype *cft,
val = 0;
for_each_online_cpu(cpu) {
struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat, cpu);
- val += kcpustat->cpustat[CPUTIME_SYSTEM];
- val += kcpustat->cpustat[CPUTIME_IRQ];
- val += kcpustat->cpustat[CPUTIME_SOFTIRQ];
+ val += atomic64_read(&kcpustat->cpustat[CPUTIME_SYSTEM]);
+ val += atomic64_read(&kcpustat->cpustat[CPUTIME_IRQ]);
+ val += atomic64_read(&kcpustat->cpustat[CPUTIME_SOFTIRQ]);
}
val = cputime64_to_clock_t(val);
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 50de5bf..2bb998b 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -135,7 +135,7 @@ static inline void task_group_account_field(struct task_struct *p, int index,
ca = task_ca(p);
while (ca && (ca != &root_cpuacct)) {
kcpustat = this_cpu_ptr(ca->cpustat);
- kcpustat->cpustat[index] += tmp;
+ atomic64_add(tmp, &kcpustat->cpustat[index]);
ca = parent_ca(ca);
}
rcu_read_unlock();
--
1.7.5.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists