From: Waiman Long <longman@redhat.com> To: Tejun Heo <tj@kernel.org> Cc: lizefan@huawei.com, hannes@cmpxchg.org, peterz@infradead.org, mingo@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, guro@fb.com Subject: Re: [PATCH 3/3] cgroup: Implement cgroup2 basic CPU usage accounting Date: Sun, 13 Aug 2017 19:20:35 -0400 [thread overview] Message-ID: <a5ff145e-e2ca-d852-bd40-bc94ccc98167@redhat.com> (raw) In-Reply-To: <20170813194421.GB1438922@devbig577.frc2.facebook.com> On 08/13/2017 03:44 PM, Tejun Heo wrote: > Hello, Waiman. > > On Fri, Aug 11, 2017 at 04:19:07PM -0400, Waiman Long wrote: >>> * CPU usage stats are collected and shown in "cgroup.stat" with "cpu." >>> prefix. Total usage is collected from scheduling events. User/sys >>> breakdown is sourced from tick sampling and adjusted to the usage >>> using cputime_adjuts(). >> Typo: cputime_adjust() > Oops, will fix. > >>> +static DEFINE_MUTEX(cgroup_stat_mutex); >>> +static DEFINE_PER_CPU(raw_spinlock_t, cgroup_cpu_stat_lock); >> If the hierarchy is large enough and the stat data hasn't been read >> recently, it may take a while to accumulate all the stat data even for >> one cpu in cgroup_stat_flush_locked(). So I think it will make more >> sense to use regular spinlock_t instead of raw_spinlock_t. > They need to be raw spinlocks because the accounting side may grab > them while scheduling. If the accumulating latency becomes > problematic, we can test for need_resched and spin_needbreak and break > out as necessary. The iteration logic is built to allow that and an > earlier revision actually did that but I wasn't sure whether it's > actually necessary and removed it for simplicity. > > If the latency ever becomes a problem, resurrecting that isn't > difficult at all. Right, I forgot they will be used by the scheduler. I think it is prudent to limit the latency, but it can be another patch when the need arises. >>> +static struct cgroup *cgroup_cpu_stat_next_updated(struct cgroup *pos, >>> + struct cgroup *root, int cpu) >> This function is trying to unwind one cgrp from the updated_children and >> updated_next linkage. It is somewhat like the opposite of >> cgroup_cpu_stat_updated(). I just feel like its name isn't intuitive >> enough to convey what it is doing. Maybe use name like >> cgroup_cpu_stat_unlink_one() to match cgroup_cpu_stat_flush_one(). > Hmm... the name comes from it being an iterator - most interators are > named _next. But, yeah, the name doesn't signifiy that it unlinks as > it goes along. I'll rename it to cgroup_cpu_stat_pop_updated(). I am fine with that. Thanks, Longman
WARNING: multiple messages have this Message-ID (diff)
From: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@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, 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, guro-b10kYP2dOMg@public.gmane.org Subject: Re: [PATCH 3/3] cgroup: Implement cgroup2 basic CPU usage accounting Date: Sun, 13 Aug 2017 19:20:35 -0400 [thread overview] Message-ID: <a5ff145e-e2ca-d852-bd40-bc94ccc98167@redhat.com> (raw) In-Reply-To: <20170813194421.GB1438922-4dN5La/x3IkLX0oZNxdnEQ2O0Ztt9esIQQ4Iyu8u01E@public.gmane.org> On 08/13/2017 03:44 PM, Tejun Heo wrote: > Hello, Waiman. > > On Fri, Aug 11, 2017 at 04:19:07PM -0400, Waiman Long wrote: >>> * CPU usage stats are collected and shown in "cgroup.stat" with "cpu." >>> prefix. Total usage is collected from scheduling events. User/sys >>> breakdown is sourced from tick sampling and adjusted to the usage >>> using cputime_adjuts(). >> Typo: cputime_adjust() > Oops, will fix. > >>> +static DEFINE_MUTEX(cgroup_stat_mutex); >>> +static DEFINE_PER_CPU(raw_spinlock_t, cgroup_cpu_stat_lock); >> If the hierarchy is large enough and the stat data hasn't been read >> recently, it may take a while to accumulate all the stat data even for >> one cpu in cgroup_stat_flush_locked(). So I think it will make more >> sense to use regular spinlock_t instead of raw_spinlock_t. > They need to be raw spinlocks because the accounting side may grab > them while scheduling. If the accumulating latency becomes > problematic, we can test for need_resched and spin_needbreak and break > out as necessary. The iteration logic is built to allow that and an > earlier revision actually did that but I wasn't sure whether it's > actually necessary and removed it for simplicity. > > If the latency ever becomes a problem, resurrecting that isn't > difficult at all. Right, I forgot they will be used by the scheduler. I think it is prudent to limit the latency, but it can be another patch when the need arises. >>> +static struct cgroup *cgroup_cpu_stat_next_updated(struct cgroup *pos, >>> + struct cgroup *root, int cpu) >> This function is trying to unwind one cgrp from the updated_children and >> updated_next linkage. It is somewhat like the opposite of >> cgroup_cpu_stat_updated(). I just feel like its name isn't intuitive >> enough to convey what it is doing. Maybe use name like >> cgroup_cpu_stat_unlink_one() to match cgroup_cpu_stat_flush_one(). > Hmm... the name comes from it being an iterator - most interators are > named _next. But, yeah, the name doesn't signifiy that it unlinks as > it goes along. I'll rename it to cgroup_cpu_stat_pop_updated(). I am fine with that. Thanks, Longman
next prev parent reply other threads:[~2017-08-13 23:20 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 [this message] 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 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=a5ff145e-e2ca-d852-bd40-bc94ccc98167@redhat.com \ --to=longman@redhat.com \ --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=luto@amacapital.net \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=pjt@google.com \ --cc=tj@kernel.org \ --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.