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

Groups > linux.kernel > #1471713 > unrolled thread

Re: [tip:perf/core] perf/core: Check return value of the perf_event_read() IPI

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

  Re: [tip:perf/core] perf/core: Check return value of the  perf_event_read() IPI Peter Zijlstra <peterz@infradead.org> - 2016-08-29 12:50 +0200
    Re: [tip:perf/core] perf/core: Check return value of the  perf_event_read() IPI Peter Zijlstra <peterz@infradead.org> - 2016-08-29 15:10 +0200
      Re: [tip:perf/core] perf/core: Check return value of the perf_event_read()  IPI Vince Weaver <vincent.weaver@maine.edu> - 2016-08-29 15:20 +0200
      Re: [tip:perf/core] perf/core: Check return value of the  perf_event_read() IPI Jiri Olsa <jolsa@redhat.com> - 2016-08-29 21:00 +0200
        Re: [tip:perf/core] perf/core: Check return value of the  perf_event_read() IPI Peter Zijlstra <peterz@infradead.org> - 2016-08-30 08:50 +0200
          Re: [tip:perf/core] perf/core: Check return value of the  perf_event_read() IPI Peter Zijlstra <peterz@infradead.org> - 2016-08-30 09:30 +0200
            Re: [tip:perf/core] perf/core: Check return value of the  perf_event_read() IPI Jiri Olsa <jolsa@redhat.com> - 2016-08-30 11:50 +0200
      Re: [tip:perf/core] perf/core: Check return value of the  perf_event_read() IPI Stephane Eranian <eranian@google.com> - 2016-08-30 18:30 +0200
        Re: [tip:perf/core] perf/core: Check return value of the  perf_event_read() IPI Peter Zijlstra <peterz@infradead.org> - 2016-09-02 10:40 +0200

#1471713 — Re: [tip:perf/core] perf/core: Check return value of the perf_event_read() IPI

