| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20110330130951.GA2124@jolsa.brq.redhat.com>
Date: Wed, 30 Mar 2011 15:09:51 +0200
From: Jiri Olsa <jolsa@...hat.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Paul Mackerras <paulus@...ba.org>, Ingo Molnar <mingo@...e.hu>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH,RFC] perf: panic due to inclied cpu context task_ctx value
On Mon, Mar 28, 2011 at 06:56:48PM +0200, Oleg Nesterov wrote:
> On 03/28, Peter Zijlstra wrote:
> >
> > Another fun race, suppose we do properly remove task_ctx and is_active,
> > but then the task gets scheduled back in before free_event() gets around
> > to disabling the jump_label..
>
> Yes, this too...
>
> Well, ignoring the HAVE_JUMP_LABEL case... perhaps we can split
> perf_sched_events into 2 counters? I mean,
>
> atomic_t perf_sched_events_in, perf_sched_events_out;
>
> static inline void perf_event_task_sched_in(struct task_struct *task)
> {
> COND_STMT(&perf_sched_events_in, __perf_event_task_sched_in(task));
> }
>
> static inline
> void perf_event_task_sched_out(struct task_struct *task, struct task_struct *next)
> {
> perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, 1, NULL, 0);
>
> COND_STMT(&perf_sched_events_out, __perf_event_task_sched_out(task, next));
> }
>
> void perf_sched_events_inc(void)
> {
> atomic_inc(&perf_sched_events_out);
> smp_mb__after_atomic_inc();
> atomic_inc(&perf_sched_events_in);
> }
>
> void perf_sched_events_dec(void)
> {
> if (atomic_dec_and_test(&perf_sched_events_in))
> synchronize_sched();
> atomic_dec(&perf_sched_events_out);
> }
>
> The last 2 helpers should be used instead of jump_label_inc/dec.
this approach seems to work, plz check attached patch
>
>
>
> Or we can remove COND_STMT() from perf_event_task_sched_out(). Say,
> __perf_event_task_sched_in() can set PF_PERF_XXX or something, then
> perf_event_task_sched_out() can check this bit.
>
>
> As for HAVE_JUMP_LABEL, I still can't understand why this test-case
> triggers the problem. But jump_label_inc/dec logic looks obviously
> racy.
I think the reason is, that there are actually 2 places that
needs to be updated by jump label code
- perf_event_task_sched_in
- perf_event_task_sched_out
As for my image it first patches perf_event_task_sched_out and then
perf_event_task_sched_in
Now, when the code is updated, migration thread is scheduled several times
and perf schedule callbacks are called.
This seems to be happening in my setup:
perf release:
- jump_label_dec(perf_sched_events)
- migration thread is scheduled for disabling perf_event_task_sched_out
jump label
- perf_event_task_sched_out/perf_event_task_sched_in is called
- task_ctx is NULL, since migration thread does not have any events
- perf_event_task_sched_out jump label got disabled
- release code is rescheduled -> now only perf_event_task_sched_in jump
label is called
- task_ctx is set
- migration thread is scheduled for disabling perf_event_task_sched_in
jump label
- perf_event_task_sched_in is called for migration thread,
task_ctx stays, since migration thread does not have any events
- perf_event_task_sched_in jump label is disabled
- release code is scheduled -> no sched callback is called
- task_ctx stays != NULL
I'm almost 5% sure it could happen as I described ;)
Back to the patch below:
IIUIC that calling synchronize_sched in perf_sched_events_dec
ensures that final schedule back to the release code will not
call perf_event_task_sched_in, so task_ctx stays sane (NULL).
The cool think is, that it also fixies the original issue,
which was: wrong counters for perf builtin test #3..
which I started with from the very beggining :)
please let me know what you think, thanks
jirka
---
include/linux/perf_event.h | 44 +++++++++++++++++++++++++++++++++++++++++---
kernel/perf_event.c | 11 ++++++-----
2 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 311b4dc..4b4f114 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1074,11 +1074,12 @@ have_event:
__perf_sw_event(event_id, nr, nmi, regs, addr);
}
-extern atomic_t perf_sched_events;
+extern atomic_t perf_sched_events_in;
+extern atomic_t perf_sched_events_out;
static inline void perf_event_task_sched_in(struct task_struct *task)
{
- COND_STMT(&perf_sched_events, __perf_event_task_sched_in(task));
+ COND_STMT(&perf_sched_events_in, __perf_event_task_sched_in(task));
}
static inline
@@ -1086,9 +1087,46 @@ void perf_event_task_sched_out(struct task_struct *task, struct task_struct *nex
{
perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, 1, NULL, 0);
- COND_STMT(&perf_sched_events, __perf_event_task_sched_out(task, next));
+ COND_STMT(&perf_sched_events_out, __perf_event_task_sched_out(task, next));
}
+#ifdef HAVE_JUMP_LABEL
+static inline
+void perf_sched_events_inc(void)
+{
+ jump_label_inc(&perf_sched_events_out);
+ smp_mb__after_atomic_inc();
+ jump_label_inc(&perf_sched_events_in);
+}
+
+static inline
+void perf_sched_events_dec(void)
+{
+ if (atomic_dec_and_test(&perf_sched_events_in)) {
+ jump_label_disable(&perf_sched_events_in);
+ synchronize_sched();
+ }
+
+ jump_label_dec(&perf_sched_events_out);
+}
+#else
+static inline
+void perf_sched_events_inc(void)
+{
+ atomic_inc(&perf_sched_events_out);
+ smp_mb__after_atomic_inc();
+ atomic_inc(&perf_sched_events_in);
+}
+
+static inline
+void perf_sched_events_dec(void)
+{
+ if (atomic_dec_and_test(&perf_sched_events_in))
+ synchronize_sched();
+ atomic_dec(&perf_sched_events_out);
+}
+#endif /* HAVE_JUMP_LABEL */
+
extern void perf_event_mmap(struct vm_area_struct *vma);
extern struct perf_guest_info_callbacks *perf_guest_cbs;
extern int perf_register_guest_info_callbacks(struct perf_guest_info_callbacks *callbacks);
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index c75925c..db38d6e 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -125,7 +125,8 @@ enum event_type_t {
* perf_sched_events : >0 events exist
* perf_cgroup_events: >0 per-cpu cgroup events exist on this cpu
*/
-atomic_t perf_sched_events __read_mostly;
+atomic_t perf_sched_events_in __read_mostly;
+atomic_t perf_sched_events_out __read_mostly;
static DEFINE_PER_CPU(atomic_t, perf_cgroup_events);
static atomic_t nr_mmap_events __read_mostly;
@@ -2894,7 +2895,7 @@ static void free_event(struct perf_event *event)
if (!event->parent) {
if (event->attach_state & PERF_ATTACH_TASK)
- jump_label_dec(&perf_sched_events);
+ perf_sched_events_dec();
if (event->attr.mmap || event->attr.mmap_data)
atomic_dec(&nr_mmap_events);
if (event->attr.comm)
@@ -2905,7 +2906,7 @@ static void free_event(struct perf_event *event)
put_callchain_buffers();
if (is_cgroup_event(event)) {
atomic_dec(&per_cpu(perf_cgroup_events, event->cpu));
- jump_label_dec(&perf_sched_events);
+ perf_sched_events_dec();
}
}
@@ -6248,7 +6249,7 @@ done:
if (!event->parent) {
if (event->attach_state & PERF_ATTACH_TASK)
- jump_label_inc(&perf_sched_events);
+ perf_sched_events_inc();
if (event->attr.mmap || event->attr.mmap_data)
atomic_inc(&nr_mmap_events);
if (event->attr.comm)
@@ -6490,7 +6491,7 @@ SYSCALL_DEFINE5(perf_event_open,
* - that may need work on context switch
*/
atomic_inc(&per_cpu(perf_cgroup_events, event->cpu));
- jump_label_inc(&perf_sched_events);
+ perf_sched_events_inc();
}
/*
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists