From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751810AbdHMXUk convert rfc822-to-8bit (ORCPT ); Sun, 13 Aug 2017 19:20:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37226 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751202AbdHMXUj (ORCPT ); Sun, 13 Aug 2017 19:20:39 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 40812C0C3840 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=longman@redhat.com Subject: Re: [PATCH 3/3] cgroup: Implement cgroup2 basic CPU usage accounting To: Tejun Heo 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 References: <20170811163754.3939102-1-tj@kernel.org> <20170811163754.3939102-4-tj@kernel.org> <20170813194421.GB1438922@devbig577.frc2.facebook.com> From: Waiman Long Organization: Red Hat Message-ID: Date: Sun, 13 Aug 2017 19:20:35 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20170813194421.GB1438922@devbig577.frc2.facebook.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Content-Language: en-US X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Sun, 13 Aug 2017 23:20:39 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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