BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Borislav Petkov]

Hi,

so I was tracing a small .c program like so:

$ ./perf record ~/test/asm

and here's what got spewed in dmesg. Kernel is plain -rc4. Any pending
fixes in tip I should try?

[ 3700.194208] BUG: using smp_processor_id() in preemptible [00000000] code: asm/8333
[ 3700.194226] caller is perf_event_task_ctx+0x55/0x140
[ 3700.194235] Pid: 8333, comm: asm Not tainted 3.9.0-rc4 #1
[ 3700.194243] Call Trace:
[ 3700.194261]  [<ffffffff812b4182>] debug_smp_processor_id+0xd2/0xf0
[ 3700.194279]  [<ffffffff810e5155>] perf_event_task_ctx+0x55/0x140
[ 3700.194296]  [<ffffffff810e5412>] perf_event_task+0x1d2/0x3c0
[ 3700.194308]  [<ffffffff810e52ad>] ? perf_event_task+0x6d/0x3c0
[ 3700.194324]  [<ffffffff810e99a6>] perf_event_exit_task+0x166/0x250
[ 3700.194333]  [<ffffffff810414be>] do_exit+0x27e/0xb20
[ 3700.194340]  [<ffffffff8159ac63>] ? error_sti+0x5/0x6
[ 3700.194349]  [<ffffffff8159a80d>] ? retint_swapgs+0xe/0x13
[ 3700.194358]  [<ffffffff81041f1c>] do_group_exit+0x4c/0xc0
[ 3700.194365]  [<ffffffff81041fa7>] sys_exit_group+0x17/0x20
[ 3700.194373]  [<ffffffff8159b306>] system_call_fastpath+0x1a/0x1f

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Borislav Petkov]

On Sun, Mar 24, 2013 at 12:55:56PM +0100, Borislav Petkov wrote:
> Hi,
> 
> so I was tracing a small .c program like so:
> 
> $ ./perf record ~/test/asm
> 
> and here's what got spewed in dmesg. Kernel is plain -rc4. Any pending
> fixes in tip I should try?
> 
> [ 3700.194208] BUG: using smp_processor_id() in preemptible [00000000] code: asm/8333
> [ 3700.194226] caller is perf_event_task_ctx+0x55/0x140

Ok, here's the call stack I was able to reconstruct:

perf_event_exit_task()
|->perf_event_exit_task_context()
   |-> perf_event_task()
       |-> perf_event_task_event()
	   |-> perf_event_task_ctx()
	       |-> perf_event_task_match()
		   |-> event_filter_match()
		       |-> smp_processor_id() -> debug_smp_processor_id()

Now, my primitive thinking would presume that since we're on the
exit_task path, we're still on the same cpu so the check triggers
wrongly but I don't know for sure. Also, is there any possibility for
this code to be moved somewhere else, i.e. to another cpu, so that the
check actually is correct?

Hmm.

Adding Stephane who added that check in
fa66f07aa1f0950e1dc78b7ab39728b3f8aa77a1.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Borislav Petkov]

On Sun, Mar 24, 2013 at 04:59:24PM +0100, Borislav Petkov wrote:
> On Sun, Mar 24, 2013 at 12:55:56PM +0100, Borislav Petkov wrote:
> > Hi,
> > 
> > so I was tracing a small .c program like so:
> > 
> > $ ./perf record ~/test/asm
> > 
> > and here's what got spewed in dmesg. Kernel is plain -rc4. Any pending
> > fixes in tip I should try?
> > 
> > [ 3700.194208] BUG: using smp_processor_id() in preemptible [00000000] code: asm/8333
> > [ 3700.194226] caller is perf_event_task_ctx+0x55/0x140
> 
> Ok, here's the call stack I was able to reconstruct:
> 
> perf_event_exit_task()
> |->perf_event_exit_task_context()
>    |-> perf_event_task()
>        |-> perf_event_task_event()
> 	   |-> perf_event_task_ctx()
> 	       |-> perf_event_task_match()
> 		   |-> event_filter_match()
> 		       |-> smp_processor_id() -> debug_smp_processor_id()

