linux-kernel - Re: [RFC/PATCH 2/5] kernel_cpustat: convert to atomic 64-bit accessors

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFTL4hw1=mKtUadL0WDrydtspOehKuW7jhhzr65vXR2P2zdTWw@mail.gmail.com>
Date:	Thu, 21 Feb 2013 22:53:07 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Kevin Hilman <khilman@...aro.org>
Cc:	Mats Liljegren <mats.liljegren@...a.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linaro-dev@...ts.linaro.org
Subject: Re: [RFC/PATCH 2/5] kernel_cpustat: convert to atomic 64-bit accessors

2013/2/21 Kevin Hilman <khilman@...aro.org>:
> Subject: [PATCH 2/5] kernel_cpustat: convert to atomic 64-bit accessors
>
> Use the atomic64_* accessors for all the kernel_cpustat fields to
> ensure atomic access on non-64 bit platforms.
>
> Thanks to Mats Liljegren for CGROUP_CPUACCT related fixes.
>
> Cc: Mats Liljegren <mats.liljegren@...a.com>
> Signed-off-by: Kevin Hilman <khilman@...aro.org>

Funny stuff, I thought struct kernel_cpustat was made of cputime_t
field. Actually it's u64. So the issue is independant from the new
full dynticks cputime accounting. It was already broken before.

But yeah that's not the point, we still want to fix this anyway. But
let's just treat this patch as independant.

> ---
>  arch/s390/appldata/appldata_os.c   | 41 +++++++++++++++++++++++---------------
>  drivers/cpufreq/cpufreq_governor.c | 18 ++++++++---------
>  drivers/cpufreq/cpufreq_ondemand.c |  2 +-
>  drivers/macintosh/rack-meter.c     |  6 +++---
>  fs/proc/stat.c                     | 40 ++++++++++++++++++-------------------
>  fs/proc/uptime.c                   |  2 +-
>  include/linux/kernel_stat.h        |  2 +-
>  kernel/sched/core.c                | 10 +++++-----
>  kernel/sched/cputime.c             | 38 +++++++++++++++++------------------
>  9 files changed, 84 insertions(+), 75 deletions(-)
>
> diff --git a/arch/s390/appldata/appldata_os.c b/arch/s390/appldata/appldata_os.c
> index 87521ba..008b180 100644
> --- a/arch/s390/appldata/appldata_os.c
> +++ b/arch/s390/appldata/appldata_os.c
> @@ -99,6 +99,7 @@ static void appldata_get_os_data(void *data)
>         int i, j, rc;
>         struct appldata_os_data *os_data;
>         unsigned int new_size;
> +       u64 val;
>
>         os_data = data;
>         os_data->sync_count_1++;
> @@ -112,22 +113,30 @@ static void appldata_get_os_data(void *data)
>
>         j = 0;
>         for_each_online_cpu(i) {
> -               os_data->os_cpu[j].per_cpu_user =
> -                       cputime_to_jiffies(kcpustat_cpu(i).cpustat[CPUTIME_USER]);
> -               os_data->os_cpu[j].per_cpu_nice =
> -                       cputime_to_jiffies(kcpustat_cpu(i).cpustat[CPUTIME_NICE]);
> -               os_data->os_cpu[j].per_cpu_system =
> -                       cputime_to_jiffies(kcpustat_cpu(i).cpustat[CPUTIME_SYSTEM]);
> -               os_data->os_cpu[j].per_cpu_idle =
> -                       cputime_to_jiffies(kcpustat_cpu(i).cpustat[CPUTIME_IDLE]);
> -               os_data->os_cpu[j].per_cpu_irq =
> -                       cputime_to_jiffies(kcpustat_cpu(i).cpustat[CPUTIME_IRQ]);
> -               os_data->os_cpu[j].per_cpu_softirq =
> -                       cputime_to_jiffies(kcpustat_cpu(i).cpustat[CPUTIME_SOFTIRQ]);
> -               os_data->os_cpu[j].per_cpu_iowait =
> -                       cputime_to_jiffies(kcpustat_cpu(i).cpustat[CPUTIME_IOWAIT]);
> -               os_data->os_cpu[j].per_cpu_steal =
> -                       cputime_to_jiffies(kcpustat_cpu(i).cpustat[CPUTIME_STEAL]);
> +               val = atomic64_read(&kcpustat_cpu(i).cpustat[CPUTIME_USER]);

So I see this repeated pattern everywhere. How about converting that to:
           kcpustat_cpu_get(i, CPUTIME_USER)

and use that accessor in all other places. That's much more readable
and then we can later modify the accessing code in one go.

We should perhaps even use atomic_64 in 32 bits and u64 in 64 bits.

[...]
> @@ -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]);

That too should be kcpustat_this_cpu_set(), or kcpustat_this_cpu_add()
FWIW. But we probably don't need the overhead of atomic_add() that
does a LOCK.
atomic_set(var, atomic_read(var) + delta) would be better. All we need
is that low/high parts of the 64 bits values are stored and read
without messing up altogether.

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