Skip to contentSkip to conversation

perf_events: add cgroup support (v7)

PATCH In review Stephane Eranian 3 participants 8 messages Jan 3, 2011 – Jan 10, 2011

Subsystems (2)
perf events cgroup
lore
Share & tools
lore

Stephane Eranian

author
#0 lore raw

PATCH 4/5 perf_events: add cgroup support (v7)

This kernel patch adds the ability to filter monitoring based on container groups (cgroups). This is for use in per-cpu mode only.

The cgroup to monitor is passed as a file descriptor in the pid argument to the syscall. The file descriptor must be opened to the cgroup name in the cgroup filesystem. For instance, if the cgroup name is foo and cgroupfs is mounted in /cgroup, then the file descriptor is opened to /cgroup/foo. Cgroup mode is activated by passing PERF_FLAG_PID_CGROUP in the flags argument to the syscall.

For instance to measure in cgroup foo on CPU1 assuming cgroupfs is mounted under /cgroup:

struct perf_event_attr attr;
int cgroup_fd, fd;

cgroup_fd = open("/cgroup/foo", O_RDONLY);
fd = perf_event_open(&attr, cgroup_fd, 1, -1, PERF_FLAG_PID_CGROUP); close(cgroup_fd);

Signed-off-by Stephane Eranian <[email protected]>

---

include/linux/cgroup.h +1 -0
index ed4ba11..5449b61 100644
@@ -626,6 +626,7 @@ bool css_is_ancestor(struct cgroup_subsys_state *cg,
 /* Get id and depth of css */
 unsigned short css_id(struct cgroup_subsys_state *css);
 unsigned short css_depth(struct cgroup_subsys_state *css);
+struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id);
 
 #else /* !CONFIG_CGROUPS */
include/linux/cgroup_subsys.h +4 -0
index ccefff0..cdbfcb8 100644
@@ -65,4 +65,8 @@ SUBSYS(net_cls)
 SUBSYS(blkio)
 #endif
 
+#ifdef CONFIG_CGROUP_PERF
+SUBSYS(perf)
+#endif
+
 /* */