On Mon, Aug 22, 2016 at 12:38:23PM +0200, Jiri Olsa wrote:
> ---
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 3f07e6cfc1b6..375274b6f3b4 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1802,8 +1802,9 @@ event_sched_out(struct perf_event *event,
>  
>  	event->tstamp_stopped = tstamp;
>  	event->pmu->del(event, 0);
> -	event->oncpu = -1;
> -	event->state = PERF_EVENT_STATE_INACTIVE;
> +	WRITE_ONCE(event->state, PERF_EVENT_STATE_INACTIVE);
> +	smp_wmb();
> +	WRITE_ONCE(event->oncpu, -1);
>  	if (event->pending_disable) {
>  		event->pending_disable = 0;
>  		event->state = PERF_EVENT_STATE_OFF;

> @@ -3561,13 +3561,17 @@ u64 perf_event_read_local(struct perf_event *event)
>  
>  static int perf_event_read(struct perf_event *event, bool group)
>  {
> -	int ret = 0, cpu_to_read, local_cpu;
> +	int ret = 0, cpu_to_read, local_cpu, state;
> +
> +	state = READ_ONCE(event->state);
> +	smp_rmb();
> +	cpu_to_read = event->oncpu;
>  

This cannot be right, this doesn't provide any guarantees. You need to
cross the variables to cancel out timing.


	X = 1		r1 = X
	wmb		rmb
	Y = 1		r2 = Y

is a no-op, you can still get all 4 possible outcomes:


r1==0 && r2==0:
			r1 = X
	                rmb
                        r2 = Y
	X = 1
	wmb
	Y = 1

r1==0 && r2==1:

			r1 = X
	X = 1
	wmb		rmb
	Y = 1
			r2 = Y

r1==1 && r2==0:

	X = 1
			r1 = X
	wmb		rmb
			r2 = Y
	Y = 1


r1==1 && r2==1:

	X = 1
	wmb
	Y = 1
			r1 = X
	                rmb
                        r2 = Y



But once you cross, like:

	X = 1		r2 = Y
	wmb		rmb
	Y = 1		r1 = X

do you get a guarantee, namely: r1==0 && r2==1 becomes impossible, since
if you observe Y==1, we must then also observe X==1.

That said, you're on the right track, and this mirrors event_sched_in()
nicely. But even if we do cross things, we're still not good, because
even if ->oncpu was valid, nothing guarantees us it still is, it could
have been unplugged meanwhile.

Luckily that's not too hard to fix, we just need to disable preemption
over the lot. And while looking at that, the put_cpu() is too early
anyway, with the current code we could get migrated between put_cpu()
and smp_call_function_single(), which would destroy the whole point of
us doing the find_cpu_to_read() thing.

So, how about something like this:

---
 kernel/events/core.c | 53 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index eed96b85503f..c672c5eb2c44 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1802,8 +1802,18 @@ event_sched_out(struct perf_event *event,
 
 	event->tstamp_stopped = tstamp;
 	event->pmu->del(event, 0);
-	event->oncpu = -1;
-	event->state = PERF_EVENT_STATE_INACTIVE;
+
+	WRITE_ONCE(event->state, PERF_EVENT_STATE_INACTIVE);
+	/*
+	 * pmu::del() will have updated the event count. Now mark it inactive,
+	 * but take care to clear ->oncpu after the INACTIVE store, such that
+	 * while ->state == ACTIVE, ->oncpu must be valid.
+	 *
+	 * See event_sched_in(), perf_event_restart() and perf_event_read().
+	 */
+	smp_wmb();
+	WRITE_ONCE(event->oncpu, -1);
+
 	if (event->pending_disable) {
 		event->pending_disable = 0;
 		event->state = PERF_EVENT_STATE_OFF;
@@ -2015,8 +2025,10 @@ event_sched_in(struct perf_event *event,
 
 	WRITE_ONCE(event->oncpu, smp_processor_id());
 	/*
-	 * Order event::oncpu write to happen before the ACTIVE state
-	 * is visible.
+	 * Order event::oncpu write to happen before the ACTIVE state is
+	 * visible, such that when we observe ACTIVE, oncpu must be correct.
+	 *
+	 * Matches the smp_rmb() in perf_event_restart().
 	 */
 	smp_wmb();
 	WRITE_ONCE(event->state, PERF_EVENT_STATE_ACTIVE);
@@ -2509,7 +2521,11 @@ static int perf_event_restart(struct perf_event *event)
 		if (READ_ONCE(event->state) != PERF_EVENT_STATE_ACTIVE)
 			return 0;
 
-		/* matches smp_wmb() in event_sched_in() */
+		/*
+		 * Matches the smp_wmb() from event_sched_in(), such that if
+		 * we observe ACTIVE above, we know the ->oncpu load below
+		 * must be a valid CPU.
+		 */
 		smp_rmb();
 
 		/*
@@ -3424,9 +3440,8 @@ struct perf_read_data {
 	int ret;
 };
 
-static int find_cpu_to_read(struct perf_event *event, int local_cpu)
+static int find_cpu_to_read(struct perf_event *event, int event_cpu, int local_cpu)
 {
-	int event_cpu = event->oncpu;
 	u16 local_pkg, event_pkg;
 
 	if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
@@ -3561,28 +3576,36 @@ u64 perf_event_read_local(struct perf_event *event)
 
 static int perf_event_read(struct perf_event *event, bool group)
 {
-	int ret = 0, cpu_to_read, local_cpu;
+	int ret = 0, cpu_to_read, local_cpu, state;
+
+	local_cpu = get_cpu(); /* disable preemption to hold off hotplut */
+	cpu_to_read = READ_ONCE(event->oncpu);
+	/*
+	 * Matches smp_wmb() from event_sched_out(), ->oncpu must be valid
+	 * IFF we observe ACTIVE.
+	 */
+	smp_rmb();
+	state = READ_ONCE(event->state);
 
 	/*
 	 * If event is enabled and currently active on a CPU, update the
 	 * value in the event structure:
 	 */
-	if (event->state == PERF_EVENT_STATE_ACTIVE) {
+	if (state == PERF_EVENT_STATE_ACTIVE) {
 		struct perf_read_data data = {
 			.event = event,
 			.group = group,
 			.ret = 0,
 		};
 
-		local_cpu = get_cpu();
-		cpu_to_read = find_cpu_to_read(event, local_cpu);
-		put_cpu();
-
+		cpu_to_read = find_cpu_to_read(event, cpu_to_read, local_cpu);
 		ret = smp_call_function_single(cpu_to_read, __perf_event_read, &data, 1);
+
 		/* The event must have been read from an online CPU: */
 		WARN_ON_ONCE(ret);
 		ret = ret ? : data.ret;
-	} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
+
+	} else if (state == PERF_EVENT_STATE_INACTIVE) {
 		struct perf_event_context *ctx = event->ctx;
 		unsigned long flags;
 
@@ -3603,6 +3626,8 @@ static int perf_event_read(struct perf_event *event, bool group)
 		raw_spin_unlock_irqrestore(&ctx->lock, flags);
 	}
 
+	put_cpu();
+
 	return ret;
 }
 

[toc] | [next] | [standalone]

#1471823

On Mon, Aug 29, 2016 at 12:03:09PM +0200, Peter Zijlstra wrote:
> @@ -1802,8 +1802,18 @@ event_sched_out(struct perf_event *event,
>  
>  	event->tstamp_stopped = tstamp;
>  	event->pmu->del(event, 0);
> -	event->oncpu = -1;
> -	event->state = PERF_EVENT_STATE_INACTIVE;
> +
> +	WRITE_ONCE(event->state, PERF_EVENT_STATE_INACTIVE);
> +	/*
> +	 * pmu::del() will have updated the event count. Now mark it inactive,
> +	 * but take care to clear ->oncpu after the INACTIVE store, such that
> +	 * while ->state == ACTIVE, ->oncpu must be valid.
> +	 *
> +	 * See event_sched_in(), perf_event_restart() and perf_event_read().
> +	 */
> +	smp_wmb();
> +	WRITE_ONCE(event->oncpu, -1);
> +
>  	if (event->pending_disable) {
>  		event->pending_disable = 0;
>  		event->state = PERF_EVENT_STATE_OFF;
> @@ -2015,8 +2025,10 @@ event_sched_in(struct perf_event *event,
>  
>  	WRITE_ONCE(event->oncpu, smp_processor_id());
>  	/*
> -	 * Order event::oncpu write to happen before the ACTIVE state
> -	 * is visible.
> +	 * Order event::oncpu write to happen before the ACTIVE state is
> +	 * visible, such that when we observe ACTIVE, oncpu must be correct.
> +	 *
> +	 * Matches the smp_rmb() in perf_event_restart().
>  	 */
>  	smp_wmb();
>  	WRITE_ONCE(event->state, PERF_EVENT_STATE_ACTIVE);

Urgh.. that cannot work either, because now perf_event_read() can race
against event_sched_in(). Since that's no longer crossed.

> @@ -3561,28 +3576,36 @@ u64 perf_event_read_local(struct perf_event *event)
>  
>  static int perf_event_read(struct perf_event *event, bool group)
>  {
> -	int ret = 0, cpu_to_read, local_cpu;
> +	int ret = 0, cpu_to_read, local_cpu, state;
> +
> +	local_cpu = get_cpu(); /* disable preemption to hold off hotplut */
> +	cpu_to_read = READ_ONCE(event->oncpu);
> +	/*
> +	 * Matches smp_wmb() from event_sched_out(), ->oncpu must be valid
> +	 * IFF we observe ACTIVE.
> +	 */
> +	smp_rmb();
> +	state = READ_ONCE(event->state);

The best I can come up with is something like:


	do {
		state = READ_ONCE(event->state);
		if (state != ACTIVE)
			break;
		smp_rmb();
		cpu = READ_ONCE(event->cpu);
		smp_rmb();
	} while (READ_ONCE(event->state) != state);


And I suppose perf_event_restart() should do the same thing... Let me
ponder this a wee bit more.

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

#1471827 — Re: [tip:perf/core] perf/core: Check return value of the perf_event_read() IPI

On Mon, 29 Aug 2016, Peter Zijlstra wrote:

> 
> The best I can come up with is something like:
> 
> 
> 	do {
> 		state = READ_ONCE(event->state);
> 		if (state != ACTIVE)
> 			break;
> 		smp_rmb();
> 		cpu = READ_ONCE(event->cpu);
> 		smp_rmb();
> 	} while (READ_ONCE(event->state) != state);
> 
> 
> And I suppose perf_event_restart() should do the same thing... Let me
> ponder this a wee bit more.

the perf_fuzzer trips over this warning fairly regularly too, so once you 
have a patch you're happy with I can give it a test.  

I ordered a whole crate of null modem cables so I've got a whole bunch of 
serial consoles ready and raring to go.  Maybe I can find out how the 
fuzzer crashes the new skylake machine, it doesn't leave anything useful 
in the syslog.

Vince

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

#1472052

On Mon, Aug 29, 2016 at 03:02:13PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 29, 2016 at 12:03:09PM +0200, Peter Zijlstra wrote:
> > @@ -1802,8 +1802,18 @@ event_sched_out(struct perf_event *event,
> >  
> >  	event->tstamp_stopped = tstamp;
> >  	event->pmu->del(event, 0);
> > -	event->oncpu = -1;
> > -	event->state = PERF_EVENT_STATE_INACTIVE;
> > +
> > +	WRITE_ONCE(event->state, PERF_EVENT_STATE_INACTIVE);
> > +	/*
> > +	 * pmu::del() will have updated the event count. Now mark it inactive,
> > +	 * but take care to clear ->oncpu after the INACTIVE store, such that
> > +	 * while ->state == ACTIVE, ->oncpu must be valid.
> > +	 *
> > +	 * See event_sched_in(), perf_event_restart() and perf_event_read().
> > +	 */
> > +	smp_wmb();
> > +	WRITE_ONCE(event->oncpu, -1);
> > +
> >  	if (event->pending_disable) {
> >  		event->pending_disable = 0;
> >  		event->state = PERF_EVENT_STATE_OFF;
> > @@ -2015,8 +2025,10 @@ event_sched_in(struct perf_event *event,
> >  
> >  	WRITE_ONCE(event->oncpu, smp_processor_id());
> >  	/*
> > -	 * Order event::oncpu write to happen before the ACTIVE state
> > -	 * is visible.
> > +	 * Order event::oncpu write to happen before the ACTIVE state is
> > +	 * visible, such that when we observe ACTIVE, oncpu must be correct.
> > +	 *
> > +	 * Matches the smp_rmb() in perf_event_restart().
> >  	 */
> >  	smp_wmb();
> >  	WRITE_ONCE(event->state, PERF_EVENT_STATE_ACTIVE);
> 
> Urgh.. that cannot work either, because now perf_event_read() can race
> against event_sched_in(). Since that's no longer crossed.
> 
> > @@ -3561,28 +3576,36 @@ u64 perf_event_read_local(struct perf_event *event)
> >  
> >  static int perf_event_read(struct perf_event *event, bool group)
> >  {
> > -	int ret = 0, cpu_to_read, local_cpu;
> > +	int ret = 0, cpu_to_read, local_cpu, state;
> > +
> > +	local_cpu = get_cpu(); /* disable preemption to hold off hotplut */
> > +	cpu_to_read = READ_ONCE(event->oncpu);
> > +	/*
> > +	 * Matches smp_wmb() from event_sched_out(), ->oncpu must be valid
> > +	 * IFF we observe ACTIVE.
> > +	 */
> > +	smp_rmb();
> > +	state = READ_ONCE(event->state);
> 
> The best I can come up with is something like:
> 
> 
> 	do {
> 		state = READ_ONCE(event->state);
> 		if (state != ACTIVE)
> 			break;
> 		smp_rmb();
> 		cpu = READ_ONCE(event->cpu);
> 		smp_rmb();
> 	} while (READ_ONCE(event->state) != state);

couldn't we just call smp_call_function_single(cpu_to_read, __perf_event_read, ...
once we read oncpu != -1 ?

__perf_event_read then checks safely if event is active

jirka


---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index eed96b85503f..b99b791c16bb 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3421,6 +3421,7 @@ out:
 struct perf_read_data {
 	struct perf_event *event;
 	bool group;
+	bool inactive;
 	int ret;
 };
 
@@ -3562,22 +3563,15 @@ u64 perf_event_read_local(struct perf_event *event)
 static int perf_event_read(struct perf_event *event, bool group)
 {
 	int ret = 0, cpu_to_read, local_cpu;
-
-	/*
-	 * If event is enabled and currently active on a CPU, update the
-	 * value in the event structure:
-	 */
-	if (event->state == PERF_EVENT_STATE_ACTIVE) {
+	local_cpu = get_cpu();
+	cpu_to_read = find_cpu_to_read(event, local_cpu);
+	if (cpu_to_read != -1) {
 		struct perf_read_data data = {
 			.event = event,
 			.group = group,
 			.ret = 0,
 		};
 
-		local_cpu = get_cpu();
-		cpu_to_read = find_cpu_to_read(event, local_cpu);
-		put_cpu();
-
 		ret = smp_call_function_single(cpu_to_read, __perf_event_read, &data, 1);
 		/* The event must have been read from an online CPU: */
 		WARN_ON_ONCE(ret);
@@ -3603,6 +3597,7 @@ static int perf_event_read(struct perf_event *event, bool group)
 		raw_spin_unlock_irqrestore(&ctx->lock, flags);
 	}
 
+	put_cpu();
 	return ret;
 }
 

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

#1472258

On Mon, Aug 29, 2016 at 08:58:41PM +0200, Jiri Olsa wrote:
> couldn't we just call smp_call_function_single(cpu_to_read, __perf_event_read, ...
> once we read oncpu != -1 ?
> 

Yes, and at that point we can simply revert this WARN, because that is
exactly the same.

Which I think is what I'm going to do.

If oncpu is not valid, the sched_out that made it invalid will have
updated the event count and we're good.

All I'll leave is an explicit comment that we've ignored the
smp_call_function_single() return value on purpose.

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

#1472272

On Tue, Aug 30, 2016 at 08:47:24AM +0200, Peter Zijlstra wrote:

> If oncpu is not valid, the sched_out that made it invalid will have
> updated the event count and we're good.
> 
> All I'll leave is an explicit comment that we've ignored the
> smp_call_function_single() return value on purpose.

Something like so..

---
 kernel/events/core.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 3f07e6cfc1b6..a35cbc382b2c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3576,12 +3576,21 @@ static int perf_event_read(struct perf_event *event, bool group)
 
 		local_cpu = get_cpu();
 		cpu_to_read = find_cpu_to_read(event, local_cpu);
+
+		/*
+		 * Purposely ignore the smp_call_function_single() return
+		 * value.
+		 *
+		 * If event->oncpu isn't a valid CPU it means the event got
+		 * scheduled out and that will have updated the event count.
+		 *
+		 * Therefore, either way, we'll have an up-to-date event count
+		 * after this.
+		 */
+		(void)smp_call_function_single(cpu_to_read, __perf_event_read, &data, 1);
 		put_cpu();
 
-		ret = smp_call_function_single(cpu_to_read, __perf_event_read, &data, 1);
-		/* The event must have been read from an online CPU: */
-		WARN_ON_ONCE(ret);
-		ret = ret ? : data.ret;
+		ret = data.ret;
 	} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
 		struct perf_event_context *ctx = event->ctx;
 		unsigned long flags;

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

#1472369

On Tue, Aug 30, 2016 at 09:26:03AM +0200, Peter Zijlstra wrote:
> On Tue, Aug 30, 2016 at 08:47:24AM +0200, Peter Zijlstra wrote:
> 
> > If oncpu is not valid, the sched_out that made it invalid will have
> > updated the event count and we're good.
> > 
> > All I'll leave is an explicit comment that we've ignored the
> > smp_call_function_single() return value on purpose.
> 
> Something like so..

yep, it works in my tests

also I thought there's no group time update in __perf_event_read,
so I was hunting that but then I noticed we do that after during
the read..  and meanwhile came to patch below ;-)

jirka


---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 305dbd28ea86..c637496251fe 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3440,6 +3440,26 @@ static int find_cpu_to_read(struct perf_event *event, int local_cpu)
 	return event_cpu;
 }
 
+static void read_update_times(struct perf_event *event, bool group)
+{
+	struct perf_event_context *ctx = event->ctx;
+
+	/*
+	 * may read while context is not active
+	 * (e.g., thread is blocked), in that case
+	 * we cannot update context time
+	 */
+	if (ctx->is_active) {
+		update_context_time(ctx);
+		update_cgrp_time_from_event(event);
+	}
+
+	if (group)
+		update_group_times(event);
+	else
+		update_event_times(event);
+}
+
 /*
  * Cross CPU call to read the hardware event
  */
@@ -3462,12 +3482,9 @@ static void __perf_event_read(void *info)
 		return;
 
 	raw_spin_lock(&ctx->lock);
-	if (ctx->is_active) {
-		update_context_time(ctx);
-		update_cgrp_time_from_event(event);
-	}
 
-	update_event_times(event);
+	read_update_times(event, data->group);
+
 	if (event->state != PERF_EVENT_STATE_ACTIVE)
 		goto unlock;
 
@@ -3482,7 +3499,6 @@ static void __perf_event_read(void *info)
 	pmu->read(event);
 
 	list_for_each_entry(sub, &event->sibling_list, group_entry) {
-		update_event_times(sub);
 		if (sub->state == PERF_EVENT_STATE_ACTIVE) {
 			/*
 			 * Use sibling's PMU rather than @event's since
@@ -3596,19 +3612,7 @@ static int perf_event_read(struct perf_event *event, bool group)
 		unsigned long flags;
 
 		raw_spin_lock_irqsave(&ctx->lock, flags);
-		/*
-		 * may read while context is not active
-		 * (e.g., thread is blocked), in that case
-		 * we cannot update context time
-		 */
-		if (ctx->is_active) {
-			update_context_time(ctx);
-			update_cgrp_time_from_event(event);
-		}
-		if (group)
-			update_group_times(event);
-		else
-			update_event_times(event);
+		read_update_times(event, group);
 		raw_spin_unlock_irqrestore(&ctx->lock, flags);
 	}
 

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

#1472624

Hi,

On Mon, Aug 29, 2016 at 6:02 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Aug 29, 2016 at 12:03:09PM +0200, Peter Zijlstra wrote:
> > @@ -1802,8 +1802,18 @@ event_sched_out(struct perf_event *event,
> >
> >       event->tstamp_stopped = tstamp;
> >       event->pmu->del(event, 0);
> > -     event->oncpu = -1;
> > -     event->state = PERF_EVENT_STATE_INACTIVE;
> > +
> > +     WRITE_ONCE(event->state, PERF_EVENT_STATE_INACTIVE);
> > +     /*
> > +      * pmu::del() will have updated the event count. Now mark it inactive,
> > +      * but take care to clear ->oncpu after the INACTIVE store, such that
> > +      * while ->state == ACTIVE, ->oncpu must be valid.
> > +      *
> > +      * See event_sched_in(), perf_event_restart() and perf_event_read().
> > +      */
> > +     smp_wmb();
> > +     WRITE_ONCE(event->oncpu, -1);
> > +
> >       if (event->pending_disable) {
> >               event->pending_disable = 0;
> >               event->state = PERF_EVENT_STATE_OFF;
> > @@ -2015,8 +2025,10 @@ event_sched_in(struct perf_event *event,
> >
> >       WRITE_ONCE(event->oncpu, smp_processor_id());
> >       /*
> > -      * Order event::oncpu write to happen before the ACTIVE state
> > -      * is visible.
> > +      * Order event::oncpu write to happen before the ACTIVE state is
> > +      * visible, such that when we observe ACTIVE, oncpu must be correct.
> > +      *
> > +      * Matches the smp_rmb() in perf_event_restart().
> >        */
> >       smp_wmb();
> >       WRITE_ONCE(event->state, PERF_EVENT_STATE_ACTIVE);
>
> Urgh.. that cannot work either, because now perf_event_read() can race
> against event_sched_in(). Since that's no longer crossed.
>
> > @@ -3561,28 +3576,36 @@ u64 perf_event_read_local(struct perf_event *event)
> >
> >  static int perf_event_read(struct perf_event *event, bool group)
> >  {
> > -     int ret = 0, cpu_to_read, local_cpu;
> > +     int ret = 0, cpu_to_read, local_cpu, state;
> > +
> > +     local_cpu = get_cpu(); /* disable preemption to hold off hotplut */
> > +     cpu_to_read = READ_ONCE(event->oncpu);
> > +     /*
> > +      * Matches smp_wmb() from event_sched_out(), ->oncpu must be valid
> > +      * IFF we observe ACTIVE.
> > +      */
> > +     smp_rmb();
> > +     state = READ_ONCE(event->state);
>
> The best I can come up with is something like:
>
>
>         do {
>                 state = READ_ONCE(event->state);
>                 if (state != ACTIVE)
>                         break;
>                 smp_rmb();
>                 cpu = READ_ONCE(event->cpu);
>                 smp_rmb();
>         } while (READ_ONCE(event->state) != state);
>
>
> And I suppose perf_event_restart() should do the same thing... Let me
> ponder this a wee bit more.


I am trying to understand this better. There is a race between
oncpu/active and the smp_call.
By the time you actually do the smp_call the oncpu may be wrong and
smp_call now returns
an error given David's change. I suspect the race was always there. It
boils down to what is
the guarantee of the API in terms of the "freshness" of the value
returned on read().
I am guessing that if you thought you had to do the smp_call, it is
because the event was still
active and oncpu != -1. If it is no longer active, it happened very
recently and, in that case, one can
use the saved count in the perf_event struct as a valid value because
it was necessarily updated
when the event was scheduled out.

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

#1474944

On Tue, Aug 30, 2016 at 09:26:18AM -0700, Stephane Eranian wrote:
> I am trying to understand this better.

> There is a race between oncpu/active and the smp_call.  By the time
> you actually do the smp_call the oncpu may be wrong and smp_call now
> returns an error given David's change.

> I suspect the race was always there. 

Me too, I might even have done it on purpose and then forgot about it.
Now cured with a comment.

> It boils down to what is the guarantee of the API in terms of the
> "freshness" of the value returned on read().  I am guessing that if
> you thought you had to do the smp_call, it is because the event was
> still active and oncpu != -1. 

> If it is no longer active, it happened very recently and, in that
> case, one can use the saved count in the perf_event struct as a valid
> value because it was necessarily updated when the event was scheduled
> out.

Almost, if its not active, its not counting. Therefore we don't care
about updates.

The other race, against sched_in(), is as you describe though, we can
observe ACTIVE && on_cpu==-1 or INACTIVE && on_cpu (due to lack of
ordering and serialization) but if we can observe that, the sched_in was
(very) recent and we still don't care because its the same as if the
read request happened slightly earlier etc..

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web