Ok, jolsa just rootcaused it: It is caused by
d610d98b5de6860feb21539726e9af7c9094151c calling perf_event_task_ctx()
outside of the preempt-safe protection.

There's a straightforward fix below, what to people think?

--
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7b4a55d41efc..f3bb3384a106 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4455,8 +4455,11 @@ static void perf_event_task_event(struct perf_task_event *task_event)
 next:
 		put_cpu_ptr(pmu->pmu_cpu_context);
 	}
+
+	preempt_disable();
 	if (task_event->task_ctx)
 		perf_event_task_ctx(task_event->task_ctx, task_event);
+	preempt_enable();
 
 	rcu_read_unlock();
 }


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Namhyung Kim]

Hi Borislav,

On Tue, 26 Mar 2013 19:34:52 +0100, Borislav Petkov wrote:
> On Sun, Mar 24, 2013 at 04:59:24PM +0100, Borislav Petkov wrote:
>> On Sun, Mar 24, 2013 at 12:55:56PM +0100, Borislav Petkov wrote:
>> > Hi,
>> > 
>> > so I was tracing a small .c program like so:
>> > 
>> > $ ./perf record ~/test/asm
>> > 
>> > and here's what got spewed in dmesg. Kernel is plain -rc4. Any pending
>> > fixes in tip I should try?
>> > 
>> > [ 3700.194208] BUG: using smp_processor_id() in preemptible [00000000] code: asm/8333
>> > [ 3700.194226] caller is perf_event_task_ctx+0x55/0x140
>> 
>> Ok, here's the call stack I was able to reconstruct:
>> 
>> perf_event_exit_task()
>> |->perf_event_exit_task_context()
>>    |-> perf_event_task()
>>        |-> perf_event_task_event()
>> 	   |-> perf_event_task_ctx()
>> 	       |-> perf_event_task_match()
>> 		   |-> event_filter_match()
>> 		       |-> smp_processor_id() -> debug_smp_processor_id()
>
> Ok, jolsa just rootcaused it: It is caused by
> d610d98b5de6860feb21539726e9af7c9094151c calling perf_event_task_ctx()
> outside of the preempt-safe protection.

Oops, my bad.  Forgot to enable CONFIG_DEBUG_PREEMPT when testing. :(

>
> There's a straightforward fix below, what to people think?

Looks okay to me.  Thanks for fixing this!
Namhyung

>
> --
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 7b4a55d41efc..f3bb3384a106 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4455,8 +4455,11 @@ static void perf_event_task_event(struct perf_task_event *task_event)
>  next:
>  		put_cpu_ptr(pmu->pmu_cpu_context);
>  	}
> +
> +	preempt_disable();
>  	if (task_event->task_ctx)
>  		perf_event_task_ctx(task_event->task_ctx, task_event);
> +	preempt_enable();
>  
>  	rcu_read_unlock();
>  }

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Borislav Petkov]

On Wed, Mar 27, 2013 at 03:02:10PM +0900, Namhyung Kim wrote:
> > --
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 7b4a55d41efc..f3bb3384a106 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -4455,8 +4455,11 @@ static void perf_event_task_event(struct perf_task_event *task_event)
> >  next:
> >  		put_cpu_ptr(pmu->pmu_cpu_context);
> >  	}
> > +
> > +	preempt_disable();
> >  	if (task_event->task_ctx)
> >  		perf_event_task_ctx(task_event->task_ctx, task_event);
> > +	preempt_enable();

Ok, just for my own understanding: how do the events on the
->task_ctx->event_list relate to the current cpu in this path? I mean,
we're on the task exit path here so is it possible to be rescheduled
somewhere else and the check in event_filter_match to become
meaningless?

