From: Thomas Gleixner <tglx@linutronix.de>
To: Vikas Shivappa <vikas.shivappa@linux.intel.com>
Cc: vikas.shivappa@intel.com, davidcc@google.com, eranian@google.com,
linux-kernel@vger.kernel.org, x86@kernel.org, hpa@zytor.com,
mingo@kernel.org, peterz@infradead.org, ravi.v.shankar@intel.com,
tony.luck@intel.com, fenghua.yu@intel.com, andi.kleen@intel.com,
h.peter.anvin@intel.com
Subject: Re: [PATCH 08/12] x86/cqm: Add support for monitoring task and cgroup together
Date: Tue, 17 Jan 2017 17:11:55 +0100 (CET) [thread overview]
Message-ID: <alpine.DEB.2.20.1701171652270.3495@nanos> (raw)
In-Reply-To: <1483740005-23499-9-git-send-email-vikas.shivappa@linux.intel.com>
On Fri, 6 Jan 2017, Vikas Shivappa wrote:
> From: Vikas Shivappa<vikas.shivappa@intel.com>
>
> This patch adds support to monitor a cgroup x and a task p1
> when p1 is part of cgroup x. Since we cannot write two RMIDs during
> sched in the driver handles this.
Again you explain WHAT not WHY....
> This patch introduces a u32 *rmid in the task_struck which keeps track
> of the RMIDs associated with the task. There is also a list in the
> arch_info of perf_cgroup called taskmon_list which keeps track of tasks
> in the cgroup that are monitored.
>
> The taskmon_list is modified in 2 scenarios.
> - at event_init of task p1 which is part of a cgroup, add the p1 to the
> cgroup->tskmon_list. At event_destroy delete the task from the list.
> - at the time of task move from cgrp x to a cgp y, if task was monitored
> remove the task from the cgrp x tskmon_list and add it the cgrp y
> tskmon_list.
>
> sched in: When the task p1 is scheduled in, we write the task RMID in
> the PQR_ASSOC MSR
Great information.
> read(for task p1): As any other cqm task event
>
> read(for the cgroup x): When counting for cgroup, the taskmon list is
> traversed and the corresponding RMID counts are added.
>
> Tests: Monitoring a cgroup x and a task with in the cgroup x should
> work.
Emphasis on should.
> +static inline int add_cgrp_tskmon_entry(u32 *rmid, struct list_head *l)
> +{
> + struct tsk_rmid_entry *entry;
> +
> + entry = kzalloc(sizeof(struct tsk_rmid_entry), GFP_KERNEL);
> + if (!entry)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&entry->list);
> + entry->rmid = rmid;
> +
> + list_add_tail(&entry->list, l);
> +
> + return 0;
And this function has a return value because the return value is never
evaluated at any call site.
> +}
> +
> +static inline void del_cgrp_tskmon_entry(u32 *rmid, struct list_head *l)
> +{
> + struct tsk_rmid_entry *entry = NULL, *tmp1;
And where is *tmp2? What is wrong with simply *tmp?
> +
> + list_for_each_entry_safe(entry, tmp1, l, list) {
> + if (entry->rmid == rmid) {
> +
> + list_del(&entry->list);
> + kfree(entry);
> + break;
> + }
> + }
> +}
> +
> #ifdef CONFIG_CGROUP_PERF
> struct cgrp_cqm_info *cqminfo_from_tsk(struct task_struct *tsk)
> {
> @@ -380,6 +410,49 @@ struct cgrp_cqm_info *cqminfo_from_tsk(struct task_struct *tsk)
> }
> #endif
>
> +static inline void
> + cgrp_tskmon_update(struct task_struct *tsk, u32 *rmid, bool ena)
Sigh
> +{
> + struct cgrp_cqm_info *ccinfo = NULL;
> +
> +#ifdef CONFIG_CGROUP_PERF
> + ccinfo = cqminfo_from_tsk(tsk);
> +#endif
> + if (!ccinfo)
> + return;
> +
> + if (ena)
> + add_cgrp_tskmon_entry(rmid, &ccinfo->tskmon_rlist);
> + else
> + del_cgrp_tskmon_entry(rmid, &ccinfo->tskmon_rlist);
> +}
> +
> +static int cqm_assign_task_rmid(struct perf_event *event, u32 *rmid)
> +{
> + struct task_struct *tsk;
> + int ret = 0;
> +
> + rcu_read_lock();
> + tsk = event->hw.target;
> + if (pid_alive(tsk)) {
> + get_task_struct(tsk);
This works because after the pid_alive() check the task cannot be
released before issuing get_task_struct(), right?
That's voodoo protection. How would a non alive task end up as event
target?
> +
> + if (rmid != NULL)
> + cgrp_tskmon_update(tsk, rmid, true);
> + else
> + cgrp_tskmon_update(tsk, tsk->rmid, false);
> +
> + tsk->rmid = rmid;
> +
> + put_task_struct(tsk);
> + } else {
> + ret = -EINVAL;
> + }
> + rcu_read_unlock();
> +
> + return ret;
> +}
> +
> static inline void cqm_enable_mon(struct cgrp_cqm_info *cqm_info, u32 *rmid)
> {
> if (rmid != NULL) {
> @@ -429,8 +502,12 @@ static void cqm_assign_hier_rmid(struct cgroup_subsys_state *rcss, u32 *rmid)
>
> static int cqm_assign_rmid(struct perf_event *event, u32 *rmid)
> {
> + if (is_task_event(event)) {
> + if (cqm_assign_task_rmid(event, rmid))
> + return -EINVAL;
> + }
> #ifdef CONFIG_CGROUP_PERF
> - if (is_cgroup_event(event)) {
> + else if (is_cgroup_event(event)) {
> cqm_assign_hier_rmid(&event->cgrp->css, rmid);
> }
So you keep adding stuff to cqm_assign_rmid() which handles enable and
disable. But the only call site is in cqm_event_free_rmid() which calls
that function with rmid = NULL, i.e. disable.
Can you finally explain how this is supposed to work and how all of this
has been tested and validated?
If you had used the already known 'Tests: Same as before' line to the
changelog, then we would have known that it's broken as before w/o looking
at the patch.
So the new variant of 'broken' is: Bla should work ....
Thanks,
tglx
next prev parent reply other threads:[~2017-01-17 18:14 UTC|newest]
Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-06 21:59 [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Vikas Shivappa
2017-01-06 21:59 ` [PATCH 01/12] Documentation, x86/cqm: Intel Resource Monitoring Documentation Vikas Shivappa
2017-01-06 21:59 ` [PATCH 02/12] x86/cqm: Remove cqm recycling/conflict handling Vikas Shivappa
2017-01-06 21:59 ` [PATCH 03/12] x86/rdt: Add rdt common/cqm compile option Vikas Shivappa
2017-01-16 18:05 ` Thomas Gleixner
2017-01-17 17:25 ` Shivappa Vikas
2017-01-06 21:59 ` [PATCH 04/12] x86/cqm: Add Per pkg rmid support Vikas Shivappa
2017-01-16 18:15 ` [PATCH 04/12] x86/cqm: Add Per pkg rmid support\ Thomas Gleixner
2017-01-17 19:11 ` Shivappa Vikas
2017-01-06 21:59 ` [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Vikas Shivappa
2017-01-17 12:11 ` Thomas Gleixner
2017-01-17 12:31 ` Peter Zijlstra
2017-01-18 2:14 ` Shivappa Vikas
2017-01-17 13:46 ` Thomas Gleixner
2017-01-17 20:22 ` Shivappa Vikas
2017-01-17 21:31 ` Thomas Gleixner
2017-01-17 15:26 ` Peter Zijlstra
2017-01-17 20:27 ` Shivappa Vikas
2017-01-06 21:59 ` [PATCH 06/12] x86/cqm: Add cgroup hierarchical monitoring support Vikas Shivappa
2017-01-17 14:07 ` Thomas Gleixner
2017-01-06 22:00 ` [PATCH 07/12] x86/rdt,cqm: Scheduling support update Vikas Shivappa
2017-01-17 21:58 ` Thomas Gleixner
2017-01-17 22:30 ` Shivappa Vikas
2017-01-06 22:00 ` [PATCH 08/12] x86/cqm: Add support for monitoring task and cgroup together Vikas Shivappa
2017-01-17 16:11 ` Thomas Gleixner [this message]
2017-01-06 22:00 ` [PATCH 09/12] x86/cqm: Add RMID reuse Vikas Shivappa
2017-01-17 16:59 ` Thomas Gleixner
2017-01-18 0:26 ` Shivappa Vikas
2017-01-06 22:00 ` [PATCH 10/12] perf/core,x86/cqm: Add read for Cgroup events,per pkg reads Vikas Shivappa
2017-01-06 22:00 ` [PATCH 11/12] perf/stat: fix bug in handling events in error state Vikas Shivappa
2017-01-06 22:00 ` [PATCH 12/12] perf/stat: revamp read error handling, snapshot and per_pkg events Vikas Shivappa
2017-01-17 17:31 ` [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner
2017-01-18 2:38 ` Shivappa Vikas
2017-01-18 8:53 ` Thomas Gleixner
2017-01-18 9:56 ` Peter Zijlstra
2017-01-19 19:59 ` Shivappa Vikas
2017-01-18 19:41 ` Shivappa Vikas
2017-01-18 21:03 ` David Carrillo-Cisneros
2017-01-19 17:41 ` Thomas Gleixner
2017-01-20 7:37 ` David Carrillo-Cisneros
2017-01-20 8:30 ` Thomas Gleixner
2017-01-20 20:27 ` David Carrillo-Cisneros
2017-01-18 21:16 ` Yu, Fenghua
2017-01-19 2:09 ` David Carrillo-Cisneros
2017-01-19 16:58 ` David Carrillo-Cisneros
2017-01-19 17:54 ` Thomas Gleixner
2017-01-19 2:21 ` Vikas Shivappa
2017-01-19 6:45 ` Stephane Eranian
2017-01-19 18:03 ` Thomas Gleixner
2017-01-20 2:32 ` Vikas Shivappa
2017-01-20 7:58 ` David Carrillo-Cisneros
2017-01-20 13:28 ` Thomas Gleixner
2017-01-20 20:11 ` David Carrillo-Cisneros
2017-01-20 21:08 ` Shivappa Vikas
2017-01-20 21:44 ` David Carrillo-Cisneros
2017-01-20 23:51 ` Shivappa Vikas
2017-02-08 10:13 ` Peter Zijlstra
2017-01-23 9:47 ` Thomas Gleixner
2017-01-23 11:30 ` Peter Zijlstra
2017-02-01 20:08 ` Luck, Tony
2017-02-01 23:12 ` David Carrillo-Cisneros
2017-02-02 17:39 ` Luck, Tony
2017-02-02 19:33 ` Luck, Tony
2017-02-02 20:20 ` Shivappa Vikas
2017-02-02 20:22 ` David Carrillo-Cisneros
2017-02-02 23:41 ` Luck, Tony
2017-02-03 1:40 ` David Carrillo-Cisneros
2017-02-03 2:14 ` David Carrillo-Cisneros
2017-02-03 17:52 ` Luck, Tony
2017-02-03 21:08 ` David Carrillo-Cisneros
2017-02-03 22:24 ` Luck, Tony
2017-02-07 8:08 ` Stephane Eranian
2017-02-07 18:52 ` Luck, Tony
2017-02-08 19:31 ` Stephane Eranian
2017-02-07 20:10 ` Shivappa Vikas
2017-02-17 13:41 ` Thomas Gleixner
2017-02-06 18:54 ` Luck, Tony
2017-02-06 21:22 ` Luck, Tony
2017-02-06 21:36 ` Shivappa Vikas
2017-02-06 21:46 ` David Carrillo-Cisneros
2017-02-06 22:16 ` David Carrillo-Cisneros
2017-02-06 23:27 ` Luck, Tony
2017-02-07 0:33 ` David Carrillo-Cisneros
2017-02-02 0:35 ` Andi Kleen
2017-02-02 1:12 ` David Carrillo-Cisneros
2017-02-02 1:19 ` Andi Kleen
2017-02-02 1:22 ` Yu, Fenghua
2017-02-02 17:51 ` Shivappa Vikas
2017-02-08 10:11 ` Peter Zijlstra
2017-01-20 20:40 ` Shivappa Vikas
2017-01-20 19:31 ` Stephane Eranian
-- strict thread matches above, loose matches on Subject: below --
2017-01-06 21:56 [PATCH 00/12 V5] Cqm2: Intel Cache quality of " Vikas Shivappa
2017-01-06 21:56 ` [PATCH 08/12] x86/cqm: Add support for monitoring task and cgroup together Vikas Shivappa
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=alpine.DEB.2.20.1701171652270.3495@nanos \
--to=tglx@linutronix.de \
--cc=andi.kleen@intel.com \
--cc=davidcc@google.com \
--cc=eranian@google.com \
--cc=fenghua.yu@intel.com \
--cc=h.peter.anvin@intel.com \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=ravi.v.shankar@intel.com \
--cc=tony.luck@intel.com \
--cc=vikas.shivappa@intel.com \
--cc=vikas.shivappa@linux.intel.com \
--cc=x86@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).