From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752496AbcELHry (ORCPT ); Thu, 12 May 2016 03:47:54 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:32781 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751614AbcELHrw (ORCPT ); Thu, 12 May 2016 03:47:52 -0400 Date: Thu, 12 May 2016 09:47:47 +0200 From: Ingo Molnar To: Zhao Lei Cc: linux-kernel@vger.kernel.org, Peter Zijlstra Subject: Re: [PATCH 2/3] cpuacct: Simplify cpuacct_stats_show Message-ID: <20160512074747.GA31930@gmail.com> References: <9199a847ae92914ddcc0c69e75aaf3084b1ac3e0.1462901697.git.zhaolei@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9199a847ae92914ddcc0c69e75aaf3084b1ac3e0.1462901697.git.zhaolei@cn.fujitsu.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Zhao Lei 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 > --- > 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