Because with this fix, we have a small window between enabling
preemption after the last pmu context and disabling it again to get
moved somewhere else.

Hmm.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Jiri Olsa]

On Wed, Mar 27, 2013 at 10:49:32AM +0100, Borislav Petkov wrote:
> On Wed, Mar 27, 2013 at 03:02:10PM +0900, Namhyung Kim wrote:
> > > --
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index 7b4a55d41efc..f3bb3384a106 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -4455,8 +4455,11 @@ static void perf_event_task_event(struct perf_task_event *task_event)
> > >  next:
> > >  		put_cpu_ptr(pmu->pmu_cpu_context);
> > >  	}
> > > +
> > > +	preempt_disable();
> > >  	if (task_event->task_ctx)
> > >  		perf_event_task_ctx(task_event->task_ctx, task_event);
> > > +	preempt_enable();
> 
> Ok, just for my own understanding: how do the events on the
> ->task_ctx->event_list relate to the current cpu in this path? I mean,
> we're on the task exit path here so is it possible to be rescheduled
> somewhere else and the check in event_filter_match to become
> meaningless?
> 
> Because with this fix, we have a small window between enabling
> preemption after the last pmu context and disabling it again to get
> moved somewhere else.

hm, at that point we have 'task_event->task_ctx' detached from
the task and all events on it are owned by the task

so I wonder maybe we could eliminate the preempt switch and make
a special case for task context, saying all event match for it,
something like in patch below (untested)

because if we keep the check we could endup with no EXIT event,
just because the exit code was scheduled on another cpu
(for task events with specified cpu)

also that's probably what we want to do in here - send EXIT
event for any task event that asked for it

the output code in perf_event_task_output is guarding
the preemption by itself

jirka


---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7b4a55d..8414bcf 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4403,12 +4403,12 @@ out:
 	task_event->event_id.header.size = size;
 }
 
-static int perf_event_task_match(struct perf_event *event)
+static int perf_event_task_match(struct perf_event *event, int match)
 {
 	if (event->state < PERF_EVENT_STATE_INACTIVE)
 		return 0;
 
-	if (!event_filter_match(event))
+	if (!match && !event_filter_match(event))
 		return 0;
 
 	if (event->attr.comm || event->attr.mmap ||
@@ -4419,12 +4419,13 @@ static int perf_event_task_match(struct perf_event *event)
 }
 
 static void perf_event_task_ctx(struct perf_event_context *ctx,
-				  struct perf_task_event *task_event)
+				  struct perf_task_event *task_event,
+				int match)
 {
 	struct perf_event *event;
 
 	list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
-		if (perf_event_task_match(event))
+		if (perf_event_task_match(event, match))
 			perf_event_task_output(event, task_event);
 	}
 }
@@ -4441,7 +4442,7 @@ static void perf_event_task_event(struct perf_task_event *task_event)
 		cpuctx = get_cpu_ptr(pmu->pmu_cpu_context);
 		if (cpuctx->unique_pmu != pmu)
 			goto next;
-		perf_event_task_ctx(&cpuctx->ctx, task_event);
+		perf_event_task_ctx(&cpuctx->ctx, task_event, 0);
 
 		ctx = task_event->task_ctx;
 		if (!ctx) {
@@ -4450,13 +4451,13 @@ static void perf_event_task_event(struct perf_task_event *task_event)
 				goto next;
 			ctx = rcu_dereference(current->perf_event_ctxp[ctxn]);
 			if (ctx)
-				perf_event_task_ctx(ctx, task_event);
+				perf_event_task_ctx(ctx, task_event, 0);
 		}
 next:
 		put_cpu_ptr(pmu->pmu_cpu_context);
 	}
 	if (task_event->task_ctx)
-		perf_event_task_ctx(task_event->task_ctx, task_event);
+		perf_event_task_ctx(task_event->task_ctx, task_event, 1);
 
 	rcu_read_unlock();
 }

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Joerg Roedel]

