---
 arch/x86/kernel/cpu/perf_counter.c |    6 ++++++
 include/linux/perf_counter.h       |    1 +
 kernel/perf_counter.c              |   27 ++++++++++++++++++++-------
 3 files changed, 27 insertions(+), 7 deletions(-)

arch/x86/kernel/cpu/perf_counter.c +6 -0

Index: linux-2.6/arch/x86/kernel/cpu/perf_counter.c
===================================================================
@@ -2153,7 +2153,13 @@ static const struct stacktrace_ops backt
 static void
 perf_callchain_kernel(struct pt_regs *regs, struct perf_callchain_entry *entry)
 {
+	u64 anon_ip = perf_paranoid_anon_ip();
+
 	callchain_store(entry, PERF_CONTEXT_KERNEL);
+	if (anon_ip) {
+		callchain_store(entry, anon_ip);
+		return;
+	}
 	callchain_store(entry, regs->ip);
 
 	dump_trace(NULL, regs, NULL, 0, &backtrace_ops, entry);

include/linux/perf_counter.h +1 -0

Index: linux-2.6/include/linux/perf_counter.h
===================================================================
@@ -754,6 +754,7 @@ static inline void perf_counter_mmap(str
 extern void perf_counter_comm(struct task_struct *tsk);
 extern void perf_counter_fork(struct task_struct *tsk);
 
+extern unsigned long perf_paranoid_anon_ip(void);
 extern struct perf_callchain_entry *perf_callchain(struct pt_regs *regs);
 
 extern int sysctl_perf_counter_paranoid;

kernel/perf_counter.c +20 -7

Index: linux-2.6/kernel/perf_counter.c
===================================================================
@@ -48,18 +48,29 @@ static atomic_t nr_task_counters __read_
  * perf counter paranoia level:
  *  0 - not paranoid
  *  1 - disallow cpu counters to unpriv
- *  2 - disallow kernel profiling to unpriv
+ *  2 - anonymize kernel RIPs to unpriv
+ *  3 - disallow kernel profiling to unpriv
  */
-int sysctl_perf_counter_paranoid __read_mostly;
+int sysctl_perf_counter_paranoid __read_mostly = 2;
 
 static inline bool perf_paranoid_cpu(void)
 {
-	return sysctl_perf_counter_paranoid > 0;
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 0;
+}
+
+static inline bool perf_paranoid_anon(void)
+{
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 1;
 }
 
 static inline bool perf_paranoid_kernel(void)
 {
-	return sysctl_perf_counter_paranoid > 1;
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 2;
+}
+
+unsigned long perf_paranoid_anon_ip(void)
+{
+	return perf_paranoid_anon() ? _THIS_IP_ : 0;
 }
 
 int sysctl_perf_counter_mlock __read_mostly = 512; /* 'free' kb per user */

@@ -1571,7 +1582,7 @@ static struct perf_counter_context *find
 	 */
 	if (cpu != -1) {
 		/* Must be root to operate on a CPU counter: */
-		if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
+		if (perf_paranoid_cpu())
 			return ERR_PTR(-EACCES);
 
 		if (cpu < 0 || cpu > num_possible_cpus())

@@ -2841,7 +2852,9 @@ void perf_counter_output(struct perf_cou
 	header.misc |= perf_misc_flags(data->regs);
 
 	if (sample_type & PERF_SAMPLE_IP) {
-		ip = perf_instruction_pointer(data->regs);
+		ip = perf_paranoid_anon_ip();
+		if (!ip || user_mode(data->regs))
+			ip = perf_instruction_pointer(data->regs);
 		header.size += sizeof(ip);
 	}

@@ -4227,7 +4240,7 @@ SYSCALL_DEFINE5(perf_counter_open,
 		return ret;
 
 	if (!attr.exclude_kernel) {
-		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
+		if (perf_paranoid_kernel())
 			return -EACCES;
 	}

- capable() uses current, which is unlikely to be counter->owner,
- but even security_real_capable(counter->owner, ...) wouldn't work, since the ->capable() callback isn't NMI safe (selinux takes locks and does allocations in that path).

This puts a severe strain on more complex anonymizers since its basically impossible to tell if counter->owner has permissions on current from NMI context.

I'll fix up this patch to pre-compute the perf_paranoid_anon_ip() per counter based on creation time state, unless somebody has a better idea.

I could possibly only anonymize IRQ context (SoftIRQ context is difficult since in_softirq() means both in-softirq and softirq-disabled).

Frederic Weisbecker

2009-08-19 16:04 UTC #3 lore raw

Re: [PATCH 1/4] perf_counter: Default to higher paranoia level

Quoted 20 lines · Peter Zijlstra

kernel/perf_counter.c static atomic_t nr_task_counters __read_ · line 69
Quoted 10 lines · Peter Zijlstra
kernel/perf_counter.c +5 -1
+static inline bool perf_paranoid_anon(void)
+{
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 1;
 }
 
 static inline bool perf_paranoid_kernel(void)
 {
-	return sysctl_perf_counter_paranoid > 1;
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 2;
+}
OK, this is buggy:
- capable() uses current, which is unlikely to be counter->owner,
- but even security_real_capable(counter->owner, ...) wouldn't work, since the ->capable() callback isn't NMI safe (selinux takes locks and does allocations in that path).

Quoted 6 lines

This puts a severe strain on more complex anonymizers since its basically impossible to tell if counter->owner has permissions on current from NMI context.
I'll fix up this patch to pre-compute the perf_paranoid_anon_ip() per counter based on creation time state, unless somebody has a better idea.

Something I don't understand there: it's about wide per cpu profiling, then the task that have been created before the counter can also be profiled, then how is the creation time useful here?

Quoted 3 lines

I could possibly only anonymize IRQ context (SoftIRQ context is difficult since in_softirq() means both in-softirq and softirq-disabled).

I don't understand why we need to set this paranoid level concerning kernel RIPS.

Peter Zijlstra

maintainer

2009-08-20 12:00 UTC #4 lore raw

Re: [PATCH 1/4] perf_counter: Default to higher paranoia level

In reply to Peter Zijlstra

Quoted 24 lines · Peter Zijlstra

Review
Quoted 10 lines · Peter Zijlstra
(quoted diff) +5 -1
+static inline bool perf_paranoid_anon(void)
+{
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 1;
 }
 
 static inline bool perf_paranoid_kernel(void)
 {
-	return sysctl_perf_counter_paranoid > 1;
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 2;
+}
OK, this is buggy:
- capable() uses current, which is unlikely to be counter->owner,
- but even security_real_capable(counter->owner, ...) wouldn't work, since the ->capable() callback isn't NMI safe (selinux takes locks and does allocations in that path).
This puts a severe strain on more complex anonymizers since its basically impossible to tell if counter->owner has permissions on current from NMI context.

Quoted 6 lines

I'll fix up this patch to pre-compute the perf_paranoid_anon_ip() per counter based on creation time state, unless somebody has a better idea.
I could possibly only anonymize IRQ context (SoftIRQ context is difficult since in_softirq() means both in-softirq and softirq-disabled).

Something like the below maybe.. its 3 patches folded and I've no clue how to adapt the ppc code, what do people think?

compile tested on x86-64 only.

---
 arch/x86/include/asm/stacktrace.h  |   14 +++++--
 arch/x86/kernel/cpu/perf_counter.c |   66 +++++++++++++++++++++++++++++++----
 arch/x86/kernel/dumpstack.c        |   10 +++--
 arch/x86/kernel/dumpstack_32.c     |    8 +----
 arch/x86/kernel/dumpstack_64.c     |   13 ++-----
 arch/x86/kernel/stacktrace.c       |    8 +++--
 arch/x86/oprofile/backtrace.c      |    5 ++-
 include/linux/perf_counter.h       |    5 ++-
 kernel/perf_counter.c              |   30 ++++++++++++----
 kernel/trace/trace_sysprof.c       |    5 ++-
 10 files changed, 117 insertions(+), 47 deletions(-)

arch/x86/include/asm/stacktrace.h +10 -4

index cf86a5e..7066caa 100644
@@ -3,17 +3,23 @@
 
 extern int kstack_depth_to_print;
 
-int x86_is_stack_id(int id, char *name);
-
 /* Generic stack tracer with callbacks */
 
+enum stack_type {
+	STACK_UNKNOWN	= -1,
+	STACK_PROCESS   = 0,
+	STACK_INTERRUPT = 1,
+	STACK_EXCEPTION = 2,
+};
+
 struct stacktrace_ops {
 	void (*warning)(void *data, char *msg);
 	/* msg must contain %s for the symbol */
 	void (*warning_symbol)(void *data, char *msg, unsigned long symbol);
-	void (*address)(void *data, unsigned long address, int reliable);
+	void (*address)(void *data, unsigned long stack,
+			unsigned long address, int reliable);
 	/* On negative return stop dumping */
-	int (*stack)(void *data, char *name);
+	int (*stack)(void *data, int type, char *name);
 };
 
 void dump_trace(struct task_struct *tsk, struct pt_regs *regs,

arch/x86/kernel/cpu/perf_counter.c +58 -8

107 lines not on this page · raw message

arch/x86/kernel/dumpstack.c +6 -4

index 2d8a371..978ae89 100644
@@ -51,7 +51,7 @@ print_ftrace_graph_addr(unsigned long addr, void *data,
 	index -= *graph;
 	ret_addr = task->ret_stack[index].ret;
 
-	ops->address(data, ret_addr, 1);
+	ops->address(data, 0, ret_addr, 1);
 
 	(*graph)++;
 }

@@ -96,12 +96,14 @@ print_context_stack(struct thread_info *tinfo,
 
 		addr = *stack;
 		if (__kernel_text_address(addr)) {
-			if ((unsigned long) stack == bp + sizeof(long)) {
-				ops->address(data, addr, 1);
+			unsigned long stk = (unsigned long)stack;
+
+			if (stk == bp + sizeof(long)) {
+				ops->address(data, stk, addr, 1);
 				frame = frame->next_frame;
 				bp = (unsigned long) frame;
 			} else {
-				ops->address(data, addr, 0);
+				ops->address(data, stk, addr, 0);
 			}
 			print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
 		}

arch/x86/kernel/dumpstack_32.c +1 -7

index bca5fba..be633ed 100644
@@ -19,12 +19,6 @@
 
 #include "dumpstack.h"
 
-/* Just a stub for now */
-int x86_is_stack_id(int id, char *name)
-{
-	return 0;
-}
-
 void dump_trace(struct task_struct *task, struct pt_regs *regs,
 		unsigned long *stack, unsigned long bp,
 		const struct stacktrace_ops *ops, void *data)

@@ -64,7 +58,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 		stack = (unsigned long *)context->previous_esp;
 		if (!stack)
 			break;
-		if (ops->stack(data, "IRQ") < 0)
+		if (ops->stack(data, STACK_UNKNOWN, "IRQ") < 0)
 			break;
 		touch_nmi_watchdog();
 	}

arch/x86/kernel/dumpstack_64.c +4 -9

index 54b0a32..9309c5c 100644
@@ -32,11 +32,6 @@ static char x86_stack_ids[][8] = {
 #endif
 	};
 
-int x86_is_stack_id(int id, char *name)
-{
-	return x86_stack_ids[id - 1] == name;
-}
-
 static unsigned long *in_exception_stack(unsigned cpu, unsigned long stack,
 					unsigned *usedp, char **idp)
 {

@@ -155,12 +150,12 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 						&used, &id);
 
 		if (estack_end) {
-			if (ops->stack(data, id) < 0)
+			if (ops->stack(data, STACK_EXCEPTION, id) < 0)
 				break;
 
 			bp = print_context_stack(tinfo, stack, bp, ops,
 						 data, estack_end, &graph);
-			ops->stack(data, "<EOE>");
+			ops->stack(data, STACK_PROCESS, "<EOE>");
 			/*
 			 * We link to the next stack via the
 			 * second-to-last pointer (index -2 to end) in the

@@ -175,7 +170,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 				(IRQ_STACK_SIZE - 64) / sizeof(*irq_stack);
 
 			if (stack >= irq_stack && stack < irq_stack_end) {
-				if (ops->stack(data, "IRQ") < 0)
+				if (ops->stack(data, STACK_INTERRUPT, "IRQ") < 0)
 					break;
 				bp = print_context_stack(tinfo, stack, bp,
 					ops, data, irq_stack_end, &graph);

@@ -186,7 +181,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
 				 */
 				stack = (unsigned long *) (irq_stack_end[-1]);
 				irq_stack_end = NULL;
-				ops->stack(data, "EOI");
+				ops->stack(data, STACK_PROCESS, "EOI");
 				continue;
 			}
 		}

arch/x86/kernel/stacktrace.c +5 -3

index c3eb207..a1415f4 100644
@@ -18,12 +18,13 @@ save_stack_warning_symbol(void *data, char *msg, unsigned long symbol)
 {
 }
 
-static int save_stack_stack(void *data, char *name)
+static int save_stack_stack(void *data, int type, char *name)
 {
 	return 0;
 }
 
-static void save_stack_address(void *data, unsigned long addr, int reliable)
+static void save_stack_address(void *data, unsigned long stack,
+			       unsigned long addr, int reliable)
 {
 	struct stack_trace *trace = data;
 	if (!reliable)

@@ -37,7 +38,8 @@ static void save_stack_address(void *data, unsigned long addr, int reliable)
 }
 
 static void
-save_stack_address_nosched(void *data, unsigned long addr, int reliable)
+save_stack_address_nosched(void *data, unsigned long stack,
+			   unsigned long addr, int reliable)
 {
 	struct stack_trace *trace = (struct stack_trace *)data;
 	if (!reliable)

arch/x86/oprofile/backtrace.c +3 -2

index 044897b..7232bc9 100644
@@ -26,13 +26,14 @@ static void backtrace_warning(void *data, char *msg)
 	/* Ignore warnings */
 }
 
-static int backtrace_stack(void *data, char *name)
+static int backtrace_stack(void *data, int type, char *name)
 {
 	/* Yes, we want all stacks */
 	return 0;
 }
 
-static void backtrace_address(void *data, unsigned long addr, int reliable)
+static void backtrace_address(void *data, unsigned long stack,
+			      unsigned long addr, int reliable)
 {
 	unsigned int *depth = data;

include/linux/perf_counter.h +4 -1

index 9ba1822..2b0528f 100644
@@ -439,6 +439,7 @@ enum perf_callchain_context {
 struct perf_callchain_entry {
 	__u64				nr;
 	__u64				ip[PERF_MAX_STACK_DEPTH];
+	int				restricted;
 };
 
 struct perf_raw_record {

@@ -535,6 +536,7 @@ struct perf_counter {
 	struct list_head		event_entry;
 	struct list_head		sibling_list;
 	int				nr_siblings;
+	int				restricted;
 	struct perf_counter		*group_leader;
 	const struct pmu		*pmu;

@@ -754,7 +756,8 @@ static inline void perf_counter_mmap(struct vm_area_struct *vma)
 extern void perf_counter_comm(struct task_struct *tsk);
 extern void perf_counter_fork(struct task_struct *tsk);
 
-extern struct perf_callchain_entry *perf_callchain(struct pt_regs *regs);
+extern struct perf_callchain_entry *
+perf_callchain(struct perf_counter *counter, struct pt_regs *regs);
 
 extern int sysctl_perf_counter_paranoid;
 extern int sysctl_perf_counter_mlock;

kernel/perf_counter.c +23 -7

index 36f65e2..cc8450e 100644
@@ -50,16 +50,28 @@ static atomic_t nr_task_counters __read_mostly;
  *  1 - disallow cpu counters to unpriv
  *  2 - disallow kernel profiling to unpriv
  */
-int sysctl_perf_counter_paranoid __read_mostly;
+int sysctl_perf_counter_paranoid __read_mostly = 1;
 
 static inline bool perf_paranoid_cpu(void)
 {
-	return sysctl_perf_counter_paranoid > 0;
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 0;
 }
 
 static inline bool perf_paranoid_kernel(void)
 {
-	return sysctl_perf_counter_paranoid > 1;
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 1;
+}
+
+/*
+ * When paranoid == 1 we're limiting kernel profiling to not include
+ * other users' information, this includes IRQ stack traces.
+ */
+static bool perf_counter_restricted(struct perf_counter *counter)
+{
+	if (counter->restricted)
+		return hardirq_count() - (in_nmi() ? 0 : HARDIRQ_OFFSET);
+
+	return 0;
 }
 
 int sysctl_perf_counter_mlock __read_mostly = 512; /* 'free' kb per user */

@@ -1571,7 +1583,7 @@ static struct perf_counter_context *find_get_context(pid_t pid, int cpu)
 	 */
 	if (cpu != -1) {
 		/* Must be root to operate on a CPU counter: */
-		if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
+		if (perf_paranoid_cpu())
 			return ERR_PTR(-EACCES);
 
 		if (cpu < 0 || cpu > num_possible_cpus())

@@ -2458,7 +2470,8 @@ void perf_counter_do_pending(void)
  * Callchain support -- arch specific
  */
 
-__weak struct perf_callchain_entry *perf_callchain(struct pt_regs *regs)
+__weak struct perf_callchain_entry *
+perf_callchain(struct perf_counter *counter, struct pt_regs *regs)
 {
 	return NULL;
 }

@@ -2846,6 +2859,8 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 
 	if (sample_type & PERF_SAMPLE_IP) {
 		ip = perf_instruction_pointer(data->regs);
+		if (perf_counter_restricted(counter))
+			ip = 0;
 		header.size += sizeof(ip);
 	}

@@ -2889,7 +2904,7 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 		header.size += perf_counter_read_size(counter);
 
 	if (sample_type & PERF_SAMPLE_CALLCHAIN) {
-		callchain = perf_callchain(data->regs);
+		callchain = perf_callchain(counter, data->regs);
 
 		if (callchain) {
 			callchain_size = (1 + callchain->nr) * sizeof(u64);

@@ -4049,6 +4064,7 @@ perf_counter_alloc(struct perf_counter_attr *attr,
 	counter->pmu		= NULL;
 	counter->ctx		= ctx;
 	counter->oncpu		= -1;
+	counter->restricted	= perf_paranoid_cpu();
 
 	counter->parent		= parent_counter;

@@ -4231,7 +4247,7 @@ SYSCALL_DEFINE5(perf_counter_open,
 		return ret;
 
 	if (!attr.exclude_kernel) {
-		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
+		if (perf_paranoid_kernel())
 			return -EACCES;
 	}

kernel/trace/trace_sysprof.c +3 -2

index f669396..d7d2d0d 100644
@@ -71,13 +71,14 @@ static void backtrace_warning(void *data, char *msg)
 	/* Ignore warnings */
 }
 
-static int backtrace_stack(void *data, char *name)
+static int backtrace_stack(void *data, int type, char *name)
 {
 	/* Don't bother with IRQ stacks for now */
 	return -1;
 }
 
-static void backtrace_address(void *data, unsigned long addr, int reliable)
+static void backtrace_address(void *data, unsigned long stack,
+			      unsigned long addr, int reliable)
 {
 	struct backtrace_info *info = data;

Ingo Molnar

2009-08-21 14:22 UTC #5 lore raw

Re: [PATCH 1/4] perf_counter: Default to higher paranoia level

Quoted 70 lines · Peter Zijlstra

Quoted 24 lines · Peter Zijlstra
kernel/perf_counter.c static atomic_t nr_task_counters __read_mostly; · line 63
Quoted 10 lines · Peter Zijlstra
kernel/perf_counter.c +5 -1
+static inline bool perf_paranoid_anon(void)
+{
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 1;
 }
 
 static inline bool perf_paranoid_kernel(void)
 {
-	return sysctl_perf_counter_paranoid > 1;
+	return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 2;
+}
OK, this is buggy:
- capable() uses current, which is unlikely to be counter->owner,
- but even security_real_capable(counter->owner, ...) wouldn't work, since the ->capable() callback isn't NMI safe (selinux takes locks and does allocations in that path).
This puts a severe strain on more complex anonymizers since its basically impossible to tell if counter->owner has permissions on current from NMI context.
Quoted 6 lines
I'll fix up this patch to pre-compute the perf_paranoid_anon_ip() per counter based on creation time state, unless somebody has a better idea.
I could possibly only anonymize IRQ context (SoftIRQ context is difficult since in_softirq() means both in-softirq and softirq-disabled).
Something like the below maybe.. its 3 patches folded and I've no clue how to adapt the ppc code, what do people think?
compile tested on x86-64 only.
---
 arch/x86/include/asm/stacktrace.h  |   14 +++++--
 arch/x86/kernel/cpu/perf_counter.c |   66 +++++++++++++++++++++++++++++++----
 arch/x86/kernel/dumpstack.c        |   10 +++--
 arch/x86/kernel/dumpstack_32.c     |    8 +----
 arch/x86/kernel/dumpstack_64.c     |   13 ++-----
 arch/x86/kernel/stacktrace.c       |    8 +++--
 arch/x86/oprofile/backtrace.c      |    5 ++-
 include/linux/perf_counter.h       |    5 ++-
 kernel/perf_counter.c              |   30 ++++++++++++----
 kernel/trace/trace_sysprof.c       |    5 ++-
 10 files changed, 117 insertions(+), 47 deletions(-)
arch/x86/include/asm/stacktrace.h +7 -2
index cf86a5e..7066caa 100644
@@ -3,17 +3,23 @@
 
 extern int kstack_depth_to_print;
 
-int x86_is_stack_id(int id, char *name);
-
 /* Generic stack tracer with callbacks */
 
+enum stack_type {
+	STACK_UNKNOWN	= -1,
+	STACK_PROCESS   = 0,
+	STACK_INTERRUPT = 1,
+	STACK_EXCEPTION = 2,
+};
+
 struct stacktrace_ops {

arch/x86/include/asm/stacktrace.h line 23

Quoted 10 lines

arch/x86/include/asm/stacktrace.h +3 -2

 	void (*warning)(void *data, char *msg);
 	/* msg must contain %s for the symbol */
 	void (*warning_symbol)(void *data, char *msg, unsigned long symbol);
-	void (*address)(void *data, unsigned long address, int reliable);
+	void (*address)(void *data, unsigned long stack,
+			unsigned long address, int reliable);
 	/* On negative return stop dumping */
-	int (*stack)(void *data, char *name);
+	int (*stack)(void *data, int type, char *name);
 };

nice generalization ...

include/linux/perf_counter.h enum perf_callchain_context { · line 443

Quoted 10 lines

include/linux/perf_counter.h +1 -0

index 9ba1822..2b0528f 100644
@@ -439,6 +439,7 @@ enum perf_callchain_context {
 struct perf_callchain_entry {
 	__u64				nr;
 	__u64				ip[PERF_MAX_STACK_DEPTH];
+	int				restricted;
 };

i'd love to have something more specific here - i.e. a context type ID that identifies these basic types:

- process
- softirq
- hardirq
- NMI

and then let it be up to upper layers to decide what they do with a restricted entry, and how to further process this information.

And it's not just security: for example it would be interesting to sample pure, non-irq overhead - as IRQ overhead is often unrelated to the process being measured.

Ingo

Peter Zijlstra

maintainer

2009-08-24 07:30 UTC #6 lore raw

Re: [PATCH 1/4] perf_counter: Default to higher paranoia level

Quoted 21 lines · Ingo Molnar

include/linux/perf_counter.h enum perf_callchain_context { · line 443
Quoted 10 lines · Peter Zijlstra
include/linux/perf_counter.h +1 -0
index 9ba1822..2b0528f 100644
@@ -439,6 +439,7 @@ enum perf_callchain_context {
 struct perf_callchain_entry {
 	__u64				nr;
 	__u64				ip[PERF_MAX_STACK_DEPTH];
+	int				restricted;
 };
i'd love to have something more specific here - i.e. a context type ID that identifies these basic types:
- process
- softirq
- hardirq
- NMI

Quoted 6 lines

and then let it be up to upper layers to decide what they do with a restricted entry, and how to further process this information.
And it's not just security: for example it would be interesting to sample pure, non-irq overhead - as IRQ overhead is often unrelated to the process being measured.

Yes it is, this is purely about not showing some data. If you don't want to sample IRQ stuff that's something else, we'd have to grow that capability in hardware (like the OS/USR bits) or put perf enable/disable hooks into the irq entry/exit hooks (which doesn't sound all too hot an idea to me).

Simply not showing the call-trace is something all-together different from not profiling it.

Ingo Molnar

2009-08-24 07:37 UTC #7 lore raw

Re: [PATCH 1/4] perf_counter: Default to higher paranoia level

Quoted 29 lines · Peter Zijlstra

Quoted 21 lines · Ingo Molnar
include/linux/perf_counter.h enum perf_callchain_context { · line 443
Quoted 10 lines · Peter Zijlstra
include/linux/perf_counter.h +1 -0
index 9ba1822..2b0528f 100644
@@ -439,6 +439,7 @@ enum perf_callchain_context {
 struct perf_callchain_entry {
 	__u64				nr;
 	__u64				ip[PERF_MAX_STACK_DEPTH];
+	int				restricted;
 };
i'd love to have something more specific here - i.e. a context type ID that identifies these basic types:
- process
- softirq
- hardirq
- NMI
Quoted 6 lines
and then let it be up to upper layers to decide what they do with a restricted entry, and how to further process this information.
And it's not just security: for example it would be interesting to sample pure, non-irq overhead - as IRQ overhead is often unrelated to the process being measured.

Quoted 8 lines

Yes it is, this is purely about not showing some data. If you don't want to sample IRQ stuff that's something else, we'd have to grow that capability in hardware (like the OS/USR bits) or put perf enable/disable hooks into the irq entry/exit hooks (which doesn't sound all too hot an idea to me).
Simply not showing the call-trace is something all-together different from not profiling it.

Well, it's not about not profiling it - it's about being able to _separate out_ the samples from the various contexts.

Right now we already have context separators for call-chains:

        PERF_CONTEXT_HV                 = (__u64)-32,
        PERF_CONTEXT_KERNEL             = (__u64)-128,
        PERF_CONTEXT_USER               = (__u64)-512,

        PERF_CONTEXT_GUEST              = (__u64)-2048,
        PERF_CONTEXT_GUEST_KERNEL       = (__u64)-2176,
        PERF_CONTEXT_GUEST_USER         = (__u64)-2560,

All i'm suggesting is to also have these context separators:

PERF_CONTEXT_KERNEL_HARDIRQ
PERF_CONTEXT_KERNEL_SOFTIRQ
PERF_CONTEXT_KERNEL /* syscall level */

So that if user-space wants to visualize just a portion of it, it can do it.

Ingo

Peter Zijlstra

author

2009-08-19 09:23 UTC #8 lore raw

PATCH 2/4 perf_counter: powerpc: Support the anonymized kernel callchain bits

In reply to Peter Zijlstra

Adds support for anonymized kernel callchains to the powerpc callchain code.

This patch is not folded into the patch that introduces this feature because the powerpc callchain code isn't upstream yet, and this allows re-ordering the patches.

Signed-off-by Peter Zijlstra <[email protected]>

---
 arch/powerpc/kernel/perf_callchain.c |    5 +++++
 1 file changed, 5 insertions(+)

arch/powerpc/kernel/perf_callchain.c +5 -0

Index: linux-2.6/arch/powerpc/kernel/perf_callchain.c
===================================================================
@@ -70,6 +70,11 @@ static void perf_callchain_kernel(struct
 	lr = regs->link;
 	sp = regs->gpr[1];
 	callchain_store(entry, PERF_CONTEXT_KERNEL);
+	next_ip = perf_paranoid_anon_ip();
+	if (next_ip) {
+		callchain_store(entry, next_ip);
+		return;
+	}
 	callchain_store(entry, regs->nip);
 
 	if (!validate_sp(sp, current, STACK_FRAME_OVERHEAD))

tip-bot for Peter Zijlstra

author

2009-08-19 13:31 UTC #9 lore raw

[tip:perfcounters/core] perf_counter: powerpc: Support the anonimized kernel callchain bits

Commit-ID:  512cec5d6f2a71e04464bf4fd76a50571bd0dea1
Gitweb:     http://git.kernel.org/tip/512cec5d6f2a71e04464bf4fd76a50571bd0dea1
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Wed, 19 Aug 2009 11:18:25 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 19 Aug 2009 14:49:03 +0200

perf_counter: powerpc: Support the anonimized kernel callchain bits

Adds support for anonimized kernel callchains to the powerpc callchain code.

This patch is not folded into the patch that introduces this feature because the powerpc callchain code isn't upstream yet, and this allows re-ordering the patches.

Signed-off-by Peter Zijlstra <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Paul Mackerras <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/powerpc/kernel/perf_callchain.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

arch/powerpc/kernel/perf_callchain.c +5 -0

index f74b62c..21fb1ad 100644
@@ -70,6 +70,11 @@ static void perf_callchain_kernel(struct pt_regs *regs,
 	lr = regs->link;
 	sp = regs->gpr[1];
 	callchain_store(entry, PERF_CONTEXT_KERNEL);
+	next_ip = perf_paranoid_anon_ip();
+	if (next_ip) {
+		callchain_store(entry, next_ip);
+		return;
+	}
 	callchain_store(entry, regs->nip);
 
 	if (!validate_sp(sp, current, STACK_FRAME_OVERHEAD))

Peter Zijlstra

author

2009-08-19 09:23 UTC #10 lore raw

PATCH 3/4 perf tools: Check perf.data owner

In reply to Peter Zijlstra

Add an owner check to opening perf.data files and a switch to silence it.

Because perf-report/perf-annotate are binary parsers reading another users' perf.data file could be a security risk if the file were explicitly engineered to trigger bugs in the parser (we hope of course there are non such bugs, but you never know).

Signed-off-by Peter Zijlstra <[email protected]>

---
 tools/perf/builtin-annotate.c |    7 +++++++
 tools/perf/builtin-report.c   |    7 +++++++
 2 files changed, 14 insertions(+)

tools/perf/builtin-annotate.c +7 -0

Index: linux-2.6/tools/perf/builtin-annotate.c
===================================================================
@@ -28,6 +28,7 @@ static char		const *input_name = "perf.d
 static char		default_sort_order[] = "comm,symbol";
 static char		*sort_order = default_sort_order;
 
+static int		force;
 static int		input;
 static int		show_mask = SHOW_KERNEL | SHOW_USER | SHOW_HV;

@@ -976,6 +977,11 @@ static int __cmd_annotate(void)
 		exit(-1);
 	}
 
+	if (!force && (input_stat.st_uid != geteuid())) {
+		fprintf(stderr, "file: %s not owned by current user\n", input_name);
+		exit(-1);
+	}
+
 	if (!input_stat.st_size) {
 		fprintf(stderr, "zero-sized file, nothing to do!\n");
 		exit(0);

@@ -1081,6 +1087,7 @@ static const struct option options[] = {
 		    "input file name"),
 	OPT_STRING('s', "symbol", &sym_hist_filter, "symbol",
 		    "symbol to annotate"),
+	OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
 	OPT_BOOLEAN('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
 	OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,

tools/perf/builtin-report.c +7 -0

Index: linux-2.6/tools/perf/builtin-report.c
===================================================================
@@ -37,6 +37,7 @@ static char		*dso_list_str, *comm_list_s
 static struct strlist	*dso_list, *comm_list, *sym_list;
 static char		*field_sep;
 
+static int		force;
 static int		input;
 static int		show_mask = SHOW_KERNEL | SHOW_USER | SHOW_HV;

@@ -1383,6 +1384,11 @@ static int __cmd_report(void)
 		exit(-1);
 	}
 
+	if (!force && (input_stat.st_uid != geteuid())) {
+		fprintf(stderr, "file: %s not owned by current user\n", input_name);
+		exit(-1);
+	}
+
 	if (!input_stat.st_size) {
 		fprintf(stderr, "zero-sized file, nothing to do!\n");
 		exit(0);

@@ -1594,6 +1600,7 @@ static const struct option options[] = {
 	OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,
 		    "dump raw trace in ASCII"),
 	OPT_STRING('k', "vmlinux", &vmlinux_name, "file", "vmlinux pathname"),
+	OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
 	OPT_BOOLEAN('m', "modules", &modules,
 		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
 	OPT_BOOLEAN('n', "show-nr-samples", &show_nr_samples,

tip-bot for Peter Zijlstra

author

2009-08-19 13:32 UTC #11 lore raw

[tip:perfcounters/core] perf tools: Check perf.data owner

Commit-ID:  fa6963b2481beff8b11f76006fbb63fdbbf2d2d7
Gitweb:     http://git.kernel.org/tip/fa6963b2481beff8b11f76006fbb63fdbbf2d2d7
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Wed, 19 Aug 2009 11:18:26 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 19 Aug 2009 15:25:51 +0200

perf tools: Check perf.data owner

Add an owner check to opening perf.data files and a switch to silence it.

Signed-off-by Peter Zijlstra <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Paul Mackerras <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 tools/perf/builtin-annotate.c |    7 +++++++
 tools/perf/builtin-report.c   |    7 +++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

tools/perf/builtin-annotate.c +7 -0

index 343e7b1..5e17de9 100644
@@ -31,6 +31,7 @@ static char		*vmlinux = "vmlinux";
 static char		default_sort_order[] = "comm,symbol";
 static char		*sort_order = default_sort_order;
 
+static int		force;
 static int		input;
 static int		show_mask = SHOW_KERNEL | SHOW_USER | SHOW_HV;

@@ -1334,6 +1335,11 @@ static int __cmd_annotate(void)
 		exit(-1);
 	}
 
+	if (!force && (stat.st_uid != geteuid())) {
+		fprintf(stderr, "file: %s not owned by current user\n", input_name);
+		exit(-1);
+	}
+
 	if (!stat.st_size) {
 		fprintf(stderr, "zero-sized file, nothing to do!\n");
 		exit(0);

@@ -1439,6 +1445,7 @@ static const struct option options[] = {
 		    "input file name"),
 	OPT_STRING('s', "symbol", &sym_hist_filter, "symbol",
 		    "symbol to annotate"),
+	OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
 	OPT_BOOLEAN('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
 	OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,

tools/perf/builtin-report.c +7 -0

index b53a60f..8b2ec88 100644
@@ -38,6 +38,7 @@ static char		*dso_list_str, *comm_list_str, *sym_list_str,
 static struct strlist	*dso_list, *comm_list, *sym_list;
 static char		*field_sep;
 
+static int		force;
 static int		input;
 static int		show_mask = SHOW_KERNEL | SHOW_USER | SHOW_HV;

@@ -1856,6 +1857,11 @@ static int __cmd_report(void)
 		exit(-1);
 	}
 
+	if (!force && (stat.st_uid != geteuid())) {
+		fprintf(stderr, "file: %s not owned by current user\n", input_name);
+		exit(-1);
+	}
+
 	if (!stat.st_size) {
 		fprintf(stderr, "zero-sized file, nothing to do!\n");
 		exit(0);

@@ -2064,6 +2070,7 @@ static const struct option options[] = {
 	OPT_BOOLEAN('D', "dump-raw-trace", &dump_trace,
 		    "dump raw trace in ASCII"),
 	OPT_STRING('k', "vmlinux", &vmlinux, "file", "vmlinux pathname"),
+	OPT_BOOLEAN('f', "force", &force, "don't complain, do it"),
 	OPT_BOOLEAN('m', "modules", &modules,
 		    "load module symbols - WARNING: use only with -k and LIVE kernel"),
 	OPT_BOOLEAN('n', "show-nr-samples", &show_nr_samples,

Peter Zijlstra

author

2009-08-19 09:22 UTC #12 lore raw

RFC PATCH 4/4 perf_counter: Allow sharing of output channels

In reply to Peter Zijlstra

Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.

[ compile tested only ]

Signed-off-by Peter Zijlstra <[email protected]>

Cc stephane eranian <[email protected]>

---
 include/linux/perf_counter.h |    5 ++
 kernel/perf_counter.c        |   83 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 3 deletions(-)

include/linux/perf_counter.h +5 -0

Index: linux-2.6/include/linux/perf_counter.h
===================================================================
@@ -216,6 +216,7 @@ struct perf_counter_attr {
 #define PERF_COUNTER_IOC_REFRESH	_IO ('$', 2)
 #define PERF_COUNTER_IOC_RESET		_IO ('$', 3)
 #define PERF_COUNTER_IOC_PERIOD		_IOW('$', 4, u64)
+#define PERF_COUNTER_IOC_SET_OUTPUT	_IO ('$', 5)
 
 enum perf_counter_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,

@@ -415,6 +416,9 @@ enum perf_callchain_context {
 	PERF_CONTEXT_MAX		= (__u64)-4095,
 };
 
+#define PERF_FLAG_FD_NO_GROUP	(1U << 0)
+#define PERF_FLAG_FD_OUTPUT	(1U << 1)
+
 #ifdef __KERNEL__
 /*
  * Kernel-internal data types and definitions:

@@ -536,6 +540,7 @@ struct perf_counter {
 	struct list_head		sibling_list;
 	int				nr_siblings;
 	struct perf_counter		*group_leader;
+	struct perf_counter		*output;
 	const struct pmu		*pmu;
 
 	enum perf_counter_active_state	state;

kernel/perf_counter.c +80 -3

149 lines not on this page · raw message

Ingo Molnar

2009-08-19 10:58 UTC #13 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

Quoted 4 lines · Peter Zijlstra

Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
[ compile tested only ]

very nice!

two comments:

include/linux/perf_counter.h struct perf_counter_attr { · line 219

Quoted 9 lines

Index: linux-2.6/include/linux/perf_counter.h
===================================================================
include/linux/perf_counter.h +1 -0
@@ -216,6 +216,7 @@ struct perf_counter_attr {
 #define PERF_COUNTER_IOC_REFRESH	_IO ('$', 2)
 #define PERF_COUNTER_IOC_RESET		_IO ('$', 3)
 #define PERF_COUNTER_IOC_PERIOD		_IOW('$', 4, u64)
+#define PERF_COUNTER_IOC_SET_OUTPUT	_IO ('$', 5)

Time to add a new sys_perf_counter_chattr() syscall and deprecate the ioctls?

include/linux/perf_counter.h enum perf_callchain_context { · line 420

Quoted 6 lines

include/linux/perf_counter.h +2 -0

@@ -415,6 +416,9 @@ enum perf_callchain_context {
 	PERF_CONTEXT_MAX		= (__u64)-4095,
 };
 
+#define PERF_FLAG_FD_NO_GROUP	(1U << 0)
+#define PERF_FLAG_FD_OUTPUT	(1U << 1)

Why not extend the size of perf_counter_attr and add an output_fd parameter? Zero would mean no fd (and this is also backwards compatible behavior).

FD_NO_GROUP is a bit unclean API as it aliases group_fd to two
purposes: the real group_fd and this new output_fd. I think we should move output_fd to the attribute structure.

That way one could also create a group counter which shares the output channel with another counter, in the same perf_counter_open() call.

Ingo

Peter Zijlstra

author

2009-08-19 11:07 UTC #14 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

Quoted 11 lines · Ingo Molnar

Quoted 4 lines · Peter Zijlstra
Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
[ compile tested only ]
very nice!
two comments:

Quoted 12 lines

include/linux/perf_counter.h struct perf_counter_attr { · line 219
Quoted 9 lines
Index: linux-2.6/include/linux/perf_counter.h
===================================================================
include/linux/perf_counter.h +1 -0
@@ -216,6 +216,7 @@ struct perf_counter_attr {
 #define PERF_COUNTER_IOC_REFRESH	_IO ('$', 2)
 #define PERF_COUNTER_IOC_RESET		_IO ('$', 3)
 #define PERF_COUNTER_IOC_PERIOD		_IOW('$', 4, u64)
+#define PERF_COUNTER_IOC_SET_OUTPUT	_IO ('$', 5)
Time to add a new sys_perf_counter_chattr() syscall and deprecate the ioctls?

Could do I guess.. in order to support things like refresh and reset we need a few new offset fields in the attr struct, but that's doable.

Quoted 11 lines

include/linux/perf_counter.h enum perf_callchain_context { · line 420
Quoted 6 lines
include/linux/perf_counter.h +2 -0
@@ -415,6 +416,9 @@ enum perf_callchain_context {
 	PERF_CONTEXT_MAX		= (__u64)-4095,
 };
 
+#define PERF_FLAG_FD_NO_GROUP	(1U << 0)
+#define PERF_FLAG_FD_OUTPUT	(1U << 1)
Why not extend the size of perf_counter_attr and add an output_fd parameter? Zero would mean no fd (and this is also backwards compatible behavior).

Quoted 3 lines

FD_NO_GROUP is a bit unclean API as it aliases group_fd to two
purposes: the real group_fd and this new output_fd. I think we should move output_fd to the attribute structure.

Yeah, the NO_GROUP thing is ugly..

I'll wait for some more comments before reworking this.

Paul Mackerras

2009-08-19 12:45 UTC #15 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

In reply to Ingo Molnar

Quoted 2 lines · Ingo Molnar

Time to add a new sys_perf_counter_chattr() syscall and deprecate the ioctls?

Hmmm... if 2.6.31 goes out with the ioctls, I think we should leave them in for a long time. I've never seen what the problem with ioctl is anyway. At least, I don't see how a new multi-function syscall would be any better.

Paul.

Paul Mackerras

2009-08-19 12:45 UTC #16 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

In reply to Peter Zijlstra

Quoted 2 lines · Peter Zijlstra

Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.

What sort of thing might this be useful for?

Does this only apply to sampling counters?

Paul.

Ingo Molnar

2009-08-19 12:56 UTC #17 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

Quoted 6 lines · Paul Mackerras

Quoted 2 lines · Peter Zijlstra
Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
What sort of thing might this be useful for?

To share the output buffer - say you have a number of tracepoint counters enabled and want a single (coherent) stream of events without a post-processing sorting pass.

Right now each counter has to be opened, mmap-ed separately and then the result stuffed into the output file.

Ingo

Peter Zijlstra

author

2009-08-19 12:56 UTC #18 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

In reply to Paul Mackerras

Quoted 6 lines · Paul Mackerras

Quoted 2 lines · Peter Zijlstra
Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
What sort of thing might this be useful for?

Some people complained that its tedious to mmap() for every counter and would like to share the mmap() output buffer between counters.

This saves on address space and mlock budget and I guess fd management logic.

As long as you're not mixing counters for different tasks/cpus there should be no performance penalty, but even if you do that it might work well enough on slow samples/small systems..

Quoted 1 line

Does this only apply to sampling counters?

Yeah, everything that would otherwise go through the mmap() buffer.

I'm not sure there's anything to be done about the read(2) thing.

Ingo Molnar

2009-08-19 13:01 UTC #19 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

Quoted 4 lines · Peter Zijlstra

Quoted 1 line
Does this only apply to sampling counters?
Yeah, everything that would otherwise go through the mmap() buffer.

Note that this can include sample-driven normal counts as well, i.e. PERF_SAMPLE_READ and perf record -s/--stat.

Ingo

stephane eranian

2009-08-20 10:13 UTC #20 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

In reply to Peter Zijlstra

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Paul Mackerras
Quoted 2 lines · Peter Zijlstra
Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.

Quoted 9 lines

Quoted 1 line
What sort of thing might this be useful for?
Some people complained that its tedious to mmap() for every counter and would like to share the mmap() output buffer between counters.
This saves on address space and mlock budget and I guess fd management logic.

Interesting to see, you seem to have changed your mind on this. I recall pointing this out in my early comments.

But anyway, here are some more comments:

- how does this work with the remapped counts?
Probably only see the count for the target, i.e., output, event

- if samples from multiple events end up in the same buffer, how do
I tell them apart,
i.e., how do I know sample X came from event A, sample X from event B? This may be useful to detect patterns.

Quoted 11 lines

As long as you're not mixing counters for different tasks/cpus there should be no performance penalty, but even if you do that it might work well enough on slow samples/small systems..
Quoted 1 line
Does this only apply to sampling counters?
Yeah, everything that would otherwise go through the mmap() buffer.
I'm not sure there's anything to be done about the read(2) thing.

Peter Zijlstra

author

2009-08-20 10:24 UTC #21 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

Quoted 9 lines · stephane eranian

Quoted 8 lines · Peter Zijlstra
Quoted 6 lines · Paul Mackerras
Quoted 2 lines · Peter Zijlstra
Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
What sort of thing might this be useful for?

Quoted 8 lines

Quoted 7 lines
Some people complained that its tedious to mmap() for every counter and would like to share the mmap() output buffer between counters.
This saves on address space and mlock budget and I guess fd management logic.
Interesting to see, you seem to have changed your mind on this.

Lets say I haven't quite made up my mind yet, but people seem to want it.

Quoted 1 line

I recall pointing this out in my early comments.

Yeah :-)

Quoted 4 lines

But anyway, here are some more comments:
- how does this work with the remapped counts?
Probably only see the count for the target, i.e., output, event

Ah, right you are, we'd have to allow mmap() of the first page but no data section.

Quoted 4 lines

- if samples from multiple events end up in the same buffer, how do
I tell them apart,
i.e., how do I know sample X came from event A, sample X from event B? This may be useful to detect patterns.

PERF_SAMPLE_ID?

Ingo Molnar

2009-08-20 10:28 UTC #22 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

In reply to stephane eranian

Quoted 15 lines · stephane eranian

Quoted 13 lines · Peter Zijlstra
Quoted 7 lines · Paul Mackerras
Quoted 3 lines · Peter Zijlstra
Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
What sort of thing might this be useful for?
Some people complained that its tedious to mmap() for every counter and would like to share the mmap() output buffer between counters.

Quoted 6 lines

Quoted 2 lines
This saves on address space and mlock budget and I guess fd management logic.
Interesting to see, you seem to have changed your mind on this. I recall pointing this out in my early comments.

Btw., did we strongly oppose the notion? The mmap output buffers were always multi-event in essence - just by virtue of the automatic events such as task creation, mmap, etc.

Quoted 5 lines

But anyway, here are some more comments:
- how does this work with the remapped counts? Probably only see the count for the target, i.e., output, event

if you want to recover the momentary index for RDPC you need to mmap the counter(s) you are interested in, to get to the header->index value.

So you can create these counters with 1 pages only (one is the header) - and then remap the output to the primary counter's output buffer.

Quoted 4 lines

- if samples from multiple events end up in the same buffer, how do I tell them apart, i.e., how do I know sample X came from event A, sample X from event B? This may be useful to detect patterns.

Via PERF_SAMPLE_ID. That saves the counter ID into the event so it can be demultiplexed later on.

Ingo

Frederic Weisbecker

2009-08-19 16:19 UTC #23 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

In reply to Peter Zijlstra

Quoted 20 lines · Peter Zijlstra

Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
[ compile tested only ]
Signed-off-by Peter Zijlstra <[email protected]>
Cc stephane eranian <[email protected]>
kernel/perf_counter.c +13 -0
+int perf_counter_set_output(struct perf_counter *counter, int output_fd)
+{
+	struct perf_counter *output_counter = NULL;
+	struct file *output_file = NULL;
+	struct perf_counter *old_output;
+	int fput_needed = 0;
+	int ret = -EINVAL;
+
+	if (!output_fd)
+		goto set;
+
+	output_file = fget_light(output_fd, &fput_needed);
+	if (!output_file)

kernel/perf_counter.c err_size: · line 4259

Quoted 10 lines

kernel/perf_counter.c +10 -0

+		return -EBADF;
+
+	if (output_file->f_op != &perf_fops)
+		goto out;
+
+	output_counter = output_file->private_data;
+
+	/* Don't chain output fds */
+	if (output_counter->output)
+		goto out;

If you don't chain them, how do you propagate more than one path of output redirected?

Peter Zijlstra

author

2009-08-19 16:24 UTC #24 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

Quoted 28 lines · Frederic Weisbecker

Quoted 27 lines · Peter Zijlstra
Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
[ compile tested only ]
Signed-off-by Peter Zijlstra <[email protected]>
Cc stephane eranian <[email protected]>
kernel/perf_counter.c +20 -0
+int perf_counter_set_output(struct perf_counter *counter, int output_fd)
+{
+	struct perf_counter *output_counter = NULL;
+	struct file *output_file = NULL;
+	struct perf_counter *old_output;
+	int fput_needed = 0;
+	int ret = -EINVAL;
+
+	if (!output_fd)
+		goto set;
+
+	output_file = fget_light(output_fd, &fput_needed);
+	if (!output_file)
+		return -EBADF;
+
+	if (output_file->f_op != &perf_fops)
+		goto out;
+
+	output_counter = output_file->private_data;
+

Quoted 8 lines

kernel/perf_counter.c err_size: · line 4259
Quoted 3 lines
kernel/perf_counter.c +3 -0
+	/* Don't chain output fds */
+	if (output_counter->output)
+		goto out;
If you don't chain them, how do you propagate more than one path of output redirected?

you mean, fd2->fd1->fd0 ?

You don't, that sounds like a silly thing to do, do fd1->fd0, fd2->fd0 instead.

Frederic Weisbecker

2009-08-19 16:27 UTC #25 lore raw

Re: [PATCH 4/4][RFC] perf_counter: Allow sharing of output channels

Quoted 38 lines · Peter Zijlstra

Quoted 28 lines · Frederic Weisbecker
Quoted 27 lines · Peter Zijlstra
Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.
[ compile tested only ]
Signed-off-by Peter Zijlstra <[email protected]>
Cc stephane eranian <[email protected]>
kernel/perf_counter.c +20 -0
+int perf_counter_set_output(struct perf_counter *counter, int output_fd)
+{
+	struct perf_counter *output_counter = NULL;
+	struct file *output_file = NULL;
+	struct perf_counter *old_output;
+	int fput_needed = 0;
+	int ret = -EINVAL;
+
+	if (!output_fd)
+		goto set;
+
+	output_file = fget_light(output_fd, &fput_needed);
+	if (!output_file)
+		return -EBADF;
+
+	if (output_file->f_op != &perf_fops)
+		goto out;
+
+	output_counter = output_file->private_data;
+
Quoted 8 lines
kernel/perf_counter.c err_size: · line 4259
Quoted 3 lines
kernel/perf_counter.c +3 -0
+	/* Don't chain output fds */
+	if (output_counter->output)
+		goto out;
If you don't chain them, how do you propagate more than one path of output redirected?

Quoted 5 lines

you mean, fd2->fd1->fd0 ?
You don't, that sounds like a silly thing to do, do fd1->fd0, fd2->fd0 instead.

Ok.

tip-bot for Peter Zijlstra

author

2009-08-25 07:41 UTC #26 lore raw

[tip:perfcounters/core] perf_counter: Allow sharing of output channels

In reply to Peter Zijlstra

Commit-ID:  a4be7c2778d1fd8e0a8a5607bec91fa221ab2c91
Gitweb:     http://git.kernel.org/tip/a4be7c2778d1fd8e0a8a5607bec91fa221ab2c91
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Wed, 19 Aug 2009 11:18:27 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 25 Aug 2009 09:36:13 +0200

perf_counter: Allow sharing of output channels

Provide the ability to configure a counter to send its output to another (already existing) counter's output stream.

Signed-off-by Peter Zijlstra <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc stephane eranian <[email protected]>

Cc Paul Mackerras <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 include/linux/perf_counter.h |    5 +++
 kernel/perf_counter.c        |   83 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 3 deletions(-)

include/linux/perf_counter.h +5 -0

index b53f700..e022b84 100644
@@ -216,6 +216,7 @@ struct perf_counter_attr {
 #define PERF_COUNTER_IOC_REFRESH	_IO ('$', 2)
 #define PERF_COUNTER_IOC_RESET		_IO ('$', 3)
 #define PERF_COUNTER_IOC_PERIOD		_IOW('$', 4, u64)
+#define PERF_COUNTER_IOC_SET_OUTPUT	_IO ('$', 5)
 
 enum perf_counter_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,

@@ -415,6 +416,9 @@ enum perf_callchain_context {
 	PERF_CONTEXT_MAX		= (__u64)-4095,
 };
 
+#define PERF_FLAG_FD_NO_GROUP	(1U << 0)
+#define PERF_FLAG_FD_OUTPUT	(1U << 1)
+
 #ifdef __KERNEL__
 /*
  * Kernel-internal data types and definitions:

@@ -536,6 +540,7 @@ struct perf_counter {
 	struct list_head		sibling_list;
 	int				nr_siblings;
 	struct perf_counter		*group_leader;
+	struct perf_counter		*output;
 	const struct pmu		*pmu;
 
 	enum perf_counter_active_state	state;

kernel/perf_counter.c +80 -3

149 lines not on this page · raw message