From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751361AbcCJN1m (ORCPT ); Thu, 10 Mar 2016 08:27:42 -0500 Received: from casper.infradead.org ([85.118.1.10]:34094 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750751AbcCJN1c (ORCPT ); Thu, 10 Mar 2016 08:27:32 -0500 Date: Thu, 10 Mar 2016 14:27:28 +0100 From: Peter Zijlstra To: Zhao Lei Cc: cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, mingo@redhat.com, tj@kernel.org, Yang Dongsheng Subject: Re: [PATCH v2 2/2] cpuacct: split usage into user_usage and sys_usage. Message-ID: <20160310132728.GX6344@twins.programming.kicks-ass.net> References: <340a5ec9cb4e7f97b3a41f3dc657b9aca4c76f25.1457082730.git.zhaolei@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <340a5ec9cb4e7f97b3a41f3dc657b9aca4c76f25.1457082730.git.zhaolei@cn.fujitsu.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 04, 2016 at 05:47:06PM +0800, Zhao Lei wrote: > +static u64 cpuacct_cpuusage_read(struct cpuacct *ca, int cpu, > + enum cpuacct_usage_index index) > { > + struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu); > + u64 data = 0; > + int i = 0; > + > + /* > + * We allow index == CPUACCT_USAGE_NRUSAGE here to read > + * the sum of suages. > + */ > + BUG_ON(index > CPUACCT_USAGE_NRUSAGE); > + > + if (index == CPUACCT_USAGE_NRUSAGE) { > + raw_spin_lock_irq(&cpu_rq(cpu)->lock); > + for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++) > + data += cpuusage->usages[i]; > + raw_spin_unlock_irq(&cpu_rq(cpu)->lock); Why do you unconditionally take the lock here? You really don't need it on 64 bit. > + > + goto out; > + } > > #ifndef CONFIG_64BIT > /* > * Take rq->lock to make 64-bit read safe on 32-bit platforms. > */ > raw_spin_lock_irq(&cpu_rq(cpu)->lock); > + data = cpuusage->usages[index]; > raw_spin_unlock_irq(&cpu_rq(cpu)->lock); > #else > + data = cpuusage->usages[index]; > #endif > > +out: > return data; > } > > +static void cpuacct_cpuusage_write(struct cpuacct *ca, int cpu, > + enum cpuacct_usage_index index, u64 val) > { > + struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu); > + int i = 0; > + > + /* > + * We allow index == CPUACCT_USAGE_NRUSAGE here to write > + * val to each index of usages. > + */ > + BUG_ON(index > CPUACCT_USAGE_NRUSAGE); > + > + if (index == CPUACCT_USAGE_NRUSAGE) { > + raw_spin_lock_irq(&cpu_rq(cpu)->lock); > + for (i = 0; i < CPUACCT_USAGE_NRUSAGE; i++) > + cpuusage->usages[i] = val; > + raw_spin_unlock_irq(&cpu_rq(cpu)->lock); > + > + return; > + } Same for the above, and the below is dead code, you only ever call this with NRUSAGE. > #ifndef CONFIG_64BIT > /* > * Take rq->lock to make 64-bit write safe on 32-bit platforms. > */ > raw_spin_lock_irq(&cpu_rq(cpu)->lock); > + cpuusage->usages[index] = val; > raw_spin_unlock_irq(&cpu_rq(cpu)->lock); > #else > + cpuusage->usages[index] = val; > #endif > } > > @@ -246,9 +344,15 @@ void cpuacct_charge(struct task_struct *tsk, u64 cputime) > > ca = task_ca(tsk); > > + user_time = user_mode(task_pt_regs(tsk)); > + > while (true) { > - u64 *cpuusage = per_cpu_ptr(ca->cpuusage, cpu); > - *cpuusage += cputime; > + struct cpuacct_usage *cpuusage = per_cpu_ptr(ca->cpuusage, cpu); > + > + if (user_time) > + cpuusage->usages[CPUACCT_USAGE_USER] += cputime; > + else > + cpuusage->usages[CPUACCT_USAGE_SYSTEM] += cputime; > > ca = parent_ca(ca); > if (!ca) Have you tried to measure the performance impact of this? Also, that code seems particularly silly for not using this_cpu_ptr(). After all, we only ever call this on current. Also that ca iteration looks daft, should we fix that to read: for (ca = task_ca(tsk); ca; ca = parent_ca(ca))