On Tue, Mar 26, 2013 at 07:34:52PM +0100, Borislav Petkov wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 7b4a55d41efc..f3bb3384a106 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4455,8 +4455,11 @@ static void perf_event_task_event(struct perf_task_event *task_event)
>  next:
>  		put_cpu_ptr(pmu->pmu_cpu_context);
>  	}
> +
> +	preempt_disable();
>  	if (task_event->task_ctx)
>  		perf_event_task_ctx(task_event->task_ctx, task_event);
> +	preempt_enable();
>  
>  	rcu_read_unlock();
>  }

What makes me wonder here is that the code is preemptible in an
rcu_read_locked section. As far as I know preemption needs to be
disabled while holding the rcu_read_lock().


	Joerg

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Peter Zijlstra]

On Wed, 2013-03-27 at 10:49 +0100, Borislav Petkov wrote:
> Ok, just for my own understanding: how do the events on the
> ->task_ctx->event_list relate to the current cpu in this path? I mean,
> we're on the task exit path here so is it possible to be rescheduled
> somewhere else and the check in event_filter_match to become
> meaningless?

Events can be per-cpu, so what could happen is that we'd send the exit
notification to a cpu we're not actually running on (anymore).

Furthermore, since we evaluate smp_processor_id() for every
event_filter_match(), it is possible we'd send the notification to
multiple events if our task migrates just right.

Also, I suspect we want something like preempt_disable_notrace() to be
sure we don't recurse or something daft like that... but I'm not
entirely sure.

---
 kernel/events/core.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7b4a55d..0097d81 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4444,19 +4444,22 @@ static void perf_event_task_event(struct perf_task_event *task_event)
 		perf_event_task_ctx(&cpuctx->ctx, task_event);
 
 		ctx = task_event->task_ctx;
-		if (!ctx) {
-			ctxn = pmu->task_ctx_nr;
-			if (ctxn < 0)
-				goto next;
-			ctx = rcu_dereference(current->perf_event_ctxp[ctxn]);
-			if (ctx)
-				perf_event_task_ctx(ctx, task_event);
-		}
+		if (ctx)
+			goto next;
+		ctxn = pmu->task_ctx_nr;
+		if (ctxn < 0)
+			goto next;
+		ctx = rcu_dereference(current->perf_event_ctxp[ctxn]);
+		if (ctx)
+			perf_event_task_ctx(ctx, task_event);
 next:
 		put_cpu_ptr(pmu->pmu_cpu_context);
 	}
-	if (task_event->task_ctx)
+	if (task_event->task_ctx) {
+		preempt_disable_notrace();
 		perf_event_task_ctx(task_event->task_ctx, task_event);
+		preempt_enable_notrace();
+	}
 
 	rcu_read_unlock();
 }

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Peter Zijlstra]

On Wed, 2013-03-27 at 14:15 +0100, Joerg Roedel wrote:
> What makes me wonder here is that the code is preemptible in an
> rcu_read_locked section. As far as I know preemption needs to be
> disabled while holding the rcu_read_lock().

Nah, a long long time ago some -rt people complained to paulmck that
keeping preemption disabled over all this RCU stuff was killing
latencies. Paul liked the challenge and came up with some mind twisting
stuff to make it work.

If you're into that kind of pain, look at CONFIG_*_PREEMPT_RCU :-)

But yeah, you need to have that stuff enabled before you can hit this
particular snag.

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Paul E. McKenney]

On Wed, Mar 27, 2013 at 03:17:32PM +0100, Peter Zijlstra wrote:
> On Wed, 2013-03-27 at 14:15 +0100, Joerg Roedel wrote:
> > What makes me wonder here is that the code is preemptible in an
> > rcu_read_locked section. As far as I know preemption needs to be
> > disabled while holding the rcu_read_lock().
> 
> Nah, a long long time ago some -rt people complained to paulmck that
> keeping preemption disabled over all this RCU stuff was killing
> latencies. Paul liked the challenge and came up with some mind twisting
> stuff to make it work.

