From: Tejun Heo <tj@kernel.org> To: Roman Gushchin <guro@fb.com> 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 Date: Thu, 17 Aug 2017 06:13:59 -0700 [thread overview] Message-ID: <20170817131359.GC3238792@devbig577.frc2.facebook.com> (raw) In-Reply-To: <20170817120741.GA22854@castle.DHCP.thefacebook.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> To: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org> Cc: lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org, peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kernel-team-b10kYP2dOMg@public.gmane.org, pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org, efault-Mmb7MZpHnFY@public.gmane.org, torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org Subject: Re: [PATCH 3/3] cgroup: Implement cgroup2 basic CPU usage accounting Date: Thu, 17 Aug 2017 06:13:59 -0700 [thread overview] Message-ID: <20170817131359.GC3238792@devbig577.frc2.facebook.com> (raw) In-Reply-To: <20170817120741.GA22854-B3w7+ongkCiLfgCeKHXN1g2O0Ztt9esIQQ4Iyu8u01E@public.gmane.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
next prev parent reply other threads:[~2017-08-17 13:14 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-08-11 16:37 [PATCHSET for-4.14] cgroup, sched: cgroup2 basic resource accounting Tejun Heo 2017-08-11 16:37 ` Tejun Heo 2017-08-11 16:37 ` [PATCH 1/3] sched/cputime: Expose cputime_adjust() Tejun Heo 2017-08-11 16:37 ` [PATCH 2/3] cpuacct: Introduce cgroup_account_cputime[_field]() Tejun Heo 2017-08-11 16:37 ` Tejun Heo 2017-08-11 17:28 ` [PATCH v2 " Tejun Heo 2017-08-11 17:28 ` Tejun Heo 2017-08-11 16:37 ` [PATCH 3/3] cgroup: Implement cgroup2 basic CPU usage accounting Tejun Heo 2017-08-11 20:19 ` Waiman Long 2017-08-11 20:19 ` Waiman Long 2017-08-13 19:44 ` Tejun Heo 2017-08-13 19:44 ` Tejun Heo 2017-08-13 23:20 ` Waiman Long 2017-08-13 23:20 ` Waiman Long 2017-08-17 12:07 ` Roman Gushchin 2017-08-17 12:07 ` Roman Gushchin 2017-08-17 13:13 ` Tejun Heo [this message] 2017-08-17 13:13 ` Tejun Heo 2017-08-29 14:32 ` Peter Zijlstra 2017-08-29 14:32 ` Peter Zijlstra 2017-08-29 15:24 ` Tejun Heo 2017-08-29 15:24 ` Tejun Heo 2017-08-29 15:32 ` Waiman Long 2017-08-29 15:32 ` Waiman Long 2017-08-29 15:42 ` Tejun Heo 2017-08-29 15:59 ` Peter Zijlstra 2017-08-29 15:59 ` Peter Zijlstra 2017-08-29 17:43 ` [PATCH v2 " Tejun Heo 2017-08-29 17:43 ` Tejun Heo 2017-08-29 18:06 ` Waiman Long 2017-08-29 18:06 ` Waiman Long 2017-08-29 18:10 ` Tejun Heo 2017-08-16 18:52 ` [PATCHSET for-4.14] cgroup, sched: cgroup2 basic resource accounting Tejun Heo 2017-08-17 8:13 ` Ingo Molnar 2017-08-17 13:01 ` Tejun Heo 2017-08-17 15:03 ` Ingo Molnar 2017-08-17 15:03 ` Ingo Molnar 2017-08-24 17:20 ` Tejun Heo 2017-08-24 17:20 ` Tejun Heo 2017-09-22 18:05 ` Tejun Heo 2017-09-22 18:05 ` Tejun Heo 2017-09-23 13:37 ` Peter Zijlstra 2017-09-23 13:37 ` Peter Zijlstra 2017-09-23 13:44 ` Tejun Heo 2017-09-23 13:44 ` Tejun Heo 2017-09-25 7:27 ` Peter Zijlstra 2017-09-25 7:27 ` Peter Zijlstra 2017-09-25 15:07 ` Tejun Heo 2017-09-25 15:07 ` Tejun Heo 2017-09-25 21:10 ` [PATCH cgroup/for-4.15] cgroup: statically initialize init_css_set->dfl_cgrp Tejun Heo 2017-09-25 21:10 ` Tejun Heo 2017-09-25 21:34 ` [PATCH cgroup/for-4.15] sched/cputime: Add dummy cputime_adjust() implementation for CONFIG_VIRT_CPU_ACCOUNTING_NATIVE Tejun Heo 2017-09-25 21:34 ` Tejun Heo 2017-09-26 9:10 ` Peter Zijlstra
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170817131359.GC3238792@devbig577.frc2.facebook.com \ --to=tj@kernel.org \ --cc=cgroups@vger.kernel.org \ --cc=efault@gmx.de \ --cc=guro@fb.com \ --cc=hannes@cmpxchg.org \ --cc=kernel-team@fb.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lizefan@huawei.com \ --cc=longman@redhat.com \ --cc=luto@amacapital.net \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=pjt@google.com \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.