include/linux/perf_event.h +26 -3
index dda5b0a..8843da4 100644
@@ -464,6 +464,7 @@ enum perf_callchain_context {
 
 #define PERF_FLAG_FD_NO_GROUP	(1U << 0)
 #define PERF_FLAG_FD_OUTPUT	(1U << 1)
+#define PERF_FLAG_PID_CGROUP	(1U << 2) /* pid=cgroup id, per-cpu mode only */
 
 #ifdef __KERNEL__
 /*

@@ -471,6 +472,7 @@ enum perf_callchain_context {
  */
 
 #ifdef CONFIG_PERF_EVENTS
+# include <linux/cgroup.h>
 # include <asm/perf_event.h>
 # include <asm/local64.h>
 #endif

@@ -716,6 +718,22 @@ struct swevent_hlist {
 #define PERF_ATTACH_GROUP	0x02
 #define PERF_ATTACH_TASK	0x04
 
+#ifdef CONFIG_CGROUP_PERF
+/*
+ * perf_cgroup_info keeps track of time_enabled for a cgroup.
+ * This is a per-cpu dynamically allocated data structure.
+ */
+struct perf_cgroup_info {
+	u64 time;
+	u64 timestamp;
+};
+
+struct perf_cgroup {
+	struct cgroup_subsys_state css;
+	struct perf_cgroup_info *info;	/* timing info, one per cpu */
+};
+#endif
+
 /**
  * struct perf_event - performance event kernel representation:
  */

@@ -832,6 +850,10 @@ struct perf_event {
 	struct event_filter		*filter;
 #endif
 
+#ifdef CONFIG_CGROUP_PERF
+	struct perf_cgroup		*cgrp; /* cgroup event is attach to */
+#endif
+
 #endif /* CONFIG_PERF_EVENTS */
 };
 

@@ -886,6 +908,7 @@ struct perf_event_context {
 	u64				generation;
 	int				pin_count;
 	struct rcu_head			rcu_head;
+	int				nr_cgroups; /* cgroup events present */
 };
 
 /*

@@ -1040,11 +1063,11 @@ have_event:
 	__perf_sw_event(event_id, nr, nmi, regs, addr);
 }
 
-extern atomic_t perf_task_events;
+extern atomic_t perf_sched_events;
 
 static inline void perf_event_task_sched_in(struct task_struct *task)
 {
-	COND_STMT(&perf_task_events, __perf_event_task_sched_in(task));
+	COND_STMT(&perf_sched_events, __perf_event_task_sched_in(task));
 }
 
 static inline

@@ -1052,7 +1075,7 @@ void perf_event_task_sched_out(struct task_struct *task, struct task_struct *nex
 {
 	perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, 1, NULL, 0);
 
-	COND_STMT(&perf_task_events, __perf_event_task_sched_out(task, next));
+	COND_STMT(&perf_sched_events, __perf_event_task_sched_out(task, next));
 }
 
 extern void perf_event_mmap(struct vm_area_struct *vma);
init/Kconfig +10 -0
index 8dfd094..fd24181 100644
@@ -680,6 +680,16 @@ config CGROUP_MEM_RES_CTLR_SWAP_ENABLED
 	  select this option (if, for some reason, they need to disable it
 	  then noswapaccount does the trick).
 
+config CGROUP_PERF
+	bool "Enable perf_event per-cpu per-container group (cgroup) monitoring"
+	depends on PERF_EVENTS && CGROUPS
+	help
+	  This option extends the per-cpu mode to restrict monitoring to
+	  threads which belong to the cgroup specificied and run on the
+	  designated cpu.
+
+	  Say N if unsure.
+
 menuconfig CGROUP_SCHED
 	bool "Group CPU scheduler"
 	depends on EXPERIMENTAL
kernel/cgroup.c +23 -0
index 66a416b..1c8bee8 100644
@@ -4790,6 +4790,29 @@ css_get_next(struct cgroup_subsys *ss, int id,
 	return ret;
 }
 
+/*
+ * get corresponding css from file open on cgroupfs directory
+ */
+struct cgroup_subsys_state *cgroup_css_from_dir(struct file *f, int id)
+{
+	struct cgroup *cgrp;
+	struct inode *inode;
+	struct cgroup_subsys_state *css;
+
+	inode = f->f_dentry->d_inode;
+	/* check in cgroup filesystem dir */
+	if (inode->i_op != &cgroup_dir_inode_operations)
+		return ERR_PTR(-EBADF);
+
+	if (id < 0 || id >= CGROUP_SUBSYS_COUNT)
+		return ERR_PTR(-EINVAL);
+
+	/* get cgroup */
+	cgrp = __d_cgrp(f->f_dentry);
+	css = cgrp->subsys[id];
+	return css ? css : ERR_PTR(-ENOENT);
+}
+
 #ifdef CONFIG_CGROUP_DEBUG
 static struct cgroup_subsys_state *debug_create(struct cgroup_subsys *ss,
 						   struct cgroup *cont)
kernel/perf_event.c +546 -40

878 lines not on this page · raw message

Acked-by on v1 4/5
#1 lore raw
Quoted 14 lines · Stephane Eranian

This kernel patch adds the ability to filter monitoring based on container groups (cgroups). This is for use in per-cpu mode only.

The cgroup to monitor is passed as a file descriptor in the pid argument to the syscall. The file descriptor must be opened to the cgroup name in the cgroup filesystem. For instance, if the cgroup name is foo and cgroupfs is mounted in /cgroup, then the file descriptor is opened to /cgroup/foo. Cgroup mode is activated by passing PERF_FLAG_PID_CGROUP in the flags argument to the syscall.

For instance to measure in cgroup foo on CPU1 assuming cgroupfs is mounted under /cgroup:

Quoted 8 lines

struct perf_event_attr attr;
int cgroup_fd, fd;

cgroup_fd = open("/cgroup/foo", O_RDONLY);
fd = perf_event_open(&attr, cgroup_fd, 1, -1, PERF_FLAG_PID_CGROUP); close(cgroup_fd);

Signed-off-by Stephane Eranian <[email protected]>

For the cgroup part:

Acked-by Li Zefan <[email protected]>

a few comments below:

Quoted 1 line

...

init/Kconfig config CGROUP_MEM_RES_CTLR_SWAP_ENABLED · line 685

Quoted 3 lines
init/Kconfig +3 -0
+config CGROUP_PERF
+	bool "Enable perf_event per-cpu per-container group (cgroup) monitoring"
+	depends on PERF_EVENTS && CGROUPS

Depending on CGROUPS is already implicated by "if CGROUPS"

Quoted 10 lines
init/Kconfig +7 -0
+	help
+	  This option extends the per-cpu mode to restrict monitoring to
+	  threads which belong to the cgroup specificied and run on the
+	  designated cpu.
+
+	  Say N if unsure.
+
 menuconfig CGROUP_SCHED
 	bool "Group CPU scheduler"
 	depends on EXPERIMENTAL

...

Quoted 17 lines
kernel/perf_event.c +17 -0
+static inline int perf_cgroup_connect(int fd, struct perf_event *event,
+				      struct perf_event_attr *attr,
+				      struct perf_event *group_leader)
+{
+	struct perf_cgroup *cgrp;
+	struct cgroup_subsys_state *css;
+	struct file *file;
+	int ret = 0, fput_needed;
+
+	file = fget_light(fd, &fput_needed);
+	if (!file)
+		return -EBADF;
+
+	css = cgroup_css_from_dir(file, perf_subsys_id);
+	if (IS_ERR(css))
+		return PTR_ERR(css);
+

kernel/perf_event.c static inline u64 perf_clock(void) · line 346

Quoted 10 lines
kernel/perf_event.c +10 -0
+	cgrp = container_of(css, struct perf_cgroup, css);
+	event->cgrp = cgrp;
+
+	/*
+	 * all events in a group must monitor
+	 * the same cgroup because a thread belongs
+	 * to only one perf cgroup at a time
+	 */
+	if (group_leader && group_leader->cgrp != cgrp) {
+		perf_detach_cgroup(event);

This doesn't seem right, because we haven't got the cgroup refcount.

Quoted 8 lines
kernel/perf_event.c +8 -0
+		ret = -EINVAL;
+	} else {
+		/* must be done before we fput() the file */
+		perf_get_cgroup(event);
+	}
+	fput_light(file, fput_needed);
+	return ret;
+}
author
#2 lore raw

Li,

Thanks for your comment. Good catch on perf_detach_cgroup(). I will resubmit this patch (part 4/5).

Quoted 89 lines · Li Zefan
Quoted 14 lines · Stephane Eranian

This kernel patch adds the ability to filter monitoring based on container groups (cgroups). This is for use in per-cpu mode only.

The cgroup to monitor is passed as a file descriptor in the pid argument to the syscall. The file descriptor must be opened to the cgroup name in the cgroup filesystem. For instance, if the cgroup name is foo and cgroupfs is mounted in /cgroup, then the file descriptor is opened to /cgroup/foo. Cgroup mode is activated by passing PERF_FLAG_PID_CGROUP in the flags argument to the syscall.

For instance to measure in cgroup foo on CPU1 assuming cgroupfs is mounted under /cgroup:

Quoted 8 lines

struct perf_event_attr attr;
int cgroup_fd, fd;

cgroup_fd = open("/cgroup/foo", O_RDONLY);
fd = perf_event_open(&attr, cgroup_fd, 1, -1, PERF_FLAG_PID_CGROUP); close(cgroup_fd);

Signed-off-by Stephane Eranian <[email protected]>

For the cgroup part:

Acked-by Li Zefan <[email protected]>

a few comments below:

Quoted 1 line

...

Quoted 3 lines

+config CGROUP_PERF
+     bool "Enable perf_event per-cpu per-container group (cgroup) monitoring"
+     depends on PERF_EVENTS && CGROUPS

Depending on CGROUPS is already implicated by "if CGROUPS"

Quoted 10 lines

+     help
+       This option extends the per-cpu mode to restrict monitoring to
+       threads which belong to the cgroup specificied and run on the
+       designated cpu.

init/Kconfig +3 -0
+
+       Say N if unsure.
+

 menuconfig CGROUP_SCHED
      bool "Group CPU scheduler"
      depends on EXPERIMENTAL

...

Quoted 17 lines
kernel/perf_event.c +17 -0
+static inline int perf_cgroup_connect(int fd, struct perf_event *event,
+                                   struct perf_event_attr *attr,
+                                   struct perf_event *group_leader)
+{
+     struct perf_cgroup *cgrp;
+     struct cgroup_subsys_state *css;
+     struct file *file;
+     int ret = 0, fput_needed;
+
+     file = fget_light(fd, &fput_needed);
+     if (!file)
+             return -EBADF;
+
+     css = cgroup_css_from_dir(file, perf_subsys_id);
+     if (IS_ERR(css))
+             return PTR_ERR(css);
+

kernel/perf_event.c static inline u64 perf_clock(void) · line 346

Quoted 10 lines
kernel/perf_event.c +10 -0
+     cgrp = container_of(css, struct perf_cgroup, css);
+     event->cgrp = cgrp;
+
+     /*
+      * all events in a group must monitor
+      * the same cgroup because a thread belongs
+      * to only one perf cgroup at a time
+      */
+     if (group_leader && group_leader->cgrp != cgrp) {
+             perf_detach_cgroup(event);

This doesn't seem right, because we haven't got the cgroup refcount.

Quoted 8 lines

+             ret = -EINVAL;
+     } else {
+             /* must be done before we fput() the file */
+             perf_get_cgroup(event);
+     }
+     fput_light(file, fput_needed);
+     return ret;
+}

maintainer
#3 lore raw
In reply to Stephane Eranian

include/linux/perf_event.h struct swevent_hlist { · line 733

Quoted 13 lines · Stephane Eranian
include/linux/perf_event.h +13 -0
+#ifdef CONFIG_CGROUP_PERF
+/*
+ * perf_cgroup_info keeps track of time_enabled for a cgroup.
+ * This is a per-cpu dynamically allocated data structure.
+ */
+struct perf_cgroup_info {
+	u64 time;
+	u64 timestamp;
+};
+
+struct perf_cgroup {
+	struct cgroup_subsys_state css;
+	struct perf_cgroup_info *info;	/* timing info, one per cpu */

I think 'they' want a __percpu annotation there.

Quoted 8 lines
(quoted diff) +2 -0
+};
+#endif
kernel/perf_event.c +0 -0
index b782b7a..905b91a 100644

kernel/perf_event.c static inline u64 perf_clock(void) · line 158

Quoted 10 lines
kernel/perf_event.c +8 -0
 
 
+static inline void __update_cgrp_time(struct perf_cgroup *cgrp)
+{
+	struct perf_cgroup_info *t;
+	u64 now;
+
+	now = perf_clock();
+
+	t = per_cpu_ptr(cgrp->info, smp_processor_id());

  this_cpu_ptr(cgrp->info);
Quoted 7 lines
kernel/perf_event.c +5 -0
+
+	t->time += now - t->timestamp;
+	t->timestamp = now;
+}
 
 
+static inline void

kernel/perf_event.c static inline u64 perf_clock(void) · line 193

Quoted 10 lines
kernel/perf_event.c +10 -0
+perf_cgroup_set_timestamp(struct task_struct *task, u64 now)
+{
+	struct perf_cgroup *cgrp;
+	struct perf_cgroup_info *info;
+
+	if (!task)
+		return;
+
+	cgrp = perf_cgroup_from_task(task);
+	info = per_cpu_ptr(cgrp->info, smp_processor_id());

this_cpu_ptr();

Quoted 13 lines
kernel/perf_event.c +12 -0
+	info->timestamp = now;
+}
+
 
+/*
+ * called from perf_event_ask_sched_out() conditional to jump label
+ */
+void
+perf_cgroup_switch(struct task_struct *task, struct task_struct *next)
+{
+	struct perf_cgroup *cgrp_out = perf_cgroup_from_task(task);
+	struct perf_cgroup *cgrp_in = perf_cgroup_from_task(next);
+	struct perf_cpu_context *cpuctx;

kernel/perf_event.c static inline u64 perf_clock(void) · line 230

Quoted 10 lines
kernel/perf_event.c +10 -0
+	struct pmu *pmu;
+	/*
+	 * if task is DEAD, then css_out is irrelevant, it has
+	 * been changed to init_cgrp in cgroup_exit() from do_exit().
+	 * Furthermore, perf_cgroup_exit_task(), has scheduled out
+	 * all css constrained events, only unconstrained events
+	 * remain. Therefore we need to reschedule based on css_in.
+	 */
+	if (task->state != TASK_DEAD && cgrp_out == cgrp_in)
+		return;

I think that check is broken, TASK_DEAD is set way after calling cgroup_exit(), so if we get preempted in between there you'll still go funny.

We do set PF_EXITING before calling cgroup_exit() though.

Quoted 12 lines
kernel/perf_event.c +12 -0
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(pmu, &pmus, entry) {
+
+		cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
+
+		perf_pmu_disable(cpuctx->ctx.pmu);
+
+		/*
+		 * perf_cgroup_events says at least one
+		 * context on this CPU has cgroup events.
+		 *

kernel/perf_event.c static inline u64 perf_clock(void) · line 253

Quoted 10 lines
kernel/perf_event.c +10 -0
+		 * ctx->nr_cgroups reports the number of cgroup
+		 * events for a context. Given there can be multiple
+		 * PMUs, there can be multiple contexts.
+		 */
+		if (cpuctx->ctx.nr_cgroups > 0) {
+			/*
+			 * schedule out everything we have
+			 * task == DEAD: only unconstrained events
+			 * task != DEAD: css constrained + unconstrained events
+			 *

Does this comment want an update? As per the above (broken) check, we should never get here for DEAD tasks, hmm?

Quoted 8 lines
kernel/perf_event.c +8 -0
+			 * We kick out all events (even if unconstrained)
+			 * to allow the constrained events to be scheduled
+			 * based on their position in the event list (fairness)
+			 */
+			cpu_ctx_sched_out(cpuctx, EVENT_ALL);
+			/*
+			 * reschedule css_in constrained + unconstrained events
+			 */

kernel/perf_event.c static inline u64 perf_clock(void) · line 271

Quoted 10 lines
kernel/perf_event.c +10 -0
+			cpu_ctx_sched_in(cpuctx, EVENT_ALL, next, 1);
+		}
+
+		perf_pmu_enable(cpuctx->ctx.pmu);
+	}
+
+	rcu_read_unlock();
+}
+
+static inline void

Copy/paste fail?

kernel/perf_event.c static inline u64 perf_clock(void) · line 322

Quoted 8 lines
kernel/perf_event.c +7 -0
+perf_cgroup_exit_task(struct task_struct *task)
+{
 
+}
+
+static inline int perf_cgroup_connect(int fd, struct perf_event *event,
+				      struct perf_event_attr *attr,
+				      struct perf_event *group_leader)

Again, do we really need this 'inline' ?

Quoted 6 lines
kernel/perf_event.c +6 -0
+{
+	struct perf_cgroup *cgrp;
+	struct cgroup_subsys_state *css;
+	struct file *file;
+	int ret = 0, fput_needed;
+

kernel/perf_event.c static inline u64 perf_clock(void) · line 338

Quoted 10 lines
kernel/perf_event.c +10 -0
+	file = fget_light(fd, &fput_needed);
+	if (!file)
+		return -EBADF;
+
+	css = cgroup_css_from_dir(file, perf_subsys_id);
+	if (IS_ERR(css))
+		return PTR_ERR(css);
+
+	cgrp = container_of(css, struct perf_cgroup, css);
+	event->cgrp = cgrp;

If we do that perf_get_cgroup() here (unconditional).

kernel/perf_event.c static inline u64 perf_clock(void) · line 351

Quoted 12 lines
kernel/perf_event.c +12 -0
+	/*
+	 * all events in a group must monitor
+	 * the same cgroup because a thread belongs
+	 * to only one perf cgroup at a time
+	 */
+	if (group_leader && group_leader->cgrp != cgrp) {
+		perf_detach_cgroup(event);
+		ret = -EINVAL;
+	} else {
+		/* must be done before we fput() the file */
+		perf_get_cgroup(event);
+	}

Then you can have that conditional perf_detach_cgroup() here, right?

Quoted 3 lines
kernel/perf_event.c +3 -0
+	fput_light(file, fput_needed);
+	return ret;
+}
author
#4 lore raw
Quoted 18 lines · Peter Zijlstra

include/linux/perf_event.h struct swevent_hlist { · line 733

Quoted 13 lines · Stephane Eranian
include/linux/perf_event.h +13 -0
+#ifdef CONFIG_CGROUP_PERF
+/*
+ * perf_cgroup_info keeps track of time_enabled for a cgroup.
+ * This is a per-cpu dynamically allocated data structure.
+ */
+struct perf_cgroup_info {
+     u64 time;
+     u64 timestamp;
+};
+
+struct perf_cgroup {
+     struct cgroup_subsys_state css;
+     struct perf_cgroup_info *info;  /* timing info, one per cpu */

I think 'they' want a __percpu annotation there.

Ok, I can fix this.

Quoted 10 lines
Quoted 8 lines
(quoted diff) +2 -0
+};
+#endif
kernel/perf_event.c +0 -0
index b782b7a..905b91a 100644
Quoted 11 lines

kernel/perf_event.c static inline u64 perf_clock(void) · line 158

Quoted 8 lines
kernel/perf_event.c +8 -0
+static inline void __update_cgrp_time(struct perf_cgroup *cgrp)
+{
+     struct perf_cgroup_info *t;
+     u64 now;
+
+     now = perf_clock();
+
+     t = per_cpu_ptr(cgrp->info, smp_processor_id());

 this_cpu_ptr(cgrp->info);

Fixed.

Quoted 6 lines
Quoted 4 lines
kernel/perf_event.c +4 -0
+
+     t->time += now - t->timestamp;
+     t->timestamp = now;
+}
Quoted 14 lines

kernel/perf_event.c static inline u64 perf_clock(void) · line 193

Quoted 11 lines
kernel/perf_event.c +11 -0
+static inline void
+perf_cgroup_set_timestamp(struct task_struct *task, u64 now)
+{
+     struct perf_cgroup *cgrp;
+     struct perf_cgroup_info *info;
+
+     if (!task)
+             return;
+
+     cgrp = perf_cgroup_from_task(task);
+     info = per_cpu_ptr(cgrp->info, smp_processor_id());

 this_cpu_ptr();

Fixed.

Quoted 28 lines
Quoted 13 lines

+     info->timestamp = now;

kernel/perf_event.c +11 -0
+}
+
 
+/*
+ * called from perf_event_ask_sched_out() conditional to jump label
+ */
+void
+perf_cgroup_switch(struct task_struct *task, struct task_struct *next)
+{
+     struct perf_cgroup *cgrp_out = perf_cgroup_from_task(task);
+     struct perf_cgroup *cgrp_in = perf_cgroup_from_task(next);
+     struct perf_cpu_context *cpuctx;

kernel/perf_event.c static inline u64 perf_clock(void) · line 230

Quoted 10 lines
kernel/perf_event.c +10 -0
+     struct pmu *pmu;
+     /*
+      * if task is DEAD, then css_out is irrelevant, it has
+      * been changed to init_cgrp in cgroup_exit() from do_exit().
+      * Furthermore, perf_cgroup_exit_task(), has scheduled out
+      * all css constrained events, only unconstrained events
+      * remain. Therefore we need to reschedule based on css_in.
+      */
+     if (task->state != TASK_DEAD && cgrp_out == cgrp_in)
+             return;

I think that check is broken, TASK_DEAD is set way after calling cgroup_exit(), so if we get preempted in between there you'll still go funny.

Quoted 2 lines

We do set PF_EXITING before calling cgroup_exit() though.

Switched to PF_EXITING. Thanks for catching this one.

Quoted 17 lines
Quoted 17 lines

+     rcu_read_lock();

kernel/perf_event.c +16 -0
+
+     list_for_each_entry_rcu(pmu, &pmus, entry) {
+
+             cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
+
+             perf_pmu_disable(cpuctx->ctx.pmu);
+
+             /*
+              * perf_cgroup_events says at least one
+              * context on this CPU has cgroup events.
+              *
+              * ctx->nr_cgroups reports the number of cgroup
+              * events for a context. Given there can be multiple
+              * PMUs, there can be multiple contexts.
+              */
+             if (cpuctx->ctx.nr_cgroups > 0) {
Quoted 9 lines
Quoted 5 lines

+                     /*
+                      * schedule out everything we have
+                      * task == DEAD: only unconstrained events
+                      * task != DEAD: css constrained + unconstrained events
+                      *

Does this comment want an update? As per the above (broken) check, we should never get here for DEAD tasks, hmm?

Updated.

Quoted 21 lines
Quoted 10 lines

+                      * We kick out all events (even if unconstrained)
+                      * to allow the constrained events to be scheduled
+                      * based on their position in the event list (fairness)
+                      */
+                     cpu_ctx_sched_out(cpuctx, EVENT_ALL);
+                     /*
+                      * reschedule css_in constrained + unconstrained events
+                      */
+                     cpu_ctx_sched_in(cpuctx, EVENT_ALL, next, 1);
+             }

kernel/perf_event.c static inline u64 perf_clock(void) · line 271

Quoted 8 lines
kernel/perf_event.c +8 -0
+
+             perf_pmu_enable(cpuctx->ctx.pmu);
+     }
+
+     rcu_read_unlock();
+}
+
+static inline void

Copy/paste fail?

Quoted 5 lines

kernel/perf_event.c static inline u64 perf_clock(void) · line 312

Quoted 5 lines
kernel/perf_event.c +4 -0
+perf_cgroup_exit_task(struct task_struct *task)
+{
 
+}
+

Dropped this one which is useless.

Quoted 7 lines

+static inline int perf_cgroup_connect(int fd, struct perf_event *event,

Quoted 2 lines

+                                   struct perf_event_attr *attr,
+                                   struct perf_event *group_leader)

Again, do we really need this 'inline' ?

Not really.

Quoted 11 lines
Quoted 11 lines
kernel/perf_event.c +11 -0
+{
+     struct perf_cgroup *cgrp;
+     struct cgroup_subsys_state *css;
+     struct file *file;
+     int ret = 0, fput_needed;
+
+     file = fget_light(fd, &fput_needed);
+     if (!file)
+             return -EBADF;
+
+     css = cgroup_css_from_dir(file, perf_subsys_id);
Quoted 9 lines
Quoted 5 lines

+     if (IS_ERR(css))
+             return PTR_ERR(css);
+
+     cgrp = container_of(css, struct perf_cgroup, css);
+     event->cgrp = cgrp;

If we do that perf_get_cgroup() here (unconditional).

That is exactly what I did in my revised patch.

Quoted 13 lines
Quoted 4 lines

+     /*
+      * all events in a group must monitor
+      * the same cgroup because a thread belongs
+      * to only one perf cgroup at a time

Quoted 8 lines

+      */
+     if (group_leader && group_leader->cgrp != cgrp) {
+             perf_detach_cgroup(event);
+             ret = -EINVAL;
+     } else {
+             /* must be done before we fput() the file */
+             perf_get_cgroup(event);
+     }

Quoted 1 line

Then you can have that conditional perf_detach_cgroup() here, right?

Thanks.
Will resubmit just this patch.

author
#5 lore raw

Peter,

Quoted 15 lines · Stephane Eranian
Quoted 14 lines · Peter Zijlstra
Quoted 13 lines · Stephane Eranian
kernel/perf_event.c +13 -0
+void
+perf_cgroup_switch(struct task_struct *task, struct task_struct *next)
+{
+     struct perf_cgroup *cgrp_out = perf_cgroup_from_task(task);
+     struct perf_cgroup *cgrp_in = perf_cgroup_from_task(next);
+     struct perf_cpu_context *cpuctx;
+     struct pmu *pmu;
+     /*
+      * if task is DEAD, then css_out is irrelevant, it has
+      * been changed to init_cgrp in cgroup_exit() from do_exit().
+      * Furthermore, perf_cgroup_exit_task(), has scheduled out
+      * all css constrained events, only unconstrained events
+      * remain. Therefore we need to reschedule based on css_in.
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

+      */
+     if (task->state != TASK_DEAD && cgrp_out == cgrp_in)
+             return;

I think that check is broken, TASK_DEAD is set way after calling cgroup_exit(), so if we get preempted in between there you'll still go funny.

I looked at this part again.

The original code checking for TASK_DEAD is correct.

The reason is simple, you're looking at perf_cgroup_switch() which is invoked as part of schedule() and NOT perf_event_task_exit() (called prior to cgroup_exit()).
Thus, by the time you do the final schedule(), the task state has indeed been switched to TASK_DEAD.

I remember testing for this condition during the debug phase.

I will resubmit with the other smaller cleanups.

maintainer
#6 lore raw
Quoted 31 lines · Stephane Eranian

Peter,

Quoted 15 lines · Stephane Eranian
Quoted 14 lines · Peter Zijlstra
Quoted 13 lines · Stephane Eranian
kernel/perf_event.c +13 -0
+void
+perf_cgroup_switch(struct task_struct *task, struct task_struct *next)
+{
+     struct perf_cgroup *cgrp_out = perf_cgroup_from_task(task);
+     struct perf_cgroup *cgrp_in = perf_cgroup_from_task(next);
+     struct perf_cpu_context *cpuctx;
+     struct pmu *pmu;
+     /*
+      * if task is DEAD, then css_out is irrelevant, it has
+      * been changed to init_cgrp in cgroup_exit() from do_exit().
+      * Furthermore, perf_cgroup_exit_task(), has scheduled out
+      * all css constrained events, only unconstrained events
+      * remain. Therefore we need to reschedule based on css_in.
Quoted 8 lines
Quoted 8 lines

kernel/perf_event.c static inline u64 perf_clock(void) · line 230

Quoted 3 lines
kernel/perf_event.c +3 -0
+      */
+     if (task->state != TASK_DEAD && cgrp_out == cgrp_in)
+             return;

I think that check is broken, TASK_DEAD is set way after calling cgroup_exit(), so if we get preempted in between there you'll still go funny.

I looked at this part again.

The original code checking for TASK_DEAD is correct.

Quoted 7 lines

The reason is simple, you're looking at perf_cgroup_switch() which is invoked as part of schedule() and NOT perf_event_task_exit() (called prior to cgroup_exit()).
Thus, by the time you do the final schedule(), the task state has indeed been switched to TASK_DEAD.

I remember testing for this condition during the debug phase.

But, cgroup_exit() detaches the task from the cgroup, after which the cgroup can disappear. Furthermore, we can schedule after cgroup_exit() and before the explicit schedule() invocation.

Some of the exit functions (say proc_exit_connector) can block and cause scheduling, and with PREEMPT=y we can get preempted.

This means you'll be context switching, and thus possibly calling perf_cgroup_switch(), on a task who's cgroup is possibly destroyed.

So I'm not at all seeing how this is correct.

author
#7 lore raw

Peter,

There is indeed a problem in the exit path. We cannot use TASK_DEAD. Using task->flags & PF_EXITING would work, but there is one last gotcha.

We need to have the sequence:

        perf_event_exit_task(tsk);
        cgroup_exit(tsk, 1);

Be atomic w.r.t., IPI. I am running into timing problem (time_ena, time_run) if you
have an IPI to read the counts between perf_event_exit_task() and cgroup_exit(). The issue is that during that time window, tsk->cgrp == current->cgrp, thus the timing code assumes cgroup monitoring is still on and it updates the cgroup time (time_enabled).

I am working on a solution that does not involve interrupt masking for those two calls.

Quoted 52 lines · Peter Zijlstra
Quoted 31 lines · Stephane Eranian

Peter,

Quoted 15 lines · Stephane Eranian
Quoted 14 lines · Peter Zijlstra
Quoted 13 lines · Stephane Eranian
kernel/perf_event.c +13 -0
+void
+perf_cgroup_switch(struct task_struct *task, struct task_struct *next)
+{
+     struct perf_cgroup *cgrp_out = perf_cgroup_from_task(task);
+     struct perf_cgroup *cgrp_in = perf_cgroup_from_task(next);
+     struct perf_cpu_context *cpuctx;
+     struct pmu *pmu;
+     /*
+      * if task is DEAD, then css_out is irrelevant, it has
+      * been changed to init_cgrp in cgroup_exit() from do_exit().
+      * Furthermore, perf_cgroup_exit_task(), has scheduled out
+      * all css constrained events, only unconstrained events
+      * remain. Therefore we need to reschedule based on css_in.
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

+      */
+     if (task->state != TASK_DEAD && cgrp_out == cgrp_in)
+             return;

I think that check is broken, TASK_DEAD is set way after calling cgroup_exit(), so if we get preempted in between there you'll still go funny.

I looked at this part again.

The original code checking for TASK_DEAD is correct.

Quoted 7 lines

The reason is simple, you're looking at perf_cgroup_switch() which is invoked as part of schedule() and NOT perf_event_task_exit() (called prior to cgroup_exit()).
Thus, by the time you do the final schedule(), the task state has indeed been switched to TASK_DEAD.

I remember testing for this condition during the debug phase.

But, cgroup_exit() detaches the task from the cgroup, after which the cgroup can disappear. Furthermore, we can schedule after cgroup_exit() and before the explicit schedule() invocation.

Some of the exit functions (say proc_exit_connector) can block and cause scheduling, and with PREEMPT=y we can get preempted.

This means you'll be context switching, and thus possibly calling perf_cgroup_switch(), on a task who's cgroup is possibly destroyed.

So I'm not at all seeing how this is correct.