* Zhao Lei <[email protected]> wrote:
> Merge code for each cpustat(system/user) into a loop,
> to avoid clone of code blocks.
> Only a little cleanup.
>
> Signed-off-by: Zhao Lei <[email protected]>
> ---
> kernel/sched/cpuacct.c | 29 ++++++++++++++---------------
> 1 file changed, 14 insertions(+), 15 deletions(-)
I see a couple of problems with this patch:
- please Cc: all scheduler maintainers to scheduler patches.
- please fix the title of the patch: have a look at 'git log
kernel/sched/cpuacct.c' how recent titles to that code look like.
- when referring to functions in changelogs, please add '()' to separate them
from
variable and other names. I.e. it's "cpuacct_stats_show()".
> static int cpuacct_stats_show(struct seq_file *sf, void *v)
> {
> struct cpuacct *ca = css_ca(seq_css(sf));
> + s64 val[CPUACCT_STAT_NSTATS];
> int cpu;
> - s64 val = 0;
> + int stat;
>
> + memset(val, 0, sizeof(val));
> for_each_possible_cpu(cpu) {
> - struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat, cpu);
> - val += kcpustat->cpustat[CPUTIME_USER];
> - val += kcpustat->cpustat[CPUTIME_NICE];
> + struct kernel_cpustat *kcpustat = per_cpu_ptr(ca->cpustat,
> + cpu);
Horrible taste: in what universe is that linebreak an improvement to the code?
> + val[CPUACCT_STAT_USER] += kcpustat->cpustat[CPUTIME_USER];
Also, please put a newline between variable definitions and the first
non-definition C statement...
> + val[CPUACCT_STAT_USER] += kcpustat->cpustat[CPUTIME_NICE];
> + val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_SYSTEM];
> + val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_IRQ];
> + val[CPUACCT_STAT_SYSTEM] += kcpustat->cpustat[CPUTIME_SOFTIRQ];
Also, if you introduce a helper variable to shorten the code, you might as well
introduce one for the cpustat array itself, and skip the whole 'kcpustat->'
repetition ...
Thanks,
Ingo