What can I say?  I was young and foolish.  And I still am pretty
foolish.  ;-)

But yes, you are not required to disable preemption across
rcu_read_lock(), and rcu_read_lock() is not guaranteed to disable
preemption.  So if you need preemption to be disabled, do it explicitly
with preempt_disable(), local_irq_save(), rcu_read_lock_sched(),
or whatever, because rcu_read_lock() isn't always going to disable
preemption.

> If you're into that kind of pain, look at CONFIG_*_PREEMPT_RCU :-)

Or just set CONFIG_PREEMPT=y, which will set CONFIG_TREE_PREEMPT_RCU=y
on CONFIG_SMP=y builds and will set CONFIG_TINY_PREEMPT_RCU=y otherwise.
(But please note that CONFIG_TINY_PREEMPT_RCU is going away, after which
CONFIG_PREEMPT=y will always set CONFIG_TREE_PREEMPT_RCU=y.)

							Thanx, Paul

> But yeah, you need to have that stuff enabled before you can hit this
> particular snag.
> 

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Borislav Petkov]

On Wed, Mar 27, 2013 at 03:09:32PM +0100, Peter Zijlstra wrote:
> On Wed, 2013-03-27 at 10:49 +0100, Borislav Petkov wrote:
> > Ok, just for my own understanding: how do the events on the
> > ->task_ctx->event_list relate to the current cpu in this path? I mean,
> > we're on the task exit path here so is it possible to be rescheduled
> > somewhere else and the check in event_filter_match to become
> > meaningless?
> 
> Events can be per-cpu, so what could happen is that we'd send the exit
> notification to a cpu we're not actually running on (anymore).

Right, but we still have a window of preemption enabled:

		put_cpu_ptr(pmu->pmu_cpu_context);	<---|
     	}						    |
	if (task_event->task_ctx) {			    |
		preempt_disable_notrace();		<---|

between the two arrows above.

I don't know the code all that well to know whether a reschedule in that
window would make us, when looking at the current cpu when matching the
events, look stupid or not...

OTOH, I can understand jolsa's approach of declaring the events on
->task_ctx->event_list for belonging to this task only and that then we
just don't want to compare event->cpu to the current cpu we're running
on.

But in the end of the day, I'd like you guys to decide what is the right
thing to do here.

Btw, jolsa, as a simplification to your solution, you could simply do:

	if (task_event->task_ctx)
		list_for_each_entry_rcu(event, &ctx->event_list, event_entry)
			perf_event_task_output(event, task_event);

and avoid adding a 'match' argument.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Joerg Roedel]

On Wed, Mar 27, 2013 at 07:37:15AM -0700, Paul E. McKenney wrote:
> On Wed, Mar 27, 2013 at 03:17:32PM +0100, Peter Zijlstra wrote:
> > On Wed, 2013-03-27 at 14:15 +0100, Joerg Roedel wrote:
> > > What makes me wonder here is that the code is preemptible in an
> > > rcu_read_locked section. As far as I know preemption needs to be
> > > disabled while holding the rcu_read_lock().
> > 
> > Nah, a long long time ago some -rt people complained to paulmck that
> > keeping preemption disabled over all this RCU stuff was killing
> > latencies. Paul liked the challenge and came up with some mind twisting
> > stuff to make it work.
> 
> What can I say?  I was young and foolish.  And I still am pretty
> foolish.  ;-)
> 
> But yes, you are not required to disable preemption across
> rcu_read_lock(), and rcu_read_lock() is not guaranteed to disable
> preemption.  So if you need preemption to be disabled, do it explicitly
> with preempt_disable(), local_irq_save(), rcu_read_lock_sched(),
> or whatever, because rcu_read_lock() isn't always going to disable
> preemption.
> 
> > If you're into that kind of pain, look at CONFIG_*_PREEMPT_RCU :-)
> 
> Or just set CONFIG_PREEMPT=y, which will set CONFIG_TREE_PREEMPT_RCU=y
> on CONFIG_SMP=y builds and will set CONFIG_TINY_PREEMPT_RCU=y otherwise.
> (But please note that CONFIG_TINY_PREEMPT_RCU is going away, after which
> CONFIG_PREEMPT=y will always set CONFIG_TREE_PREEMPT_RCU=y.)
> 
> 							Thanx, Paul
> 
> > But yeah, you need to have that stuff enabled before you can hit this
> > particular snag.

