Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310060 > unrolled thread

[PATCH] perf: Cleanup user's child events

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf: Cleanup user's child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-15 12:30 +0100
    Re: [PATCH] perf: Cleanup user's child events Peter Zijlstra <peterz@infradead.org> - 2016-01-15 14:00 +0100
      Re: [PATCH] perf: Cleanup user's child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-15 14:10 +0100
        Re: [PATCH] perf: Cleanup user's child events Peter Zijlstra <peterz@infradead.org> - 2016-01-15 14:10 +0100
          [PATCH] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-15 15:10 +0100
            Re: [PATCH] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-15 19:00 +0100
              Re: [PATCH] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-18 13:20 +0100
              Re: [PATCH] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-18 13:40 +0100
                Re: [PATCH] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-18 15:50 +0100
                  [PATCH v2] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-19 16:20 +0100
                    Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-19 21:10 +0100
                    Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-19 21:10 +0100
                      Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-19 23:00 +0100
                        Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-20 09:40 +0100
                          Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-21 06:00 +0100
                      Re: [PATCH v2] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-20 08:10 +0100
                        Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-20 09:10 +0100
                      Re: [PATCH v2] perf: Synchronously cleanup child events Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-01-22 12:40 +0100
                        Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-22 13:20 +0100
                        Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-22 13:40 +0100
                          Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-22 20:50 +0100
                            Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-25 12:50 +0100
                              Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-25 16:00 +0100
                                Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-25 22:10 +0100
                                  Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-26 06:10 +0100
                                    Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-26 17:20 +0100
                                      Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-26 18:30 +0100
                                        Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-27 00:40 +0100
                                          Re: [PATCH v2] perf: Synchronously cleanup child events Peter Zijlstra <peterz@infradead.org> - 2016-01-27 11:00 +0100
                                            Re: [PATCH v2] perf: Synchronously cleanup child events Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-27 19:00 +0100
            Re: [PATCH] perf: Synchronously cleanup child events Ingo Molnar <mingo@kernel.org> - 2016-01-19 08:50 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1315239 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote:
> On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote:
> > Peter Zijlstra <peterz@infradead.org> writes:
> > 
> > > So I think there's a number of problems still :-(

I've been looking at how perf_event->owner is handled and couldn't
figure out how you deal with the case of passing perf_event_fd via scm_rights.
It seems one process can open an event, pass it to another process,
but when current process exists owner will still point to dead task,
since refcount > 0.
Which part am I missing?

[toc] | [prev] | [next] | [standalone]

#1316538 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Fri, Jan 22, 2016 at 11:44:03AM -0800, Alexei Starovoitov wrote:
> On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote:
> > On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote:
> > > Peter Zijlstra <peterz@infradead.org> writes:
> > > 
> > > > So I think there's a number of problems still :-(
> 
> I've been looking at how perf_event->owner is handled and couldn't
> figure out how you deal with the case of passing perf_event_fd via scm_rights.
> It seems one process can open an event, pass it to another process,
> but when current process exists owner will still point to dead task,
> since refcount > 0.
> Which part am I missing?

Nothing, you raised a good point. I think this shows we cannot link
!event->owner to an event being 'dead'.

If we keep these two states separate, the scm_rights thing should work
again.

[toc] | [prev] | [next] | [standalone]

#1316788 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Mon, Jan 25, 2016 at 12:48:46PM +0100, Peter Zijlstra wrote:
> On Fri, Jan 22, 2016 at 11:44:03AM -0800, Alexei Starovoitov wrote:
> > On Fri, Jan 22, 2016 at 01:38:47PM +0100, Peter Zijlstra wrote:
> > > On Fri, Jan 22, 2016 at 01:35:40PM +0200, Alexander Shishkin wrote:
> > > > Peter Zijlstra <peterz@infradead.org> writes:
> > > > 
> > > > > So I think there's a number of problems still :-(
> > 
> > I've been looking at how perf_event->owner is handled and couldn't
> > figure out how you deal with the case of passing perf_event_fd via scm_rights.
> > It seems one process can open an event, pass it to another process,
> > but when current process exists owner will still point to dead task,
> > since refcount > 0.
> > Which part am I missing?
> 
> Nothing, you raised a good point. I think this shows we cannot link
> !event->owner to an event being 'dead'.
> 
> If we keep these two states separate, the scm_rights thing should work
> again.

Alexander, Alexei,

How about the below? That uses event->state == PERF_EVENT_STATE_EXIT to
indicate the event has been given up by its 'owner' and decouples us
from the actual event->owner logic.

This retains the event->owner and event->owner_list thing purely for the
prclt(.option = PR_TASK_PERF_EVENTS_{EN,DIS}ABLE) calls, but does give
us strict 'owner' semantics in that:

  struct perf_event *my_event = perf_event_create_kernel_counter();

  /* ... */

  perf_event_release_kernel(my_event);

Or

  int fd = sys_perf_event_open(...);

  close(fd); /* last, calls fops::release */

