From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752085AbdHQNOF (ORCPT ); Thu, 17 Aug 2017 09:14:05 -0400 Received: from mail-qk0-f194.google.com ([209.85.220.194]:37086 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751620AbdHQNOD (ORCPT ); Thu, 17 Aug 2017 09:14:03 -0400 Date: Thu, 17 Aug 2017 06:13:59 -0700 From: Tejun Heo To: Roman Gushchin Cc: lizefan@huawei.com, hannes@cmpxchg.org, peterz@infradead.org, mingo@redhat.com, longman@redhat.com, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com, pjt@google.com, luto@amacapital.net, efault@gmx.de, torvalds@linux-foundation.org Subject: Re: [PATCH 3/3] cgroup: Implement cgroup2 basic CPU usage accounting Message-ID: <20170817131359.GC3238792@devbig577.frc2.facebook.com> References: <20170811163754.3939102-1-tj@kernel.org> <20170811163754.3939102-4-tj@kernel.org> <20170817120741.GA22854@castle.DHCP.thefacebook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170817120741.GA22854@castle.DHCP.thefacebook.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On Thu, Aug 17, 2017 at 01:07:41PM +0100, Roman Gushchin wrote: > On Fri, Aug 11, 2017 at 09:37:54AM -0700, Tejun Heo wrote: > > In cgroup1, while cpuacct isn't actually controlling any resources, it > > is a separate controller due to combinaton of two factors - > > s/combinaton/combination Fixed. > > @@ -4466,6 +4470,8 @@ static void css_free_work_fn(struct work_struct *work) > > */ > > cgroup_put(cgroup_parent(cgrp)); > > kernfs_put(cgrp->kn); > > + if (cgroup_on_dfl(cgrp)) > > + cgroup_stat_exit(cgrp); > > It looks like this "if (cgroup_on_dfl(cgrp))" works here and further similar to > "#ifdef CGROUP_V2". I wonder, if it's better to move this check into the > calling function: cgroup_stat_exit() in this case. I have a slight preference to keeping these topology-aware tests on the core / management part of code because that makes it obvious that these stats aren't available for all cgroups. Also, during cgroup creation, because @cgrp isn't linked to its parent yet, we'd have to pass @parent to cgroup_stat_init/exit() too. > > +void cgroup_stat_show_cputime(struct seq_file *seq, const char *prefix) > > +{ > > What are any other possible prefix values except "cpu."? Empty string when the stats are exposed through cpu.stat. > > +void __init cgroup_stat_boot(void) > > +{ > > + int cpu; > > + > > + for_each_possible_cpu(cpu) > > + raw_spin_lock_init(per_cpu_ptr(&cgroup_cpu_stat_lock, cpu)); > > + > > + WARN_ON(cgroup_stat_init(&cgrp_dfl_root.cgrp)); > > I'm not sure WARN_ON() is enough here: if cgroup_stat_init() returned -ENOMEM, > the following OOPS is not avoidable, as you don't check cpu_stat pointer. > But it's very unlikely, of course. Sure, will switch to BUG_ON(). Thanks. -- tejun