Interesting read, thanks guys. I think I should have a look into the
tree-preempt implementation and try to understand it :)


	Joerg

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Borislav Petkov]

On Wed, Mar 27, 2013 at 05:34:27PM +0100, Joerg Roedel wrote:
> Interesting read, thanks guys. I think I should have a look into the
> tree-preempt implementation and try to understand it :)

Hurry, before paulmck comes and changes it unrecognizable! :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Paul E. McKenney]

On Wed, Mar 27, 2013 at 05:38:06PM +0100, Borislav Petkov wrote:
> On Wed, Mar 27, 2013 at 05:34:27PM +0100, Joerg Roedel wrote:
> > Interesting read, thanks guys. I think I should have a look into the
> > tree-preempt implementation and try to understand it :)
> 
> Hurry, before paulmck comes and changes it unrecognizable! :-)

Better yet, review my changes for 3.10.  ;-)

							Thanx, Paul

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Borislav Petkov]

On Wed, Mar 27, 2013 at 11:04:55AM -0700, Paul E. McKenney wrote:
> On Wed, Mar 27, 2013 at 05:38:06PM +0100, Borislav Petkov wrote:
> > On Wed, Mar 27, 2013 at 05:34:27PM +0100, Joerg Roedel wrote:
> > > Interesting read, thanks guys. I think I should have a look into the
> > > tree-preempt implementation and try to understand it :)
> > 
> > Hurry, before paulmck comes and changes it unrecognizable! :-)
> 
> Better yet, review my changes for 3.10.  ;-)

Oh, and if one needs to brush up on RCU while doing that, there's this
cool thing called perfbook (whole title is too long :)) I discovered
while searching for your 3.10 queue on k.org. :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Borislav Petkov]

On Wed, Mar 27, 2013 at 08:07:27PM +0100, Borislav Petkov wrote:
> Oh, and if one needs to brush up on RCU while doing that, there's this
> cool thing called perfbook (whole title is too long :)) I discovered
> while searching for your 3.10 queue on k.org. :-)

And here's me contributing :-):

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 27 Mar 2013 20:18:03 +0100
Subject: [PATCH] Deferred Processing: Fix a typo

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 defer/defer.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/defer/defer.tex b/defer/defer.tex
index cedc86e13e5c..9895b6b5fce9 100644
--- a/defer/defer.tex
+++ b/defer/defer.tex
@@ -4,7 +4,7 @@
 
 The strategy of deferring work goes back before the dawn of recorded
 history, and has generally been derided as procrastination or
-even shear laziness.
+even sheer laziness.
 However, in the last few decades have workers recognized this strategy's value
 in simplifying and streamlining parallel algorithms~\cite{Kung80,HMassalinPhD}.
 Believe it or not, in parallel programming, laziness often performs and
-- 
1.8.2.135.g7b592fa

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Paul E. McKenney]