Will destroy the event dead. event::refcount will 'retain' the object
but it will become non functional and is strictly meant as a temporal
existence guarantee (for when RCU isn't good enough).

So this should restore the scm_rights case, which preserves the fd but
could result in not having event->owner (and therefore being removed
from its owner_list), which is fine.

BPF still needs to get fixed to use filedesc references instead.

---

--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -634,9 +634,6 @@ struct perf_event_context {
 	int				nr_cgroups;	 /* cgroup evts */
 	void				*task_ctx_data; /* pmu specific data */
 	struct rcu_head			rcu_head;
-
-	struct delayed_work		orphans_remove;
-	bool				orphans_remove_sched;
 };
 
 /*
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -49,8 +49,6 @@
 
 #include <asm/irq_regs.h>
 
-static struct workqueue_struct *perf_wq;
-
 typedef int (*remote_function_f)(void *);
 
 struct remote_function_call {
@@ -1086,8 +1084,8 @@ static void put_ctx(struct perf_event_co
  * Lock order:
  *	task_struct::perf_event_mutex
  *	  perf_event_context::mutex
- *	    perf_event_context::lock
  *	    perf_event::child_mutex;
+ *	      perf_event_context::lock
  *	    perf_event::mmap_mutex
  *	    mmap_sem
  */
@@ -1646,45 +1644,11 @@ static void perf_group_detach(struct per
 		perf_event__header_size(tmp);
 }
 
-/*
- * User event without the task.
- */
 static bool is_orphaned_event(struct perf_event *event)
 {
-	return event && !is_kernel_event(event) && !event->owner;
-}
-
-/*
- * Event has a parent but parent's task finished and it's
- * alive only because of children holding refference.
- */
-static bool is_orphaned_child(struct perf_event *event)
-{
-	return is_orphaned_event(event->parent);
-}
-
-static void orphans_remove_work(struct work_struct *work);
-
-static void schedule_orphans_remove(struct perf_event_context *ctx)
-{
-	if (!ctx->task || ctx->orphans_remove_sched || !perf_wq)
-		return;
-
-	if (queue_delayed_work(perf_wq, &ctx->orphans_remove, 1)) {
-		get_ctx(ctx);
-		ctx->orphans_remove_sched = true;
-	}
+	return event->state == PERF_EVENT_STATE_EXIT;
 }
 
-static int __init perf_workqueue_init(void)
-{
-	perf_wq = create_singlethread_workqueue("perf");
-	WARN(!perf_wq, "failed to create perf workqueue\n");
-	return perf_wq ? 0 : -1;
-}
-
-core_initcall(perf_workqueue_init);
-
 static inline int pmu_filter_match(struct perf_event *event)
 {
 	struct pmu *pmu = event->pmu;
@@ -1745,9 +1709,6 @@ event_sched_out(struct perf_event *event
 	if (event->attr.exclusive || !cpuctx->active_oncpu)
 		cpuctx->exclusive = 0;
 
-	if (is_orphaned_child(event))
-		schedule_orphans_remove(ctx);
-
 	perf_pmu_enable(event->pmu);
 }
 
@@ -1771,6 +1732,9 @@ group_sched_out(struct perf_event *group
 		cpuctx->exclusive = 0;
 }
 
+#define DETACH_GROUP	0x01
+#define DETACH_STATE	0x02
+
 /*
  * Cross CPU call to remove a performance event
  *
@@ -1783,13 +1747,19 @@ __perf_remove_from_context(struct perf_e
 			   struct perf_event_context *ctx,
 			   void *info)
 {
-	bool detach_group = (unsigned long)info;
+	unsigned long flags = (unsigned long)info;
 
 	event_sched_out(event, cpuctx, ctx);
-	if (detach_group)
+
+	if (flags & DETACH_GROUP)
 		perf_group_detach(event);
 	list_del_event(event, ctx);
 
+	if (flags & DETACH_STATE) {
+		event->state = PERF_EVENT_STATE_EXIT;
+		perf_event_wakeup(event);
+	}
+
 	if (!ctx->nr_events && ctx->is_active) {
 		ctx->is_active = 0;
 		if (ctx->task) {
@@ -1809,12 +1779,11 @@ __perf_remove_from_context(struct perf_e
  * When called from perf_event_exit_task, it's OK because the
  * context has been detached from its task.
  */
-static void perf_remove_from_context(struct perf_event *event, bool detach_group)
+static void perf_remove_from_context(struct perf_event *event, unsigned long flags)
 {
 	lockdep_assert_held(&event->ctx->mutex);
 
-	event_function_call(event, __perf_remove_from_context,
-			    (void *)(unsigned long)detach_group);
+	event_function_call(event, __perf_remove_from_context, (void *)flags);
 }
 
 /*
@@ -1980,9 +1949,6 @@ event_sched_in(struct perf_event *event,
 	if (event->attr.exclusive)
 		cpuctx->exclusive = 1;
 
-	if (is_orphaned_child(event))
-		schedule_orphans_remove(ctx);
-
 out:
 	perf_pmu_enable(event->pmu);
 
@@ -2253,7 +2219,8 @@ static void __perf_event_enable(struct p
 	struct perf_event *leader = event->group_leader;
 	struct perf_event_context *task_ctx;
 
-	if (event->state >= PERF_EVENT_STATE_INACTIVE)
+	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
+	    event->state <= PERF_EVENT_STATE_ERROR)
 		return;
 
 	update_context_time(ctx);
@@ -2298,7 +2265,8 @@ static void _perf_event_enable(struct pe
 	struct perf_event_context *ctx = event->ctx;
 
 	raw_spin_lock_irq(&ctx->lock);
-	if (event->state >= PERF_EVENT_STATE_INACTIVE) {
+	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
+	    event->state < PERF_EVENT_STATE_ERROR) {
 		raw_spin_unlock_irq(&ctx->lock);
 		return;
 	}
@@ -3363,7 +3331,6 @@ static void __perf_event_init_context(st
 	INIT_LIST_HEAD(&ctx->flexible_groups);
 	INIT_LIST_HEAD(&ctx->event_list);
 	atomic_set(&ctx->refcount, 1);
-	INIT_DELAYED_WORK(&ctx->orphans_remove, orphans_remove_work);
 }
 
 static struct perf_event_context *
@@ -3665,29 +3632,6 @@ static bool exclusive_event_installable(
 	return true;
 }
 
-static void __free_event(struct perf_event *event)
-{
-	if (!event->parent) {
-		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
-			put_callchain_buffers();
-	}
-
-	perf_event_free_bpf_prog(event);
-
-	if (event->destroy)
-		event->destroy(event);
-
-	if (event->ctx)
-		put_ctx(event->ctx);
-
-	if (event->pmu) {
-		exclusive_event_destroy(event);
-		module_put(event->pmu->module);
-	}
-
-	call_rcu(&event->rcu_head, free_event_rcu);
-}
-
 static void _free_event(struct perf_event *event)
 {
 	irq_work_sync(&event->pending);
@@ -3709,7 +3653,25 @@ static void _free_event(struct perf_even
 	if (is_cgroup_event(event))
 		perf_detach_cgroup(event);
 
-	__free_event(event);
+	if (!event->parent) {
+		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
+			put_callchain_buffers();
+	}
+
+	perf_event_free_bpf_prog(event);
+
+	if (event->destroy)
+		event->destroy(event);
+
+	if (event->ctx)
+		put_ctx(event->ctx);
+
+	if (event->pmu) {
+		exclusive_event_destroy(event);
+		module_put(event->pmu->module);
+	}
+
+	call_rcu(&event->rcu_head, free_event_rcu);
 }
 
 /*
@@ -3771,8 +3733,10 @@ static void perf_remove_from_owner(struc
 		 * ensured they're done, and we can proceed with freeing the
 		 * event.
 		 */
-		if (event->owner)
+		if (event->owner) {
 			list_del_init(&event->owner_entry);
+			event->owner = NULL;
+		}
 		mutex_unlock(&owner->perf_event_mutex);
 		put_task_struct(owner);
 	}
@@ -3780,11 +3744,23 @@ static void perf_remove_from_owner(struc
 
 static void put_event(struct perf_event *event)
 {
-	struct perf_event_context *ctx;
-
 	if (!atomic_long_dec_and_test(&event->refcount))
 		return;
 
+	_free_event(event);
+}
+
+/*
+ * Kill an event dead; while event:refcount will preserve the event
+ * object, it will not preserve its functionality. Once the last 'user'
+ * gives up the object, we'll destroy the thing.
+ */
+int perf_event_release_kernel(struct perf_event *event)
+{
+	struct perf_event_context *ctx;
+	struct perf_event *child, *tmp;
+	LIST_HEAD(child_list);
+
 	if (!is_kernel_event(event))
 		perf_remove_from_owner(event);
 
@@ -3802,14 +3778,57 @@ static void put_event(struct perf_event
 	 */
 	ctx = perf_event_ctx_lock_nested(event, SINGLE_DEPTH_NESTING);
 	WARN_ON_ONCE(ctx->parent_ctx);
-	perf_remove_from_context(event, true);
+	perf_remove_from_context(event, DETACH_GROUP | DETACH_STATE);
 	perf_event_ctx_unlock(event, ctx);
 
-	_free_event(event);
-}
+	/*
+	 * At this point we must have event->state == PERF_EVENT_STATE_EXIT,
+	 * either from the above perf_remove_from_context() or through
+	 * __perf_event_exit_task().
+	 *
+	 * Therefore, anybody acquireing event->child_mutex after the below
+	 * list_splice_init() _must_ also see this, most importantly
+	 * inherit_event() which will avoid placing more children on the
+	 * list.
+	 *
+	 * Thus this guarantees that we will in fact observe and kill _ALL_
+	 * child events.
+	 */
+	WARN_ON_ONCE(event->state != PERF_EVENT_STATE_EXIT);
 
-int perf_event_release_kernel(struct perf_event *event)
-{
+	/*
+	 * event::child_mutex nests inside ctx::mutex, so move children
+	 * to a safe place first and avoid inversion
+	 */
+	mutex_lock(&event->child_mutex);
+	list_splice_init(&event->child_list, &child_list);
+	mutex_unlock(&event->child_mutex);
+
+	list_for_each_entry_safe(child, tmp, &child_list, child_list) {
+		struct perf_event_context *ctx;
+
+		/*
+		 * This is somewhat similar to perf_free_event(),
+		 * except for these events are alive and need
+		 * proper perf_remove_from_context().
+		 */
+		ctx = perf_event_ctx_lock(child);
+		perf_remove_from_context(child, DETACH_GROUP);
+		perf_event_ctx_unlock(child, ctx);
+
+		list_del(&child->child_list);
+
+		/* Children will have exactly one reference */
+		free_event(child);
+
+		/*
+		 * This matches the refcount bump in inherit_event();
+		 * this can't be the last reference.
+		 */
+		put_event(event);
+	}
+
+	/* Must be the last reference */
 	put_event(event);
 	return 0;
 }
@@ -3820,46 +3839,10 @@ EXPORT_SYMBOL_GPL(perf_event_release_ker
  */
 static int perf_release(struct inode *inode, struct file *file)
 {
-	put_event(file->private_data);
+	perf_event_release_kernel(file->private_data);
 	return 0;
 }
 
-/*
- * Remove all orphanes events from the context.
- */
-static void orphans_remove_work(struct work_struct *work)
-{
-	struct perf_event_context *ctx;
-	struct perf_event *event, *tmp;
-
-	ctx = container_of(work, struct perf_event_context,
-			   orphans_remove.work);
-
-	mutex_lock(&ctx->mutex);
-	list_for_each_entry_safe(event, tmp, &ctx->event_list, event_entry) {
-		struct perf_event *parent_event = event->parent;
-
-		if (!is_orphaned_child(event))
-			continue;
-
-		perf_remove_from_context(event, true);
-
-		mutex_lock(&parent_event->child_mutex);
-		list_del_init(&event->child_list);
-		mutex_unlock(&parent_event->child_mutex);
-
-		free_event(event);
-		put_event(parent_event);
-	}
-
-	raw_spin_lock_irq(&ctx->lock);
-	ctx->orphans_remove_sched = false;
-	raw_spin_unlock_irq(&ctx->lock);
-	mutex_unlock(&ctx->mutex);
-
-	put_ctx(ctx);
-}
-
 u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
 {
 	struct perf_event *child;
@@ -8432,11 +8415,11 @@ SYSCALL_DEFINE5(perf_event_open,
 		 * See perf_event_ctx_lock() for comments on the details
 		 * of swizzling perf_event::ctx.
 		 */
-		perf_remove_from_context(group_leader, false);
+		perf_remove_from_context(group_leader, 0);
 
 		list_for_each_entry(sibling, &group_leader->sibling_list,
 				    group_entry) {
-			perf_remove_from_context(sibling, false);
+			perf_remove_from_context(sibling, 0);
 			put_ctx(gctx);
 		}
 
@@ -8616,7 +8599,7 @@ void perf_pmu_migrate_context(struct pmu
 	mutex_lock_double(&src_ctx->mutex, &dst_ctx->mutex);
 	list_for_each_entry_safe(event, tmp, &src_ctx->event_list,
 				 event_entry) {
-		perf_remove_from_context(event, false);
+		perf_remove_from_context(event, 0);
 		unaccount_event_cpu(event, src_cpu);
 		put_ctx(src_ctx);
 		list_add(&event->migrate_entry, &events);
@@ -8665,7 +8648,7 @@ void perf_pmu_migrate_context(struct pmu
 EXPORT_SYMBOL_GPL(perf_pmu_migrate_context);
 
 static void sync_child_event(struct perf_event *child_event,
-			       struct task_struct *child)
+			     struct task_struct *child)
 {
 	struct perf_event *parent_event = child_event->parent;
 	u64 child_val;
@@ -8684,25 +8667,6 @@ static void sync_child_event(struct perf
 	atomic64_add(child_event->total_time_running,
 		     &parent_event->child_total_time_running);
 
-	/*
-	 * Remove this event from the parent's list
-	 */
-	WARN_ON_ONCE(parent_event->ctx->parent_ctx);
-	mutex_lock(&parent_event->child_mutex);
-	list_del_init(&child_event->child_list);
-	mutex_unlock(&parent_event->child_mutex);
-
-	/*
-	 * Make sure user/parent get notified, that we just
-	 * lost one event.
-	 */
-	perf_event_wakeup(parent_event);
-
-	/*
-	 * Release the parent event, if this was the last
-	 * reference to it.
-	 */
-	put_event(parent_event);
 }
 
 static void
@@ -8710,6 +8674,8 @@ __perf_event_exit_task(struct perf_event
 			 struct perf_event_context *child_ctx,
 			 struct task_struct *child)
 {
+	struct perf_event *parent_event = child_event->parent;
+
 	/*
 	 * Do not destroy the 'original' grouping; because of the context
 	 * switch optimization the original events could've ended up in a
@@ -8728,20 +8694,39 @@ __perf_event_exit_task(struct perf_event
 	if (!!child_event->parent)
 		perf_group_detach(child_event);
 	list_del_event(child_event, child_ctx);
+	child_event->state = PERF_EVENT_STATE_EXIT; /* see perf_event_release_kernel() */
 	raw_spin_unlock_irq(&child_ctx->lock);
 
 	/*
-	 * It can happen that the parent exits first, and has events
-	 * that are still around due to the child reference. These
-	 * events need to be zapped.
+	 * Parent events are governed by their filedesc, retain them.
 	 */
-	if (child_event->parent) {
-		sync_child_event(child_event, child);
-		free_event(child_event);
-	} else {
-		child_event->state = PERF_EVENT_STATE_EXIT;
+	if (!child_event->parent) {
 		perf_event_wakeup(child_event);
+		return;
 	}
+	/*
+	 * This is a child event, cleanup and free.
+	 */
+
+	/*
+	 * Fold delta back into parent counts.
+	 */
+	sync_child_event(child_event, child);
+
+	/*
+	 * Remove this event from the parent's list.
+	 */
+	WARN_ON_ONCE(parent_event->ctx->parent_ctx);
+	mutex_lock(&parent_event->child_mutex);
+	list_del_init(&child_event->child_list);
+	mutex_unlock(&parent_event->child_mutex);
+
+	/*
+	 * Kick perf_poll for is_event_hup().
+	 */
+	perf_event_wakeup(parent_event);
+	free_event(child_event);
+	put_event(parent_event);
 }
 
 static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
@@ -8973,8 +8958,15 @@ inherit_event(struct perf_event *parent_
 	if (IS_ERR(child_event))
 		return child_event;
 
+	/*
+	 * is_orphaned_event() and list_add_tail(&parent_event->child_list)
+	 * must be under the same lock in order to serialize against
+	 * perf_event_release_kernel().
+	 */
+	mutex_lock(&parent_event->child_mutex);
 	if (is_orphaned_event(parent_event) ||
 	    !atomic_long_inc_not_zero(&parent_event->refcount)) {
+		mutex_unlock(&parent_event->child_mutex);
 		free_event(child_event);
 		return NULL;
 	}
@@ -9022,8 +9014,6 @@ inherit_event(struct perf_event *parent_
 	/*
 	 * Link this into the parent event's child list
 	 */
-	WARN_ON_ONCE(parent_event->ctx->parent_ctx);
-	mutex_lock(&parent_event->child_mutex);
 	list_add_tail(&child_event->child_list, &parent_event->child_list);
 	mutex_unlock(&parent_event->child_mutex);
 

[toc] | [prev] | [next] | [standalone]

#1317301 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Mon, Jan 25, 2016 at 03:54:14PM +0100, Peter Zijlstra wrote:
> Alexander, Alexei,
> 
> How about the below? That uses event->state == PERF_EVENT_STATE_EXIT to
> indicate the event has been given up by its 'owner' and decouples us
> from the actual event->owner logic.
> 
> This retains the event->owner and event->owner_list thing purely for the
> prclt(.option = PR_TASK_PERF_EVENTS_{EN,DIS}ABLE) calls, but does give
> us strict 'owner' semantics in that:
> 
>   struct perf_event *my_event = perf_event_create_kernel_counter();
> 
>   /* ... */
> 
>   perf_event_release_kernel(my_event);
> 
> Or
> 
>   int fd = sys_perf_event_open(...);
> 
>   close(fd); /* last, calls fops::release */
> 
> Will destroy the event dead. event::refcount will 'retain' the object
> but it will become non functional and is strictly meant as a temporal
> existence guarantee (for when RCU isn't good enough).
> 
> So this should restore the scm_rights case, which preserves the fd but
> could result in not having event->owner (and therefore being removed
> from its owner_list), which is fine.
> 
> BPF still needs to get fixed to use filedesc references instead.

Still no BPF, but this one actually 'works', as in it doesn't have the
blatant exit races and has survived a few hours of runtime.

---
 include/linux/perf_event.h |    3 
 kernel/events/core.c       |  304 ++++++++++++++++++++++-----------------------
 2 files changed, 150 insertions(+), 157 deletions(-)

--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -634,9 +634,6 @@ struct perf_event_context {
 	int				nr_cgroups;	 /* cgroup evts */
 	void				*task_ctx_data; /* pmu specific data */
 	struct rcu_head			rcu_head;
-
-	struct delayed_work		orphans_remove;
-	bool				orphans_remove_sched;
 };
 
 /*
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -49,8 +49,6 @@
 
 #include <asm/irq_regs.h>
 
-static struct workqueue_struct *perf_wq;
-
 typedef int (*remote_function_f)(void *);
 
 struct remote_function_call {
@@ -1086,8 +1084,8 @@ static void put_ctx(struct perf_event_co
  * Lock order:
  *	task_struct::perf_event_mutex
  *	  perf_event_context::mutex
- *	    perf_event_context::lock
  *	    perf_event::child_mutex;
+ *	      perf_event_context::lock
  *	    perf_event::mmap_mutex
  *	    mmap_sem
  */
@@ -1646,45 +1644,11 @@ static void perf_group_detach(struct per
 		perf_event__header_size(tmp);
 }
 
-/*
- * User event without the task.
- */
 static bool is_orphaned_event(struct perf_event *event)
 {
-	return event && !is_kernel_event(event) && !event->owner;
-}
-
-/*
- * Event has a parent but parent's task finished and it's
- * alive only because of children holding refference.
- */
-static bool is_orphaned_child(struct perf_event *event)
-{
-	return is_orphaned_event(event->parent);
+	return event->state == PERF_EVENT_STATE_EXIT;
 }
 
-static void orphans_remove_work(struct work_struct *work);
-
-static void schedule_orphans_remove(struct perf_event_context *ctx)
-{
-	if (!ctx->task || ctx->orphans_remove_sched || !perf_wq)
-		return;
-
-	if (queue_delayed_work(perf_wq, &ctx->orphans_remove, 1)) {
-		get_ctx(ctx);
-		ctx->orphans_remove_sched = true;
-	}
-}
-
-static int __init perf_workqueue_init(void)
-{
-	perf_wq = create_singlethread_workqueue("perf");
-	WARN(!perf_wq, "failed to create perf workqueue\n");
-	return perf_wq ? 0 : -1;
-}
-
-core_initcall(perf_workqueue_init);
-
 static inline int pmu_filter_match(struct perf_event *event)
 {
 	struct pmu *pmu = event->pmu;
@@ -1745,9 +1709,6 @@ event_sched_out(struct perf_event *event
 	if (event->attr.exclusive || !cpuctx->active_oncpu)
 		cpuctx->exclusive = 0;
 
-	if (is_orphaned_child(event))
-		schedule_orphans_remove(ctx);
-
 	perf_pmu_enable(event->pmu);
 }
 
@@ -1771,6 +1732,9 @@ group_sched_out(struct perf_event *group
 		cpuctx->exclusive = 0;
 }
 
+#define DETACH_GROUP	0x01
+#define DETACH_STATE	0x02
+
 /*
  * Cross CPU call to remove a performance event
  *
@@ -1783,13 +1747,19 @@ __perf_remove_from_context(struct perf_e
 			   struct perf_event_context *ctx,
 			   void *info)
 {
-	bool detach_group = (unsigned long)info;
+	unsigned long flags = (unsigned long)info;
 
 	event_sched_out(event, cpuctx, ctx);
-	if (detach_group)
+
+	if (flags & DETACH_GROUP)
 		perf_group_detach(event);
 	list_del_event(event, ctx);
 
+	if (flags & DETACH_STATE) {
+		event->state = PERF_EVENT_STATE_EXIT;
+		perf_event_wakeup(event);
+	}
+
 	if (!ctx->nr_events && ctx->is_active) {
 		ctx->is_active = 0;
 		if (ctx->task) {
@@ -1809,12 +1779,11 @@ __perf_remove_from_context(struct perf_e
  * When called from perf_event_exit_task, it's OK because the
  * context has been detached from its task.
  */
-static void perf_remove_from_context(struct perf_event *event, bool detach_group)
+static void perf_remove_from_context(struct perf_event *event, unsigned long flags)
 {
 	lockdep_assert_held(&event->ctx->mutex);
 
-	event_function_call(event, __perf_remove_from_context,
-			    (void *)(unsigned long)detach_group);
+	event_function_call(event, __perf_remove_from_context, (void *)flags);
 }
 
 /*
@@ -1980,9 +1949,6 @@ event_sched_in(struct perf_event *event,
 	if (event->attr.exclusive)
 		cpuctx->exclusive = 1;
 
-	if (is_orphaned_child(event))
-		schedule_orphans_remove(ctx);
-
 out:
 	perf_pmu_enable(event->pmu);
 
@@ -2253,7 +2219,8 @@ static void __perf_event_enable(struct p
 	struct perf_event *leader = event->group_leader;
 	struct perf_event_context *task_ctx;
 
-	if (event->state >= PERF_EVENT_STATE_INACTIVE)
+	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
+	    event->state <= PERF_EVENT_STATE_ERROR)
 		return;
 
 	update_context_time(ctx);
@@ -2298,7 +2265,8 @@ static void _perf_event_enable(struct pe
 	struct perf_event_context *ctx = event->ctx;
 
 	raw_spin_lock_irq(&ctx->lock);
-	if (event->state >= PERF_EVENT_STATE_INACTIVE) {
+	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
+	    event->state < PERF_EVENT_STATE_ERROR) {
 		raw_spin_unlock_irq(&ctx->lock);
 		return;
 	}
@@ -3363,7 +3331,6 @@ static void __perf_event_init_context(st
 	INIT_LIST_HEAD(&ctx->flexible_groups);
 	INIT_LIST_HEAD(&ctx->event_list);
 	atomic_set(&ctx->refcount, 1);
-	INIT_DELAYED_WORK(&ctx->orphans_remove, orphans_remove_work);
 }
 
 static struct perf_event_context *
@@ -3665,29 +3632,6 @@ static bool exclusive_event_installable(
 	return true;
 }
 
-static void __free_event(struct perf_event *event)
-{
-	if (!event->parent) {
-		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
-			put_callchain_buffers();
-	}
-
-	perf_event_free_bpf_prog(event);
-
-	if (event->destroy)
-		event->destroy(event);
-
-	if (event->ctx)
-		put_ctx(event->ctx);
-
-	if (event->pmu) {
-		exclusive_event_destroy(event);
-		module_put(event->pmu->module);
-	}
-
-	call_rcu(&event->rcu_head, free_event_rcu);
-}
-
 static void _free_event(struct perf_event *event)
 {
 	irq_work_sync(&event->pending);
@@ -3709,7 +3653,25 @@ static void _free_event(struct perf_even
 	if (is_cgroup_event(event))
 		perf_detach_cgroup(event);
 
-	__free_event(event);
+	if (!event->parent) {
+		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
+			put_callchain_buffers();
+	}
+
+	perf_event_free_bpf_prog(event);
+
+	if (event->destroy)
+		event->destroy(event);
+
+	if (event->ctx)
+		put_ctx(event->ctx);
+
+	if (event->pmu) {
+		exclusive_event_destroy(event);
+		module_put(event->pmu->module);
+	}
+
+	call_rcu(&event->rcu_head, free_event_rcu);
 }
 
 /*
@@ -3771,8 +3733,10 @@ static void perf_remove_from_owner(struc
 		 * ensured they're done, and we can proceed with freeing the
 		 * event.
 		 */
-		if (event->owner)
+		if (event->owner) {
 			list_del_init(&event->owner_entry);
+			event->owner = NULL;
+		}
 		mutex_unlock(&owner->perf_event_mutex);
 		put_task_struct(owner);
 	}
@@ -3780,11 +3744,23 @@ static void perf_remove_from_owner(struc
 
 static void put_event(struct perf_event *event)
 {
-	struct perf_event_context *ctx;
-
 	if (!atomic_long_dec_and_test(&event->refcount))
 		return;
 
+	_free_event(event);
+}
+
+/*
+ * Kill an event dead; while event:refcount will preserve the event
+ * object, it will not preserve its functionality. Once the last 'user'
+ * gives up the object, we'll destroy the thing.
+ */
+int perf_event_release_kernel(struct perf_event *event)
+{
+	struct perf_event_context *ctx;
+	struct perf_event *child, *tmp;
+	LIST_HEAD(child_list);
+
 	if (!is_kernel_event(event))
 		perf_remove_from_owner(event);
 
@@ -3802,14 +3778,61 @@ static void put_event(struct perf_event
 	 */
 	ctx = perf_event_ctx_lock_nested(event, SINGLE_DEPTH_NESTING);
 	WARN_ON_ONCE(ctx->parent_ctx);
-	perf_remove_from_context(event, true);
+	perf_remove_from_context(event, DETACH_GROUP | DETACH_STATE);
 	perf_event_ctx_unlock(event, ctx);
 
-	_free_event(event);
-}
+	/*
+	 * At this point we must have event->state == PERF_EVENT_STATE_EXIT,
+	 * either from the above perf_remove_from_context() or through
+	 * __perf_event_exit_task().
+	 *
+	 * Therefore, anybody acquireing event->child_mutex after the below
+	 * list_splice_init() _must_ also see this, most importantly
+	 * inherit_event() which will avoid placing more children on the
+	 * list.
+	 *
+	 * Thus this guarantees that we will in fact observe and kill _ALL_
+	 * child events.
+	 */
+	WARN_ON_ONCE(event->state != PERF_EVENT_STATE_EXIT);
 
-int perf_event_release_kernel(struct perf_event *event)
-{
+again:
+	mutex_lock(&event->child_mutex);
+	list_for_each_entry(child, &event->child_list, child_list) {
+
+		ctx = lockless_dereference(child->ctx);
+		/*
+		 * must stay valid, see __perf_event_exit_task() holding
+		 * event->child_mutex.
+		 */
+		get_ctx(ctx);
+
+		mutex_unlock(&event->child_mutex);
+		mutex_lock(&ctx->mutex);
+		mutex_lock(&event->child_mutex);
+
+		tmp = list_first_entry_or_null(&event->child_list,
+					       struct perf_event, child_list);
+
+		if (tmp == child) {
+			perf_remove_from_context(child, DETACH_GROUP);
+			list_del(&child->child_list);
+			free_event(child);
+			/*
+			 * This matches the refcount bump in inherit_event();
+			 * this can't be the last reference.
+			 */
+			put_event(event);
+		}
+
+		mutex_unlock(&event->child_mutex);
+		mutex_unlock(&ctx->mutex);
+		put_ctx(ctx);
+		goto again;
+	}
+	mutex_unlock(&event->child_mutex);
+
+	/* Must be the last reference */
 	put_event(event);
 	return 0;
 }
@@ -3820,46 +3843,10 @@ EXPORT_SYMBOL_GPL(perf_event_release_ker
  */
 static int perf_release(struct inode *inode, struct file *file)
 {
-	put_event(file->private_data);
+	perf_event_release_kernel(file->private_data);
 	return 0;
 }
 
-/*
- * Remove all orphanes events from the context.
- */
-static void orphans_remove_work(struct work_struct *work)
-{
-	struct perf_event_context *ctx;
-	struct perf_event *event, *tmp;
-
-	ctx = container_of(work, struct perf_event_context,
-			   orphans_remove.work);
-
-	mutex_lock(&ctx->mutex);
-	list_for_each_entry_safe(event, tmp, &ctx->event_list, event_entry) {
-		struct perf_event *parent_event = event->parent;
-
-		if (!is_orphaned_child(event))
-			continue;
-
-		perf_remove_from_context(event, true);
-
-		mutex_lock(&parent_event->child_mutex);
-		list_del_init(&event->child_list);
-		mutex_unlock(&parent_event->child_mutex);
-
-		free_event(event);
-		put_event(parent_event);
-	}
-
-	raw_spin_lock_irq(&ctx->lock);
-	ctx->orphans_remove_sched = false;
-	raw_spin_unlock_irq(&ctx->lock);
-	mutex_unlock(&ctx->mutex);
-
-	put_ctx(ctx);
-}
-
 u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
 {
 	struct perf_event *child;
@@ -8432,11 +8419,11 @@ SYSCALL_DEFINE5(perf_event_open,
 		 * See perf_event_ctx_lock() for comments on the details
 		 * of swizzling perf_event::ctx.
 		 */
-		perf_remove_from_context(group_leader, false);
+		perf_remove_from_context(group_leader, 0);
 
 		list_for_each_entry(sibling, &group_leader->sibling_list,
 				    group_entry) {
-			perf_remove_from_context(sibling, false);
+			perf_remove_from_context(sibling, 0);
 			put_ctx(gctx);
 		}
 
@@ -8616,7 +8603,7 @@ void perf_pmu_migrate_context(struct pmu
 	mutex_lock_double(&src_ctx->mutex, &dst_ctx->mutex);
 	list_for_each_entry_safe(event, tmp, &src_ctx->event_list,
 				 event_entry) {
-		perf_remove_from_context(event, false);
+		perf_remove_from_context(event, 0);
 		unaccount_event_cpu(event, src_cpu);
 		put_ctx(src_ctx);
 		list_add(&event->migrate_entry, &events);
@@ -8665,7 +8652,7 @@ void perf_pmu_migrate_context(struct pmu
 EXPORT_SYMBOL_GPL(perf_pmu_migrate_context);
 
 static void sync_child_event(struct perf_event *child_event,
-			       struct task_struct *child)
+			     struct task_struct *child)
 {
 	struct perf_event *parent_event = child_event->parent;
 	u64 child_val;
@@ -8684,25 +8671,6 @@ static void sync_child_event(struct perf
 	atomic64_add(child_event->total_time_running,
 		     &parent_event->child_total_time_running);
 
-	/*
-	 * Remove this event from the parent's list
-	 */
-	WARN_ON_ONCE(parent_event->ctx->parent_ctx);
-	mutex_lock(&parent_event->child_mutex);
-	list_del_init(&child_event->child_list);
-	mutex_unlock(&parent_event->child_mutex);
-
-	/*
-	 * Make sure user/parent get notified, that we just
-	 * lost one event.
-	 */
-	perf_event_wakeup(parent_event);
-
-	/*
-	 * Release the parent event, if this was the last
-	 * reference to it.
-	 */
-	put_event(parent_event);
 }
 
 static void
@@ -8710,6 +8678,11 @@ __perf_event_exit_task(struct perf_event
 			 struct perf_event_context *child_ctx,
 			 struct task_struct *child)
 {
+	struct perf_event *parent_event = child_event->parent;
+
+	if (parent_event)
+		mutex_lock(&parent_event->child_mutex);
+
 	/*
 	 * Do not destroy the 'original' grouping; because of the context
 	 * switch optimization the original events could've ended up in a
@@ -8728,20 +8701,38 @@ __perf_event_exit_task(struct perf_event
 	if (!!child_event->parent)
 		perf_group_detach(child_event);
 	list_del_event(child_event, child_ctx);
+	child_event->state = PERF_EVENT_STATE_EXIT; /* see perf_event_release_kernel() */
 	raw_spin_unlock_irq(&child_ctx->lock);
 
 	/*
-	 * It can happen that the parent exits first, and has events
-	 * that are still around due to the child reference. These
-	 * events need to be zapped.
+	 * Parent events are governed by their filedesc, retain them.
 	 */
-	if (child_event->parent) {
-		sync_child_event(child_event, child);
-		free_event(child_event);
-	} else {
-		child_event->state = PERF_EVENT_STATE_EXIT;
+	if (!child_event->parent) {
 		perf_event_wakeup(child_event);
+		return;
 	}
+	/*
+	 * This is a child event, cleanup and free.
+	 */
+
+	/*
+	 * Fold delta back into parent counts.
+	 */
+	sync_child_event(child_event, child);
+
+	/*
+	 * Remove this event from the parent's list.
+	 */
+	WARN_ON_ONCE(parent_event->ctx->parent_ctx);
+	list_del_init(&child_event->child_list);
+	mutex_unlock(&parent_event->child_mutex);
+
+	/*
+	 * Kick perf_poll for is_event_hup().
+	 */
+	perf_event_wakeup(parent_event);
+	free_event(child_event);
+	put_event(parent_event);
 }
 
 static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
@@ -8973,8 +8964,15 @@ inherit_event(struct perf_event *parent_
 	if (IS_ERR(child_event))
 		return child_event;
 
+	/*
+	 * is_orphaned_event() and list_add_tail(&parent_event->child_list)
+	 * must be under the same lock in order to serialize against
+	 * perf_event_release_kernel().
+	 */
+	mutex_lock(&parent_event->child_mutex);
 	if (is_orphaned_event(parent_event) ||
 	    !atomic_long_inc_not_zero(&parent_event->refcount)) {
+		mutex_unlock(&parent_event->child_mutex);
 		free_event(child_event);
 		return NULL;
 	}
@@ -9022,8 +9020,6 @@ inherit_event(struct perf_event *parent_
 	/*
 	 * Link this into the parent event's child list
 	 */
-	WARN_ON_ONCE(parent_event->ctx->parent_ctx);
-	mutex_lock(&parent_event->child_mutex);
 	list_add_tail(&child_event->child_list, &parent_event->child_list);
 	mutex_unlock(&parent_event->child_mutex);
 

[toc] | [prev] | [next] | [standalone]

#1317540 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Mon, Jan 25, 2016 at 10:04:10PM +0100, Peter Zijlstra wrote:
> On Mon, Jan 25, 2016 at 03:54:14PM +0100, Peter Zijlstra wrote:
> > Alexander, Alexei,
> > 
> > How about the below? That uses event->state == PERF_EVENT_STATE_EXIT to
> > indicate the event has been given up by its 'owner' and decouples us
> > from the actual event->owner logic.
> > 
> > This retains the event->owner and event->owner_list thing purely for the
> > prclt(.option = PR_TASK_PERF_EVENTS_{EN,DIS}ABLE) calls, but does give
> > us strict 'owner' semantics in that:
> > 
> >   struct perf_event *my_event = perf_event_create_kernel_counter();
> > 
> >   /* ... */
> > 
> >   perf_event_release_kernel(my_event);
> > 
> > Or
> > 
> >   int fd = sys_perf_event_open(...);
> > 
> >   close(fd); /* last, calls fops::release */
> > 
> > Will destroy the event dead. event::refcount will 'retain' the object
> > but it will become non functional and is strictly meant as a temporal
> > existence guarantee (for when RCU isn't good enough).
> > 
> > So this should restore the scm_rights case, which preserves the fd but
> > could result in not having event->owner (and therefore being removed
> > from its owner_list), which is fine.
> > 
> > BPF still needs to get fixed to use filedesc references instead.
> 
> Still no BPF, but this one actually 'works', as in it doesn't have the
> blatant exit races and has survived a few hours of runtime.
> 
> ---
>  include/linux/perf_event.h |    3 
>  kernel/events/core.c       |  304 ++++++++++++++++++++++-----------------------
>  2 files changed, 150 insertions(+), 157 deletions(-)

I think I understand what you're trying to do and
the patch looks good to me.
As far as BPF side I did the following...
does it match the model you outlined above?
I did basic testing and it looks fine.

Subject: [PATCH ] perf,bpf: convert perf_event_array to use struct file

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/perf_event.h |  4 ++--
 kernel/bpf/arraymap.c      | 21 +++++++++++----------
 kernel/events/core.c       | 20 ++++++++------------
 kernel/trace/bpf_trace.c   | 14 ++++++++++----
 4 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index f9828a48f16a..df275020fde9 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -729,7 +729,7 @@ extern int perf_event_init_task(struct task_struct *child);
 extern void perf_event_exit_task(struct task_struct *child);
 extern void perf_event_free_task(struct task_struct *task);
 extern void perf_event_delayed_put(struct task_struct *task);
-extern struct perf_event *perf_event_get(unsigned int fd);
+extern struct file *perf_event_get(unsigned int fd);
 extern const struct perf_event_attr *perf_event_attrs(struct perf_event *event);
 extern void perf_event_print_debug(void);
 extern void perf_pmu_disable(struct pmu *pmu);
@@ -1070,7 +1070,7 @@ static inline int perf_event_init_task(struct task_struct *child)	{ return 0; }
 static inline void perf_event_exit_task(struct task_struct *child)	{ }
 static inline void perf_event_free_task(struct task_struct *task)	{ }
 static inline void perf_event_delayed_put(struct task_struct *task)	{ }
-static inline struct perf_event *perf_event_get(unsigned int fd)	{ return ERR_PTR(-EINVAL); }
+static inline struct file *perf_event_get(unsigned int fd)	{ return ERR_PTR(-EINVAL); }
 static inline const struct perf_event_attr *perf_event_attrs(struct perf_event *event)
 {
 	return ERR_PTR(-EINVAL);
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index b0799bced518..89ebbc4d1164 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -291,10 +291,13 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd)
 {
 	struct perf_event *event;
 	const struct perf_event_attr *attr;
+	struct file *file;
 
-	event = perf_event_get(fd);
-	if (IS_ERR(event))
-		return event;
+	file = perf_event_get(fd);
+	if (IS_ERR(file))
+		return file;
+
+	event = file->private_data;
 
 	attr = perf_event_attrs(event);
 	if (IS_ERR(attr))
@@ -304,24 +307,22 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd)
 		goto err;
 
 	if (attr->type == PERF_TYPE_RAW)
-		return event;
+		return file;
 
 	if (attr->type == PERF_TYPE_HARDWARE)
-		return event;
+		return file;
 
 	if (attr->type == PERF_TYPE_SOFTWARE &&
 	    attr->config == PERF_COUNT_SW_BPF_OUTPUT)
-		return event;
+		return file;
 err:
-	perf_event_release_kernel(event);
+	fput(file);
 	return ERR_PTR(-EINVAL);
 }
 
 static void perf_event_fd_array_put_ptr(void *ptr)
 {
-	struct perf_event *event = ptr;
-
-	perf_event_release_kernel(event);
+	fput((struct file *)ptr);
 }
 
 static const struct bpf_map_ops perf_event_array_ops = {
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 06ae52e99ac2..2a95e0d2370f 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8896,21 +8896,17 @@ void perf_event_delayed_put(struct task_struct *task)
 		WARN_ON_ONCE(task->perf_event_ctxp[ctxn]);
 }
 
-struct perf_event *perf_event_get(unsigned int fd)
+struct file *perf_event_get(unsigned int fd)
 {
-	int err;
-	struct fd f;
-	struct perf_event *event;
-
-	err = perf_fget_light(fd, &f);
-	if (err)
-		return ERR_PTR(err);
+	struct file *file;
 
-	event = f.file->private_data;
-	atomic_long_inc(&event->refcount);
-	fdput(f);
+	file = fget_raw(fd);
+	if (file->f_op != &perf_fops) {
+		fput(file);
+		return ERR_PTR(-EBADF);
+	}
 
-	return event;
+	return file;
 }
 
 const struct perf_event_attr *perf_event_attrs(struct perf_event *event)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 45dd798bcd37..326a75e884db 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -191,14 +191,17 @@ static u64 bpf_perf_event_read(u64 r1, u64 index, u64 r3, u64 r4, u64 r5)
 	struct bpf_map *map = (struct bpf_map *) (unsigned long) r1;
 	struct bpf_array *array = container_of(map, struct bpf_array, map);
 	struct perf_event *event;
+	struct file *file;
 
 	if (unlikely(index >= array->map.max_entries))
 		return -E2BIG;
 
-	event = (struct perf_event *)array->ptrs[index];
-	if (!event)
+	file = (struct file *)array->ptrs[index];
+	if (unlikely(!file))
 		return -ENOENT;
 
+	event = file->private_data;
+
 	/* make sure event is local and doesn't have pmu::count */
 	if (event->oncpu != smp_processor_id() ||
 	    event->pmu->count)
@@ -228,6 +231,7 @@ static u64 bpf_perf_event_output(u64 r1, u64 r2, u64 index, u64 r4, u64 size)
 	void *data = (void *) (long) r4;
 	struct perf_sample_data sample_data;
 	struct perf_event *event;
+	struct file *file;
 	struct perf_raw_record raw = {
 		.size = size,
 		.data = data,
@@ -236,10 +240,12 @@ static u64 bpf_perf_event_output(u64 r1, u64 r2, u64 index, u64 r4, u64 size)
 	if (unlikely(index >= array->map.max_entries))
 		return -E2BIG;
 
-	event = (struct perf_event *)array->ptrs[index];
-	if (unlikely(!event))
+	file = (struct file *)array->ptrs[index];
+	if (unlikely(!file))
 		return -ENOENT;
 
+	event = file->private_data;
+
 	if (unlikely(event->attr.type != PERF_TYPE_SOFTWARE ||
 		     event->attr.config != PERF_COUNT_SW_BPF_OUTPUT))
 		return -EINVAL;
-- 
2.4.6

[toc] | [prev] | [next] | [standalone]

#1318110 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Mon, Jan 25, 2016 at 08:59:49PM -0800, Alexei Starovoitov wrote:
> I think I understand what you're trying to do and
> the patch looks good to me.

Great, thanks!

> As far as BPF side I did the following...
> does it match the model you outlined above?

Yes, a few comments/questions below.

> 
> Subject: [PATCH ] perf,bpf: convert perf_event_array to use struct file
> 
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Can I take this through the tip/perf tree so that all these changes land
together?

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 06ae52e99ac2..2a95e0d2370f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8896,21 +8896,17 @@ void perf_event_delayed_put(struct task_struct *task)
>  		WARN_ON_ONCE(task->perf_event_ctxp[ctxn]);
>  }
>  
> +struct file *perf_event_get(unsigned int fd)
>  {
> +	struct file *file;
>  
> +	file = fget_raw(fd);

fget_raw() to guarantee the return value isn't NULL? afaict the O_PATH
stuff does not apply to perf events, so you'd put any fd for which the
distinction matters anyway.

> +	if (file->f_op != &perf_fops) {
> +		fput(file);
> +		return ERR_PTR(-EBADF);
> +	}
>  
> +	return file;
>  }

[toc] | [prev] | [next] | [standalone]

#1318197 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Tue, Jan 26, 2016 at 05:16:37PM +0100, Peter Zijlstra wrote:
> > +struct file *perf_event_get(unsigned int fd)
> >  {
> > +	struct file *file;
> >  
> > +	file = fget_raw(fd);
> 
> fget_raw() to guarantee the return value isn't NULL? afaict the O_PATH
> stuff does not apply to perf events, so you'd put any fd for which the
> distinction matters anyway.
> 
> > +	if (file->f_op != &perf_fops) {
> > +		fput(file);
> > +		return ERR_PTR(-EBADF);
> > +	}
> >  
> > +	return file;
> >  }

It is not possible for one thread to concurrently call close() while
this thread tries to fget() ? In which case, we must check the return
value anyway?

[toc] | [prev] | [next] | [standalone]

#1318471 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Tue, Jan 26, 2016 at 06:24:25PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 26, 2016 at 05:16:37PM +0100, Peter Zijlstra wrote:
> > > +struct file *perf_event_get(unsigned int fd)
> > >  {
> > > +	struct file *file;
> > >  
> > > +	file = fget_raw(fd);
> > 
> > fget_raw() to guarantee the return value isn't NULL? afaict the O_PATH
> > stuff does not apply to perf events, so you'd put any fd for which the
> > distinction matters anyway.

yeah good catch. the following is needed:
        file = fget_raw(fd);
+       if (!file)
+               return ERR_PTR(-EBADF);

> > 
> > > +	if (file->f_op != &perf_fops) {
> > > +		fput(file);
> > > +		return ERR_PTR(-EBADF);
> > > +	}
> > >  
> > > +	return file;
> > >  }
> 
> It is not possible for one thread to concurrently call close() while
> this thread tries to fget() ? In which case, we must check the return
> value anyway?

the !file check is definitely needed, fd passed by the user can be bogus.
The caller of perf_event_get() checks for errors too.

This patch will conflict with kernel/bpf/arraymap.c and
kernel/trace/bpf_trace.c that are planned for net-next,
but the conflicts in kernel/events/core.c are probably harder
to resolve, so yes please take it into tip/perf.
I think your scm_right fixes depend on this patch and together
it's an important bug fix, so probably makes sense to send
them right now without waiting for the next merge window?
As soon as you get the whole thing into tip, I'll test it
to make sure bpf side is ok and I hope Wang will test it too.

I'm still a bit concerned about taking file reference for this,
since bpf prorgams that use perf_events won't be able to be
'detached'. Meaning there gotta be always a user space process
that will be holding perf_event FDs. On networking side we
don't have this limitation. Like we can attach bpf to TC,
iproute2 will exit and reattach some time later. So it
kinda sux, but sounds like you want to get rid of
perf_event->refcnt completely, so I don't see any other way.
We can fix it later if it really becomes an issue.

[toc] | [prev] | [next] | [standalone]

#1318806 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Tue, Jan 26, 2016 at 03:31:59PM -0800, Alexei Starovoitov wrote:

> This patch will conflict with kernel/bpf/arraymap.c and
> kernel/trace/bpf_trace.c that are planned for net-next,
> but the conflicts in kernel/events/core.c are probably harder
> to resolve, so yes please take it into tip/perf.

Thanks.

> I think your scm_right fixes depend on this patch and together
> it's an important bug fix, so probably makes sense to send
> them right now without waiting for the next merge window?

I'll leave that up to Ingo, but likely yes.

> As soon as you get the whole thing into tip, I'll test it
> to make sure bpf side is ok and I hope Wang will test it too.
> 
> I'm still a bit concerned about taking file reference for this,
> since bpf prorgams that use perf_events won't be able to be
> 'detached'. 

I was not aware BPF could be detached like that.

> Meaning there gotta be always a user space process
> that will be holding perf_event FDs.

By using fget() the BPF array thing will hold the FDs, right? I mean
once you do a full fget() userspace can go and kill itself, the struct
file will persists.

> On networking side we
> don't have this limitation. Like we can attach bpf to TC,
> iproute2 will exit and reattach some time later. So it
> kinda sux, but sounds like you want to get rid of
> perf_event->refcnt completely, 

We cannot actually get rid of it, we need it for some existence stuff.
But yes, we need stricter cleanup.

> so I don't see any other way.
> We can fix it later if it really becomes an issue.

One possible avenue would be to allow BPF to create its own (kernel)
events and manage its lifetime along with the BPF object. But so far I
don't see a problem with the file thing.

[toc] | [prev] | [next] | [standalone]

#1319193 — Re: [PATCH v2] perf: Synchronously cleanup child events

On Wed, Jan 27, 2016 at 10:58:22AM +0100, Peter Zijlstra wrote:
> 
> > Meaning there gotta be always a user space process
> > that will be holding perf_event FDs.
> 
> By using fget() the BPF array thing will hold the FDs, right? I mean
> once you do a full fget() userspace can go and kill itself, the struct
> file will persists.

Right. I mistakenly thought that fget() will prevent the task
from freeing or some fs/directory resources will be hanging.
It's from anon_inode. So all good. All concerns cleared.

[toc] | [prev] | [next] | [standalone]

#1311981 — Re: [PATCH] perf: Synchronously cleanup child events

* Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:

> The orphan cleanup workqueue doesn't always catch orphans, for example, if they 
> never schedule after they are orphaned. IOW, the event leak is still very real. 
> It also wouldn't work for kernel counters.
> 
> Also, there seems to be no reason not to carry out this cleanup procedure 
> synchronously during parent event's destruction.

Absolutely, synchronous cleanup will probably also be quicker at triggering any 
remaining (and new ;-) races.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web