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

Groups > linux.kernel > #1282916 > unrolled thread

[PATCH 4/7] perf: Free aux pages in unmap path

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/7] perf: Free aux pages in unmap path Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-12-03 11:40 +0100
    Re: [PATCH 4/7] perf: Free aux pages in unmap path Peter Zijlstra <peterz@infradead.org> - 2015-12-04 18:10 +0100
      Re: [PATCH 4/7] perf: Free aux pages in unmap path Peter Zijlstra <peterz@infradead.org> - 2015-12-04 23:20 +0100
        Re: [PATCH 4/7] perf: Free aux pages in unmap path Peter Zijlstra <peterz@infradead.org> - 2015-12-07 17:20 +0100
      Re: [PATCH 4/7] perf: Free aux pages in unmap path Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-12-09 11:00 +0100
        Re: [PATCH 4/7] perf: Free aux pages in unmap path Peter Zijlstra <peterz@infradead.org> - 2015-12-09 12:00 +0100
          Re: [PATCH 4/7] perf: Free aux pages in unmap path Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-12-10 12:30 +0100
            Re: [PATCH 4/7] perf: Free aux pages in unmap path Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2015-12-10 14:10 +0100

#1282916 — [PATCH 4/7] perf: Free aux pages in unmap path

Now that we can ensure that when ring buffer's aux area is on the way
to getting unmapped new transactions won't start, and we have means of
stopping the running transactions, we can do the latter to the events
on this ring buffer's event list and then safely free the aux pages and
corresponding pmu data, as this time it is guaranteed to be the last
aux reference holder. This partially reverts 57ffc5ca679 ("perf: Fix AUX
buffer refcounting"), which was made to defer deallocation that was
otherwise possible from an NMI context. Now it is no longer the case;
the last call to rb_free_aux() that drops the last AUX reference has
to happen in perf_mmap_close() on that AUX area.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 kernel/events/core.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++-
 kernel/events/internal.h    |  1 -
 kernel/events/ring_buffer.c | 37 ++++++++++---------------------
 3 files changed, 63 insertions(+), 28 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 66f835a2df..10fce18710 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4630,11 +4630,62 @@ static void perf_mmap_close(struct vm_area_struct *vma)
 	 */
 	if (rb_has_aux(rb) && vma->vm_pgoff == rb->aux_pgoff &&
 	    atomic_dec_and_mutex_lock(&rb->aux_mmap_count, &event->mmap_mutex)) {
+		struct perf_event *iter;
+		LIST_HEAD(stop_list);
+		unsigned long flags;
+
+		/*
+		 * Stop all aux events that are writing to this here buffer,
+		 * so that we can free its aux pages and corresponding pmu
+		 * data. Note that after rb::aux_mmap_count dropped to zero,
+		 * they won't start any more (see perf_aux_output_begin()).
+		 *
+		 * Since we can't take ctx::mutex under rb::event_lock, we
+		 * need to jump through hoops to get there, namely fish out
+		 * all events from rb::event_list onto an on-stack list,
+		 * carry out the stopping and splice this on-stack list back
+		 * to rb::event_list.
+		 * This means that these events will miss wakeups during this
+		 * window, but since it's mmap_close, assume the consumer
+		 * doesn't care any more.
+		 *
+		 * Note: list_splice_init_rcu() doesn't cut it, since it syncs
+		 * and rb::event_lock is a spinlock.
+		 */
+retry:
+		spin_lock_irqsave(&rb->event_lock, flags);
+		list_for_each_entry_rcu(iter, &rb->event_list, rb_entry) {
+			list_del_rcu(&iter->rb_entry);
+			spin_unlock_irqrestore(&rb->event_lock, flags);
+
+			synchronize_rcu();
+			list_add_tail(&iter->rb_entry, &stop_list);
+
+			goto retry;
+		}
+		spin_unlock_irqrestore(&rb->event_lock, flags);
+
+		mutex_unlock(&event->mmap_mutex);
+
+		list_for_each_entry(iter, &stop_list, rb_entry) {
+			if (!has_aux(iter))
+				continue;
+
+			perf_event_stop(iter);
+		}
+
+		/* and splice it back now that we're done with them */
+		spin_lock_irqsave(&rb->event_lock, flags);
+		list_splice_tail(&stop_list, &rb->event_list);
+		spin_unlock_irqrestore(&rb->event_lock, flags);
+
+		/* now it's safe to free the pages */
 		atomic_long_sub(rb->aux_nr_pages, &mmap_user->locked_vm);
 		vma->vm_mm->pinned_vm -= rb->aux_mmap_locked;
 
+		/* this has to be the last one */
 		rb_free_aux(rb);
-		mutex_unlock(&event->mmap_mutex);
+		WARN_ON_ONCE(atomic_read(&rb->aux_refcount));
 	}
 
 	atomic_dec(&rb->mmap_count);
diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 2bbad9c127..2b229fdcfc 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -11,7 +11,6 @@
 struct ring_buffer {
 	atomic_t			refcount;
 	struct rcu_head			rcu_head;
-	struct irq_work			irq_work;
 #ifdef CONFIG_PERF_USE_VMALLOC
 	struct work_struct		work;
 	int				page_order;	/* allocation order  */
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 5709cc222f..6865ac95ca 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -221,8 +221,6 @@ void perf_output_end(struct perf_output_handle *handle)
 	rcu_read_unlock();
 }
 
-static void rb_irq_work(struct irq_work *work);
-
 static void
 ring_buffer_init(struct ring_buffer *rb, long watermark, int flags)
 {
@@ -243,16 +241,6 @@ ring_buffer_init(struct ring_buffer *rb, long watermark, int flags)
 
 	INIT_LIST_HEAD(&rb->event_list);
 	spin_lock_init(&rb->event_lock);
-	init_irq_work(&rb->irq_work, rb_irq_work);
-}
-
-static void ring_buffer_put_async(struct ring_buffer *rb)
-{
-	if (!atomic_dec_and_test(&rb->refcount))
-		return;
-
-	rb->rcu_head.next = (void *)rb;
-	irq_work_queue(&rb->irq_work);
 }
 
 /*
@@ -292,7 +280,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
 	 * the aux buffer is in perf_mmap_close(), about to get free'd.
 	 */
 	if (!atomic_read(&rb->aux_mmap_count))
-		goto err;
+		goto err_put;
 
 	/*
 	 * Nesting is not supported for AUX area, make sure nested
@@ -338,7 +326,7 @@ err_put:
 	rb_free_aux(rb);
 
 err:
-	ring_buffer_put_async(rb);
+	ring_buffer_put(rb);
 	handle->event = NULL;
 
 	return NULL;
@@ -389,7 +377,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size,
 
 	local_set(&rb->aux_nest, 0);
 	rb_free_aux(rb);
-	ring_buffer_put_async(rb);
+	ring_buffer_put(rb);
 }
 
 /*
@@ -563,6 +551,14 @@ static void __rb_free_aux(struct ring_buffer *rb)
 {
 	int pg;
 
+	/*
+	 * Should never happen, the last reference should be dropped from
+	 * perf_mmap_close() path, which first stops aux transactions (which
+	 * in turn are the atomic holders of aux_refcount) and then does the
+	 * last rb_free_aux().
+	 */
+	WARN_ON_ONCE(in_atomic());
+
 	if (rb->aux_priv) {
 		rb->free_aux(rb->aux_priv);
 		rb->free_aux = NULL;
@@ -581,18 +577,7 @@ static void __rb_free_aux(struct ring_buffer *rb)
 void rb_free_aux(struct ring_buffer *rb)
 {
 	if (atomic_dec_and_test(&rb->aux_refcount))
-		irq_work_queue(&rb->irq_work);
-}
-
-static void rb_irq_work(struct irq_work *work)
-{
-	struct ring_buffer *rb = container_of(work, struct ring_buffer, irq_work);
-
-	if (!atomic_read(&rb->aux_refcount))
 		__rb_free_aux(rb);
-
-	if (rb->rcu_head.next == (void *)rb)
-		call_rcu(&rb->rcu_head, rb_free_rcu);
 }
 
 #ifndef CONFIG_PERF_USE_VMALLOC
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1284042

On Thu, Dec 03, 2015 at 12:32:39PM +0200, Alexander Shishkin wrote:
> +++ b/kernel/events/core.c
> @@ -4630,11 +4630,62 @@ static void perf_mmap_close(struct vm_area_struct *vma)
>  	 */
>  	if (rb_has_aux(rb) && vma->vm_pgoff == rb->aux_pgoff &&
>  	    atomic_dec_and_mutex_lock(&rb->aux_mmap_count, &event->mmap_mutex)) {
> +		struct perf_event *iter;
> +		LIST_HEAD(stop_list);
> +		unsigned long flags;
> +
> +		/*
> +		 * Stop all aux events that are writing to this here buffer,
> +		 * so that we can free its aux pages and corresponding pmu
> +		 * data. Note that after rb::aux_mmap_count dropped to zero,
> +		 * they won't start any more (see perf_aux_output_begin()).
> +		 *
> +		 * Since we can't take ctx::mutex under rb::event_lock, we
> +		 * need to jump through hoops to get there, namely fish out
> +		 * all events from rb::event_list onto an on-stack list,
> +		 * carry out the stopping and splice this on-stack list back
> +		 * to rb::event_list.
> +		 * This means that these events will miss wakeups during this
> +		 * window, but since it's mmap_close, assume the consumer
> +		 * doesn't care any more.
> +		 *
> +		 * Note: list_splice_init_rcu() doesn't cut it, since it syncs
> +		 * and rb::event_lock is a spinlock.
> +		 */
> +retry:
> +		spin_lock_irqsave(&rb->event_lock, flags);
> +		list_for_each_entry_rcu(iter, &rb->event_list, rb_entry) {
> +			list_del_rcu(&iter->rb_entry);
> +			spin_unlock_irqrestore(&rb->event_lock, flags);
> +
> +			synchronize_rcu();
> +			list_add_tail(&iter->rb_entry, &stop_list);
> +
> +			goto retry;
> +		}
> +		spin_unlock_irqrestore(&rb->event_lock, flags);
> +
> +		mutex_unlock(&event->mmap_mutex);
> +
> +		list_for_each_entry(iter, &stop_list, rb_entry) {
> +			if (!has_aux(iter))
> +				continue;
> +
> +			perf_event_stop(iter);
> +		}
> +
> +		/* and splice it back now that we're done with them */
> +		spin_lock_irqsave(&rb->event_lock, flags);
> +		list_splice_tail(&stop_list, &rb->event_list);
> +		spin_unlock_irqrestore(&rb->event_lock, flags);
> +
> +		/* now it's safe to free the pages */
>  		atomic_long_sub(rb->aux_nr_pages, &mmap_user->locked_vm);
>  		vma->vm_mm->pinned_vm -= rb->aux_mmap_locked;
>  
> +		/* this has to be the last one */
>  		rb_free_aux(rb);
> -		mutex_unlock(&event->mmap_mutex);
> +		WARN_ON_ONCE(atomic_read(&rb->aux_refcount));
>  	}

Yuck, nasty problem. Also, I think its broken. By not having
mmap_mutex around the whole thing, notably rb_free_aux(), you can race
against mmap().

What seems possible now is that:

	mmap(aux); // rb->aux_mmap_count == 1
	munmap(aux)
	  atomic_dec_and_mutex_lock(&rb->aux_mmap_count, &event->mmap_mutex); // == 0

	  mutex_unlock(&event->mmap_mutex);

					mmap(aux)
					  if (rb_has_aux())
					    atomic_inc(&rb->aux_mmap_count); // == 1

	  rb_free_aux(); // oops!!




So I thought that pulling all the aux bits out from the ring_buffer
struct, such that we have rb->aux, would solve the issue in that we can
then fix mmap() to have the same retry loop as for event->rb.

And while that fixes that race (I almost had that patch complete -- I
might still send it out, just so you can see what it looks like), it
doesn't solve the complete problem I don't think.

Because in that case, you want the event to start again on the new
buffer, and I think its possible we end up calling ->start() before
we've issued the ->stop() and that would be BAD (tm).

The only solution I've come up with is:

	struct rb_aux *aux = rb->aux;

	if (aux && vma->vm_pgoff == aux->pgoff) {
		ctx = perf_event_ctx_lock(event);
		if (!atomic_dec_and_mutex_lock(&aux->mmap_count, &event->mmap_mutex) {
			/* we now hold both ctx::mutex and event::mmap_mutex */
			rb->aux = NULL;
			ring_buffer_put(rb); /* aux had a reference */
			_perf_event_stop(event);
			ring_buffer_put_aux(aux); /* should be last */
			mutex_unlock(&event->mmap_mutex);
		}
		mutex_unlock(&ctx->mutex);
	}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1284300

On Fri, Dec 04, 2015 at 06:02:06PM +0100, Peter Zijlstra wrote:
> The only solution I've come up with is:
> 
> 	struct rb_aux *aux = rb->aux;
> 
> 	if (aux && vma->vm_pgoff == aux->pgoff) {
> 		ctx = perf_event_ctx_lock(event);

Can't do this at all, see the comment in put_event(). perf_read_group()
accesses user memory (and hence causes faults, which in turn take
mmap_sem) while holding ctx::mutex.

So neither this, not what you proposed can work.

Will need moar thinking.

> 		if (!atomic_dec_and_mutex_lock(&aux->mmap_count, &event->mmap_mutex) {
> 			/* we now hold both ctx::mutex and event::mmap_mutex */
> 			rb->aux = NULL;
> 			ring_buffer_put(rb); /* aux had a reference */
> 			_perf_event_stop(event);
> 			ring_buffer_put_aux(aux); /* should be last */
> 			mutex_unlock(&event->mmap_mutex);
> 		}
> 		mutex_unlock(&ctx->mutex);
> 	}
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1285735

On Fri, Dec 04, 2015 at 11:17:23PM +0100, Peter Zijlstra wrote:
> On Fri, Dec 04, 2015 at 06:02:06PM +0100, Peter Zijlstra wrote:
> > The only solution I've come up with is:
> > 
> > 	struct rb_aux *aux = rb->aux;
> > 
> > 	if (aux && vma->vm_pgoff == aux->pgoff) {
> > 		ctx = perf_event_ctx_lock(event);
> 
> Can't do this at all, see the comment in put_event(). perf_read_group()
> accesses user memory (and hence causes faults, which in turn take
> mmap_sem) while holding ctx::mutex.
> 
> So neither this, not what you proposed can work.
> 
> Will need moar thinking.

So we could try and see if we can get this working:

static int __perf_event_stop(void *info)
{
	struct perf_event *event = info;

	/* IRQs disabled, cannot get scheduled away */
	if (event->oncpu == smp_processor_id()) {
		event->pmu->stop(event);
		return 0;
	}

	return -EAGAIN;
}

perf_event_stop(struct perf_event *event)
{
	for (;;) {
		if (READ_ONCE(event->state) != PERF_EVENT_STATE_ACTIVE)
			break;

		smp_rmb(); /* if we see ACTIVE, ->oncpu must be set */

		if (!cpu_function_call(READ_ONCE(event->oncpu), __perf_event_stop, event))
			break;
	}
}


That probably wants some WRITE_ONCE() and maybe some memory barriers in
event_sched_in() as well, like:

	WRITE_ONCE(event->oncpu, smp_processor_id());
	smp_wmb();
	WRITE_ONCE(event->state, PERF_EVENT_STATE_ACTIVE);



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1287308

Peter Zijlstra <peterz@infradead.org> writes:

> Yuck, nasty problem. Also, I think its broken. By not having
> mmap_mutex around the whole thing, notably rb_free_aux(), you can race
> against mmap().
>
> What seems possible now is that:
>
> 	mmap(aux); // rb->aux_mmap_count == 1
> 	munmap(aux)
> 	  atomic_dec_and_mutex_lock(&rb->aux_mmap_count, &event->mmap_mutex); // == 0
>
> 	  mutex_unlock(&event->mmap_mutex);
>
> 					mmap(aux)
> 					  if (rb_has_aux())
> 					    atomic_inc(&rb->aux_mmap_count); // == 1
>
> 	  rb_free_aux(); // oops!!

Wait, this isn't actually a problem, we can hold mmap_mutex over
rb_free_aux(), as we actually already do in current code. My patch did
it wrongly though, but there's really no reason to drop the mutex before
rb_free_aux().

> So I thought that pulling all the aux bits out from the ring_buffer
> struct, such that we have rb->aux, would solve the issue in that we can
> then fix mmap() to have the same retry loop as for event->rb.
>
> And while that fixes that race (I almost had that patch complete -- I
> might still send it out, just so you can see what it looks like), it
> doesn't solve the complete problem I don't think.

I was toying with that some time ago, but I couldn't really see the
benefits that would justify the hassle.

> Because in that case, you want the event to start again on the new
> buffer, and I think its possible we end up calling ->start() before
> we've issued the ->stop() and that would be BAD (tm).

So if we just hold the mmap_mutex over rb_free_aux(), this won't
happen, right?

> The only solution I've come up with is:
>
> 	struct rb_aux *aux = rb->aux;
>
> 	if (aux && vma->vm_pgoff == aux->pgoff) {
> 		ctx = perf_event_ctx_lock(event);
> 		if (!atomic_dec_and_mutex_lock(&aux->mmap_count, &event->mmap_mutex) {
> 			/* we now hold both ctx::mutex and event::mmap_mutex */
> 			rb->aux = NULL;
> 			ring_buffer_put(rb); /* aux had a reference */
> 			_perf_event_stop(event);

Here we really need to ensure that none of the events on the
rb->event_list is running, not just the parent, and that still presents
complications wrt irqsave rb->event_lock even with your new idea for
perf_event_stop().

How about something like this to stop the writers:

static int __ring_buffer_output_stop(void *info)
{
	struct ring_buffer *rb = info;
	struct perf_event *event;
 
	spin_lock(&rb->event_lock);
	list_for_each_entry_rcu(event, &rb->event_list, rb_entry) {
		if (event->state != PERF_EVENT_STATE_ACTIVE)
			continue;

		event->pmu->stop(event, PERF_EF_UPDATE);
	}
	spin_unlock(&rb->event_lock);

	return 0;
}

static void perf_event_output_stop(struct perf_event *event)
{
	struct ring_buffer *rb = event->rb;

	lockdep_assert_held(&event->mmap_mutex);

	if (event->cpu == -1)
		perf_event_stop(event);

	cpu_function_call(event->cpu, __ring_buffer_output_stop, rb);
}
 
And then in the mmap_close:

	if (rb_has_aux(rb) && vma->vm_pgoff == rb->aux_pgoff &&
	    atomic_dec_and_mutex_lock(&rb->aux_mmap_count, &event->mmap_mutex)) {
		perf_event_output_stop(event);

                /* undo the mlock accounting here */

 		rb_free_aux(rb);
               	mutex_unlock(&event->mmap_mutex);
	}

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1287411

On Wed, Dec 09, 2015 at 11:57:51AM +0200, Alexander Shishkin wrote:
> Peter Zijlstra <peterz@infradead.org> writes:
> 
> > Yuck, nasty problem. Also, I think its broken. By not having
> > mmap_mutex around the whole thing, notably rb_free_aux(), you can race
> > against mmap().
> >
> > What seems possible now is that:
> >
> > 	mmap(aux); // rb->aux_mmap_count == 1
> > 	munmap(aux)
> > 	  atomic_dec_and_mutex_lock(&rb->aux_mmap_count, &event->mmap_mutex); // == 0
> >
> > 	  mutex_unlock(&event->mmap_mutex);
> >
> > 					mmap(aux)
> > 					  if (rb_has_aux())
> > 					    atomic_inc(&rb->aux_mmap_count); // == 1
> >
> > 	  rb_free_aux(); // oops!!
> 
> Wait, this isn't actually a problem, we can hold mmap_mutex over
> rb_free_aux(), as we actually already do in current code. My patch did
> it wrongly though, but there's really no reason to drop the mutex before
> rb_free_aux().

Well, you had to drop it because you wanted to acquire the ctx::mutex,
but if we drop that requirement, as we must per the other emails, then
this should indeed be possible.

> So if we just hold the mmap_mutex over rb_free_aux(), this won't
> happen, right?

Correct.

> How about something like this to stop the writers:
> 
> static int __ring_buffer_output_stop(void *info)
> {
> 	struct ring_buffer *rb = info;
> 	struct perf_event *event;
>  
> 	spin_lock(&rb->event_lock);
> 	list_for_each_entry_rcu(event, &rb->event_list, rb_entry) {
> 		if (event->state != PERF_EVENT_STATE_ACTIVE)
> 			continue;
> 
> 		event->pmu->stop(event, PERF_EF_UPDATE);
> 	}
> 	spin_unlock(&rb->event_lock);
> 
> 	return 0;
> }
> 
> static void perf_event_output_stop(struct perf_event *event)
> {
> 	struct ring_buffer *rb = event->rb;
> 
> 	lockdep_assert_held(&event->mmap_mutex);
> 
> 	if (event->cpu == -1)
> 		perf_event_stop(event);
> 
> 	cpu_function_call(event->cpu, __ring_buffer_output_stop, rb);

I'm not sure about the different semantics between event->cpu == -1 and
not, but yes, something along those likes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1288455

Peter Zijlstra <peterz@infradead.org> writes:

>> How about something like this to stop the writers:
>> 
>> static int __ring_buffer_output_stop(void *info)
>> {
>> 	struct ring_buffer *rb = info;
>> 	struct perf_event *event;
>>  
>> 	spin_lock(&rb->event_lock);
>> 	list_for_each_entry_rcu(event, &rb->event_list, rb_entry) {
>> 		if (event->state != PERF_EVENT_STATE_ACTIVE)
>> 			continue;
>> 
>> 		event->pmu->stop(event, PERF_EF_UPDATE);
>> 	}
>> 	spin_unlock(&rb->event_lock);
>> 
>> 	return 0;
>> }
>> 
>> static void perf_event_output_stop(struct perf_event *event)
>> {
>> 	struct ring_buffer *rb = event->rb;
>> 
>> 	lockdep_assert_held(&event->mmap_mutex);
>> 
>> 	if (event->cpu == -1)
>> 		perf_event_stop(event);
>> 
>> 	cpu_function_call(event->cpu, __ring_buffer_output_stop, rb);
>
> I'm not sure about the different semantics between event->cpu == -1 and
> not, but yes, something along those likes.

So this also doesn't quite cut it, since we also have children (which
aren't explicitly ring_buffer_attach()ed to the ring buffer, but will
still write there) and in particular children of those events that are
on rb->event_list, which triples the fun of synchronizing and iterating
these.

So I tried a different approach: iterating through pmu's contexts'
instead. Consider the following. 

+static void __perf_event_output_stop(struct perf_event *event, void *data)
+{
+       struct perf_event *parent = event->parent;
+       struct ring_buffer *rb = data;
+
+       if (rcu_dereference(event->rb) == rb)
+               __perf_event_stop(event);
+       if (parent && rcu_dereference(parent->rb) == rb)
+               __perf_event_stop(event);
+}
+
+static int __perf_pmu_output_stop(void *info)
+{
+       struct perf_event *event = info;
+       struct pmu *pmu = event->pmu;
+       struct perf_cpu_context *cpuctx = get_cpu_ptr(pmu->pmu_cpu_context);
+
+       rcu_read_lock();
+       perf_event_aux_ctx(&cpuctx->ctx, __perf_event_output_stop, event->rb, true);
+       if (cpuctx->task_ctx)
+               perf_event_aux_ctx(cpuctx->task_ctx, __perf_event_output_stop,
+                                  event->rb, true);
+       rcu_read_unlock();
+
+       return 0;
+}
+
+static void perf_pmu_output_stop(struct perf_event *event)
+{
+       int cpu;
+
+       get_online_cpus();
+       for_each_online_cpu(cpu) {
+               cpu_function_call(cpu, __perf_pmu_output_stop, event);
+       }
+       put_online_cpus();
+}

And then we just call this perf_pmu_output_stop() from perf_mmap_close()
under mmap_mutex, before rb_free_aux(). Likely going through online cpus
is not necessary, but I'm gonna grab a lunch first.

I also hacked perf_event_aux_ctx() to make the above work, like so:

@@ -5696,15 +5696,18 @@ typedef void (perf_event_aux_output_cb)(struct perf_event *event, void *data);
 static void
 perf_event_aux_ctx(struct perf_event_context *ctx,
                   perf_event_aux_output_cb output,
-                  void *data)
+                  void *data, bool all)
 {
        struct perf_event *event;
 
        list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
-               if (event->state < PERF_EVENT_STATE_INACTIVE)
-                       continue;
-               if (!event_filter_match(event))
-                       continue;
+               if (!all) {
+                       if (event->state < PERF_EVENT_STATE_INACTIVE)
+                               continue;
+                       if (!event_filter_match(event))
+                               continue;
+               }
+
                output(event, data);
        }
 }

How does this look to you?

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1288501

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

> I also hacked perf_event_aux_ctx() to make the above work, like so:
>
> @@ -5696,15 +5696,18 @@ typedef void (perf_event_aux_output_cb)(struct perf_event *event, void *data);
>  static void
>  perf_event_aux_ctx(struct perf_event_context *ctx,
>                    perf_event_aux_output_cb output,
> -                  void *data)
> +                  void *data, bool all)
>  {
>         struct perf_event *event;
>  
>         list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
> -               if (event->state < PERF_EVENT_STATE_INACTIVE)
> -                       continue;
> -               if (!event_filter_match(event))
> -                       continue;
> +               if (!all) {
> +                       if (event->state < PERF_EVENT_STATE_INACTIVE)
> +                               continue;
> +                       if (!event_filter_match(event))
> +                               continue;
> +               }
> +
>                 output(event, data);
>         }
>  }
>

This last bit is actually not needed at all, not for the task at hand anyway.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web