On Wed, Mar 27, 2013 at 08:07:27PM +0100, Borislav Petkov wrote:
> On Wed, Mar 27, 2013 at 11:04:55AM -0700, Paul E. McKenney wrote:
> > On Wed, Mar 27, 2013 at 05:38:06PM +0100, Borislav Petkov wrote:
> > > On Wed, Mar 27, 2013 at 05:34:27PM +0100, Joerg Roedel wrote:
> > > > Interesting read, thanks guys. I think I should have a look into the
> > > > tree-preempt implementation and try to understand it :)
> > > 
> > > Hurry, before paulmck comes and changes it unrecognizable! :-)
> > 
> > Better yet, review my changes for 3.10.  ;-)
> 
> Oh, and if one needs to brush up on RCU while doing that, there's this
> cool thing called perfbook (whole title is too long :)) I discovered
> while searching for your 3.10 queue on k.org. :-)

Reviews of perfbook are also welcome.  ;-)

In case you didn't find it, the 3.10 queue is at rcu/next in:

	git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git

The corresponding LKML posts may be found at:

	https://lkml.org/lkml/2013/3/18/960
	https://lkml.org/lkml/2013/3/18/570
	https://lkml.org/lkml/2013/3/18/594

							Thanx, Paul

[RFCv2] Re: BUG: using smp_processor_id() in preemptible [00000000] code: asm/8267

[Jiri Olsa]

On Wed, Mar 27, 2013 at 03:09:32PM +0100, Peter Zijlstra wrote:
> On Wed, 2013-03-27 at 10:49 +0100, Borislav Petkov wrote:
> > Ok, just for my own understanding: how do the events on the
> > ->task_ctx->event_list relate to the current cpu in this path? I mean,
> > we're on the task exit path here so is it possible to be rescheduled
> > somewhere else and the check in event_filter_match to become
> > meaningless?
> 
> Events can be per-cpu, so what could happen is that we'd send the exit
> notification to a cpu we're not actually running on (anymore).
> 
> Furthermore, since we evaluate smp_processor_id() for every
> event_filter_match(), it is possible we'd send the notification to
> multiple events if our task migrates just right.
> 
> Also, I suspect we want something like preempt_disable_notrace() to be
> sure we don't recurse or something daft like that... but I'm not
> entirely sure.

hum, I did not find reason for using the *_notrace stuff in here
also we use preempt_disable indirectly via get_cpu_ptr

sending updated patch.. tests looks ok so far

thanks for comments,
jirka


---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 82c01bf..bdcbda8 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4459,7 +4459,7 @@ static void perf_event_task_ctx(struct perf_event_context *ctx,
 static void perf_event_task_event(struct perf_task_event *task_event)
 {
 	struct perf_cpu_context *cpuctx;
-	struct perf_event_context *ctx;
+	struct perf_event_context *ctx, *task_ctx = task_event->task_ctx;
 	struct pmu *pmu;
 	int ctxn;
 
@@ -4470,20 +4470,22 @@ static void perf_event_task_event(struct perf_task_event *task_event)
 			goto next;
 		perf_event_task_ctx(&cpuctx->ctx, task_event);
 
-		ctx = task_event->task_ctx;
-		if (!ctx) {
-			ctxn = pmu->task_ctx_nr;
-			if (ctxn < 0)
-				goto next;
-			ctx = rcu_dereference(current->perf_event_ctxp[ctxn]);
-			if (ctx)
-				perf_event_task_ctx(ctx, task_event);
-		}
+		if (task_ctx)
+			goto next;
+		ctxn = pmu->task_ctx_nr;
+		if (ctxn < 0)
+			goto next;
+		ctx = rcu_dereference(current->perf_event_ctxp[ctxn]);
+		if (ctx)
+			perf_event_task_ctx(ctx, task_event);
 next:
 		put_cpu_ptr(pmu->pmu_cpu_context);
 	}
-	if (task_event->task_ctx)
-		perf_event_task_ctx(task_event->task_ctx, task_event);
+	if (task_ctx) {
+		preempt_disable();
+		perf_event_task_ctx(task_ctx, task_event);
+		preempt_enable();
+	}
 
 	rcu_read_unlock();
 }