From: "Liang, Kan" <kan.liang@linux.intel.com>
To: Peter Zijlstra <peterz@infradead.org>, Ian Rogers <irogers@google.com>
Cc: tglx@linutronix.de, mingo@redhat.com,
linux-kernel@vger.kernel.org,
Stephane Eranian <eranian@google.com>,
tj@kernel.org, ak@linux.intel.com
Subject: Re: [PATCH 3/4] perf cgroup: Add cgroup ID as a key of RB tree
Date: Tue, 30 Apr 2019 11:46:54 -0400 [thread overview]
Message-ID: <5bd40a66-bb20-44c7-9c0e-35bfa1d271f6@linux.intel.com> (raw)
In-Reply-To: <20190430090847.GM2623@hirez.programming.kicks-ass.net>
On 4/30/2019 5:08 AM, Peter Zijlstra wrote:
> On Mon, Apr 29, 2019 at 04:02:33PM -0700, Ian Rogers wrote:
>> This is very interesting. How does the code handle cgroup hierarchies?
>> For example, if we have:
>>
>> cgroup0 is the cgroup root
>> cgroup1 whose parent is cgroup0
>> cgroup2 whose parent is cgroup1
>>
>> we have task0 running in cgroup0, task1 in cgroup1, task2 in cgroup2
>> and then a perf command line like:
>> perf stat -e cycles,cycles,cycles -G cgroup0,cgroup1,cgroup2 --no-merge sleep 10
>>
>> we expected 3 cycles counts:
>> - for cgroup0 including task2, task1 and task0
>> - for cgroup1 including task2 and task1
>> - for cgroup2 just including task2
>>
>> It looks as though:
>> + if (next && (next->cpu == event->cpu) && (next->cgrp_id ==
>> event->cgrp_id))
>>
>> will mean that events will only consider cgroups that directly match
>> the cgroup of the event. Ie we'd get 3 cycles counts of:
>> - for cgroup0 just task0
>> - for cgroup1 just task1
>> - for cgroup2 just task2
> Yeah, I think you're right; the proposed code doesn't capture the
> hierarchy thing at all.
The hierarchies is handled in the next patch as below.
But I once thought we only need to handle directly match. So it will be
return immediately once a match found.
I believe we can fix it by simply remove the "return 0".
> +static int cgroup_visit_groups_merge(struct perf_event_groups *groups, int cpu,
> + int (*func)(struct perf_event *, void *, int (*)(struct perf_event *)),
> + void *data)
> +{
> + struct sched_in_data *sid = data;
> + struct cgroup_subsys_state *css;
> + struct perf_cgroup *cgrp;
> + struct perf_event *evt;
> + u64 cgrp_id;
> +
> + for (css = &sid->cpuctx->cgrp->css; css; css = css->parent) {
> + /* root cgroup doesn't have events */
> + if (css->id == 1)
> + return 0;
> +
> + cgrp = container_of(css, struct perf_cgroup, css);
> + cgrp_id = *this_cpu_ptr(cgrp->cgrp_id);
> + /* Only visit groups when the cgroup has events */
> + if (cgrp_id) {
> + evt = perf_event_groups_first_cgroup(groups, cpu, cgrp_id);
> + while (evt) {
> + if (func(evt, (void *)sid, pmu_filter_match))
> + break;
> + evt = perf_event_groups_next_cgroup(evt);
> + }
> + return 0; <--- need to remove for hierarchies
> + }
> + }
> +
> + return 0;
> +}
Thanks,
Kan
next prev parent reply other threads:[~2019-04-30 15:46 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-29 14:44 [PATCH 0/4] Optimize cgroup context switch kan.liang
2019-04-29 14:44 ` [PATCH 1/4] perf: Fix system-wide events miscounting during cgroup monitoring kan.liang
2019-04-29 15:04 ` Mark Rutland
2019-04-29 15:27 ` Liang, Kan
2019-04-30 8:56 ` Peter Zijlstra
2019-04-30 15:45 ` Liang, Kan
2019-04-29 14:44 ` [PATCH 2/4] perf: Add filter_match() as a parameter for pinned/flexible_sched_in() kan.liang
2019-04-29 15:12 ` Mark Rutland
2019-04-29 15:31 ` Liang, Kan
2019-04-29 16:56 ` Mark Rutland
2019-04-29 14:44 ` [PATCH 3/4] perf cgroup: Add cgroup ID as a key of RB tree kan.liang
2019-04-29 23:02 ` Ian Rogers
2019-04-30 9:08 ` Peter Zijlstra
2019-04-30 15:46 ` Liang, Kan [this message]
2019-04-30 9:03 ` Peter Zijlstra
2019-04-30 15:46 ` Liang, Kan
2019-04-30 9:03 ` Peter Zijlstra
2019-04-29 14:44 ` [PATCH 4/4] perf cgroup: Add fast path for cgroup switch kan.liang
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=5bd40a66-bb20-44c7-9c0e-35bfa1d271f6@linux.intel.com \
--to=kan.liang@linux.intel.com \
--cc=ak@linux.intel.com \
--cc=eranian@google.com \
--cc=irogers@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=tj@kernel.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).