perf/ftrace: Sanitize perf function-trace interaction

PATCH In review Peter Zijlstra 4 participants 13 messages Oct 11, 2017 – Oct 16, 2017

Subsystems (3)

perf events Tracing ftrace

lore

Share & tools

lore

Peter Zijlstra

author maintainer

2017-10-11 08:07 UTC #0 lore raw

cover perf/ftrace: Sanitize perf function-trace interaction

less is more..

---
 include/linux/ftrace.h          | 83 +++++++---------------------------------
 include/linux/perf_event.h      |  2 +-
 include/linux/trace_events.h    |  9 ++++-
 kernel/events/core.c            | 13 ++-----
 kernel/trace/ftrace.c           | 55 +++------------------------
 kernel/trace/trace_event_perf.c | 84 ++++++++++++++++++++++++-----------------
 kernel/trace/trace_kprobe.c     |  4 +-
 kernel/trace/trace_syscalls.c   |  4 +-
 kernel/trace/trace_uprobe.c     |  2 +-
 9 files changed, 87 insertions(+), 169 deletions(-)

Peter Zijlstra

author maintainer

2017-10-11 08:07 UTC #1 lore raw

PATCH 1/4 perf/ftrace: Revert ("perf/ftrace: Fix double traces of perf on ftrace:function")

Revert commit:

75e8387685f6 ("perf/ftrace: Fix double traces of perf on ftrace:function")

The reason I instantly stumbled on that patch is that it only addresses the ftrace situation and doesn't mention the other _5_ places that use this interface. It doesn't explain why those don't have the problem and if not, why their solution doesn't work for ftrace.

It doesn't, but this is just putting more duct tape on.

Cc Zhou Chengming <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Steven Rostedt (VMware) <[email protected]>

Cc Ingo Molnar <[email protected]>

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

---
 include/linux/perf_event.h      |    2 +-
 include/linux/trace_events.h    |    4 ++--
 kernel/events/core.c            |   13 ++++---------
 kernel/trace/trace_event_perf.c |    4 +---
 kernel/trace/trace_kprobe.c     |    4 ++--
 kernel/trace/trace_syscalls.c   |    4 ++--
 kernel/trace/trace_uprobe.c     |    2 +-
 7 files changed, 13 insertions(+), 20 deletions(-)

include/linux/perf_event.h +1 -1

@@ -1169,7 +1169,7 @@ extern void perf_event_init(void);
 extern void perf_tp_event(u16 event_type, u64 count, void *record,
 			  int entry_size, struct pt_regs *regs,
 			  struct hlist_head *head, int rctx,
-			  struct task_struct *task, struct perf_event *event);
+			  struct task_struct *task);
 extern void perf_bp_event(struct perf_event *event, void *data);
 
 #ifndef perf_misc_flags

include/linux/trace_events.h +2 -2

@@ -507,9 +507,9 @@ void perf_trace_run_bpf_submit(void *raw
 static inline void
 perf_trace_buf_submit(void *raw_data, int size, int rctx, u16 type,
 		       u64 count, struct pt_regs *regs, void *head,
-		       struct task_struct *task, struct perf_event *event)
+		       struct task_struct *task)
 {
-	perf_tp_event(type, count, raw_data, size, regs, head, rctx, task, event);
+	perf_tp_event(type, count, raw_data, size, regs, head, rctx, task);
 }
 #endif

kernel/events/core.c +4 -9

@@ -7873,15 +7873,16 @@ void perf_trace_run_bpf_submit(void *raw
 		}
 	}
 	perf_tp_event(call->event.type, count, raw_data, size, regs, head,
-		      rctx, task, NULL);
+		      rctx, task);
 }
 EXPORT_SYMBOL_GPL(perf_trace_run_bpf_submit);
 
 void perf_tp_event(u16 event_type, u64 count, void *record, int entry_size,
 		   struct pt_regs *regs, struct hlist_head *head, int rctx,
-		   struct task_struct *task, struct perf_event *event)
+		   struct task_struct *task)
 {
 	struct perf_sample_data data;
+	struct perf_event *event;
 
 	struct perf_raw_record raw = {
 		.frag = {

@@ -7895,15 +7896,9 @@ void perf_tp_event(u16 event_type, u64 c
 
 	perf_trace_buf_update(record, event_type);
 
-	/* Use the given event instead of the hlist */
-	if (event) {
+	hlist_for_each_entry_rcu(event, head, hlist_entry) {
 		if (perf_tp_event_match(event, &data, regs))
 			perf_swevent_event(event, count, &data, regs);
-	} else {
-		hlist_for_each_entry_rcu(event, head, hlist_entry) {
-			if (perf_tp_event_match(event, &data, regs))
-				perf_swevent_event(event, count, &data, regs);
-		}
 	}
 
 	/*

kernel/trace/trace_event_perf.c +1 -3

@@ -306,7 +306,6 @@ static void
 perf_ftrace_function_call(unsigned long ip, unsigned long parent_ip,
 			  struct ftrace_ops *ops, struct pt_regs *pt_regs)
 {
-	struct perf_event *event;
 	struct ftrace_entry *entry;
 	struct hlist_head *head;
 	struct pt_regs regs;

@@ -330,9 +329,8 @@ perf_ftrace_function_call(unsigned long
 
 	entry->ip = ip;
 	entry->parent_ip = parent_ip;
-	event = container_of(ops, struct perf_event, ftrace_ops);
 	perf_trace_buf_submit(entry, ENTRY_SIZE, rctx, TRACE_FN,
-			      1, &regs, head, NULL, event);
+			      1, &regs, head, NULL);
 
 #undef ENTRY_SIZE
 }

kernel/trace/trace_kprobe.c +2 -2

@@ -1200,7 +1200,7 @@ kprobe_perf_func(struct trace_kprobe *tk
 	memset(&entry[1], 0, dsize);
 	store_trace_args(sizeof(*entry), &tk->tp, regs, (u8 *)&entry[1], dsize);
 	perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
-			      head, NULL, NULL);
+			      head, NULL);
 }
 NOKPROBE_SYMBOL(kprobe_perf_func);

@@ -1236,7 +1236,7 @@ kretprobe_perf_func(struct trace_kprobe
 	entry->ret_ip = (unsigned long)ri->ret_addr;
 	store_trace_args(sizeof(*entry), &tk->tp, regs, (u8 *)&entry[1], dsize);
 	perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
-			      head, NULL, NULL);
+			      head, NULL);
 }
 NOKPROBE_SYMBOL(kretprobe_perf_func);
 #endif	/* CONFIG_PERF_EVENTS */

kernel/trace/trace_syscalls.c +2 -2

@@ -622,7 +622,7 @@ static void perf_syscall_enter(void *ign
 
 	perf_trace_buf_submit(rec, size, rctx,
 			      sys_data->enter_event->event.type, 1, regs,
-			      head, NULL, NULL);
+			      head, NULL);
 }
 
 static int perf_sysenter_enable(struct trace_event_call *call)

@@ -716,7 +716,7 @@ static void perf_syscall_exit(void *igno
 	}
 
 	perf_trace_buf_submit(rec, size, rctx, sys_data->exit_event->event.type,
-			      1, regs, head, NULL, NULL);
+			      1, regs, head, NULL);
 }
 
 static int perf_sysexit_enable(struct trace_event_call *call)

kernel/trace/trace_uprobe.c +1 -1

@@ -1156,7 +1156,7 @@ static void __uprobe_perf_func(struct tr
 	}
 
 	perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
-			      head, NULL, NULL);
+			      head, NULL);
  out:
 	preempt_enable();
 }

Steven Rostedt

maintainer Acked-by on v1 1/4

2017-10-11 12:57 UTC #2 lore raw

Re: [PATCH 1/4] perf/ftrace: Revert ("perf/ftrace: Fix double traces of perf on ftrace:function")

Quoted 9 lines · On Wed, 11 Oct 2017 09:45:29 +0200 Peter Zijlstra <[email protected]> wrote:

Revert commit:
75e8387685f6 ("perf/ftrace: Fix double traces of perf on ftrace:function")
The reason I instantly stumbled on that patch is that it only addresses the ftrace situation and doesn't mention the other _5_ places that use this interface. It doesn't explain why those don't have the problem and if not, why their solution doesn't work for ftrace.

Quoted 7 lines

It doesn't, but this is just putting more duct tape on.
Cc Zhou Chengming <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Steven Rostedt (VMware) <[email protected]>
Cc Ingo Molnar <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Steven Rostedt (VMware) <[email protected]>

-- Steve

Peter Zijlstra

author maintainer

2017-10-11 08:08 UTC #3 lore raw

PATCH 2/4 perf/ftrace: Fix function trace events

In reply to Peter Zijlstra

The function-trace <-> perf interface is a tad messed up. Where all the other trace <-> perf interfaces use a single trace hook registration and use per-cpu RCU based hlist to iterate the events, function-trace actually needs multiple hook registrations in order to minimize function entry patching when filters are present.

The end result is that we iterate events both on the trace hook and on the hlist, which results in reporting events multiple times.

Since function-trace cannot use the regular scheme, fix it the other way around, use singleton hlists.

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

---
 include/linux/trace_events.h    |    5 ++
 kernel/trace/trace_event_perf.c |   82 ++++++++++++++++++++++++----------------
 2 files changed, 55 insertions(+), 32 deletions(-)

include/linux/trace_events.h +5 -0

@@ -173,6 +173,11 @@ enum trace_reg {
 	TRACE_REG_PERF_UNREGISTER,
 	TRACE_REG_PERF_OPEN,
 	TRACE_REG_PERF_CLOSE,
+	/*
+	 * These (ADD/DEL) use a 'boolean' return value, where 1 (true) means a
+	 * custom action was taken and the default action is not to be
+	 * performed.
+	 */
 	TRACE_REG_PERF_ADD,
 	TRACE_REG_PERF_DEL,
 #endif

kernel/trace/trace_event_perf.c +50 ~~-32~~

136 lines not on this page · raw message

zhouchengming

2017-10-11 09:03 UTC #4 lore raw

Re: [PATCH 2/4] perf/ftrace: Fix function trace events

Quoted 51 lines · Peter Zijlstra

The function-trace<-> perf interface is a tad messed up. Where all the other trace<-> perf interfaces use a single trace hook registration and use per-cpu RCU based hlist to iterate the events, function-trace actually needs multiple hook registrations in order to minimize function entry patching when filters are present.
The end result is that we iterate events both on the trace hook and on the hlist, which results in reporting events multiple times.
Since function-trace cannot use the regular scheme, fix it the other way around, use singleton hlists.
Signed-off-by Peter Zijlstra (Intel)<[email protected]>
---
  include/linux/trace_events.h    |    5 ++
  kernel/trace/trace_event_perf.c |   82 ++++++++++++++++++++++++----------------
  2 files changed, 55 insertions(+), 32 deletions(-)
include/linux/trace_events.h +5 -0
@@ -173,6 +173,11 @@ enum trace_reg {
  	TRACE_REG_PERF_UNREGISTER,
  	TRACE_REG_PERF_OPEN,
  	TRACE_REG_PERF_CLOSE,
+	/*
+	 * These (ADD/DEL) use a 'boolean' return value, where 1 (true) means a
+	 * custom action was taken and the default action is not to be
+	 * performed.
+	 */
  	TRACE_REG_PERF_ADD,
  	TRACE_REG_PERF_DEL,
  #endif
kernel/trace/trace_event_perf.c +7 -5
@@ -240,27 +240,41 @@ void perf_trace_destroy(struct perf_even
  int perf_trace_add(struct perf_event *p_event, int flags)
  {
  	struct trace_event_call *tp_event = p_event->tp_event;
-	struct hlist_head __percpu *pcpu_list;
-	struct hlist_head *list;
 
-	pcpu_list = tp_event->perf_events;
-	if (WARN_ON_ONCE(!pcpu_list))
-		return -EINVAL;
+	/*
+	 * If TRACE_REG_PERF_ADD returns false; no custom action was performed
+	 * and we need to take the default action of enqueueing our event on
+	 * the right per-cpu hlist.
+	 */
+	if (!tp_event->class->reg(tp_event, TRACE_REG_PERF_ADD, p_event)) {
+		struct hlist_head __percpu *pcpu_list;

kernel/trace/trace_event_perf.c void perf_trace_destroy(struct perf_even · line 258

Quoted 10 lines

kernel/trace/trace_event_perf.c +7 -2

+		struct hlist_head *list;
+
+		pcpu_list = tp_event->perf_events;
+		if (WARN_ON_ONCE(!pcpu_list))
+			return -EINVAL;
 
-	if (!(flags&  PERF_EF_START))
-		p_event->hw.state = PERF_HES_STOPPED;
+		if (!(flags&  PERF_EF_START))
+			p_event->hw.state = PERF_HES_STOPPED;

Don't we need to check the flags for ftrace perf_event? So if we should put this outside the if (!tp_event->class->reg()) ?

kernel/trace/trace_event_perf.c void perf_trace_destroy(struct perf_even · line 262

Quoted 6 lines

kernel/trace/trace_event_perf.c +3 -2

-	list = this_cpu_ptr(pcpu_list);
-	hlist_add_head_rcu(&p_event->hlist_entry, list);
+		list = this_cpu_ptr(pcpu_list);
+		hlist_add_head_rcu(&p_event->hlist_entry, list);
+	}

Now we don't add perf_event to the pcpu_list, so we also can avoid to alloc pcpu_list for function tp_event in perf_trace_event_reg().

Thanks.

Quoted 114 lines

kernel/trace/trace_event_perf.c +33 ~~-23~~

-	return tp_event->class->reg(tp_event, TRACE_REG_PERF_ADD, p_event);
+	return 0;
  }
 
  void perf_trace_del(struct perf_event *p_event, int flags)
  {
  	struct trace_event_call *tp_event = p_event->tp_event;
-	hlist_del_rcu(&p_event->hlist_entry);
-	tp_event->class->reg(tp_event, TRACE_REG_PERF_DEL, p_event);
+
+	/*
+	 * If TRACE_REG_PERF_DEL returns false; no custom action was performed
+	 * and we need to take the default action of dequeueing our event from
+	 * the right per-cpu hlist.
+	 */
+	if (!tp_event->class->reg(tp_event, TRACE_REG_PERF_DEL, p_event))
+		hlist_del_rcu(&p_event->hlist_entry);
  }
 
  void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)

@@ -307,14 +321,24 @@ perf_ftrace_function_call(unsigned long
  			  struct ftrace_ops *ops, struct pt_regs *pt_regs)
  {
  	struct ftrace_entry *entry;
-	struct hlist_head *head;
+	struct perf_event *event;
+	struct hlist_head head;
  	struct pt_regs regs;
  	int rctx;
 
-	head = this_cpu_ptr(event_function.perf_events);
-	if (hlist_empty(head))
+	if ((unsigned long)ops->private != smp_processor_id())
  		return;
 
+	event = container_of(ops, struct perf_event, ftrace_ops);
+
+	/*
+	 * @event->hlist entry is NULL (per INIT_HLIST_NODE), and all
+	 * the perf code does is hlist_for_each_entry_rcu(), so we can
+	 * get away with simply setting the @head.first pointer in order
+	 * to create a singular list.
+	 */
+	head.first =&event->hlist_entry;
+
  #define ENTRY_SIZE (ALIGN(sizeof(struct ftrace_entry) + sizeof(u32), \
  		    sizeof(u64)) - sizeof(u32))

@@ -330,7 +354,7 @@ perf_ftrace_function_call(unsigned long
  	entry->ip = ip;
  	entry->parent_ip = parent_ip;
  	perf_trace_buf_submit(entry, ENTRY_SIZE, rctx, TRACE_FN,
-			      1,&regs, head, NULL);
+			      1,&regs,&head, NULL);
 
  #undef ENTRY_SIZE
  }

@@ -339,8 +363,10 @@ static int perf_ftrace_function_register
  {
  	struct ftrace_ops *ops =&event->ftrace_ops;
 
-	ops->flags |= FTRACE_OPS_FL_PER_CPU | FTRACE_OPS_FL_RCU;
-	ops->func = perf_ftrace_function_call;
+	ops->flags   |= FTRACE_OPS_FL_RCU;
+	ops->func    = perf_ftrace_function_call;
+	ops->private = (void *)(unsigned long)nr_cpu_ids;
+
  	return register_ftrace_function(ops);
  }

@@ -352,19 +378,11 @@ static int perf_ftrace_function_unregist
  	return ret;
  }
 
-static void perf_ftrace_function_enable(struct perf_event *event)
-{
-	ftrace_function_local_enable(&event->ftrace_ops);
-}
-
-static void perf_ftrace_function_disable(struct perf_event *event)
-{
-	ftrace_function_local_disable(&event->ftrace_ops);
-}
-
  int perf_ftrace_event_register(struct trace_event_call *call,
  			       enum trace_reg type, void *data)
  {
+	struct perf_event *event = data;
+
  	switch (type) {
  	case TRACE_REG_REGISTER:
  	case TRACE_REG_UNREGISTER:

@@ -377,11 +395,11 @@ int perf_ftrace_event_register(struct tr
  	case TRACE_REG_PERF_CLOSE:
  		return perf_ftrace_function_unregister(data);
  	case TRACE_REG_PERF_ADD:
-		perf_ftrace_function_enable(data);
-		return 0;
+		event->ftrace_ops.private = (void *)(unsigned long)smp_processor_id();
+		return 1;
  	case TRACE_REG_PERF_DEL:
-		perf_ftrace_function_disable(data);
-		return 0;
+		event->ftrace_ops.private = (void *)(unsigned long)nr_cpu_ids;
+		return 1;
  	}
 
  	return -EINVAL;

Peter Zijlstra

author maintainer

2017-10-11 16:40 UTC #5 lore raw

Re: [PATCH 2/4] perf/ftrace: Fix function trace events

Quoted 7 lines · zhouchengming

Review
Quoted 2 lines · Peter Zijlstra
(quoted diff) +2 -0
+		if (!(flags&  PERF_EF_START))
+			p_event->hw.state = PERF_HES_STOPPED;
Don't we need to check the flags for ftrace perf_event? So if we should put this outside the if (!tp_event->class->reg()) ?

Oh, right you are, that's independent of the hlist nonsense.

Quoted 6 lines

kernel/trace/trace_event_perf.c void perf_trace_destroy(struct perf_even · line 262
Quoted 3 lines
kernel/trace/trace_event_perf.c +3 -0
+		list = this_cpu_ptr(pcpu_list);
+		hlist_add_head_rcu(&p_event->hlist_entry, list);
+	}
Now we don't add perf_event to the pcpu_list, so we also can avoid to alloc pcpu_list for function tp_event in perf_trace_event_reg().

Yeah that allocation is superfluous, Not entirely sure how to cleanly get rid of that though; that's another web to untangle.

Jiri Olsa

2017-10-11 11:59 UTC #6 lore raw

Re: [PATCH 2/4] perf/ftrace: Fix function trace events

In reply to Peter Zijlstra

On Wed, Oct 11, 2017 at 09:45:30AM +0200, Peter Zijlstra wrote:

SNIP

Quoted 45 lines

void perf_trace_del(struct perf_event *p_event, int flags) {

kernel/trace/trace_event_perf.c +21 -5

 	struct trace_event_call *tp_event = p_event->tp_event;
-	hlist_del_rcu(&p_event->hlist_entry);
-	tp_event->class->reg(tp_event, TRACE_REG_PERF_DEL, p_event);
+
+	/*
+	 * If TRACE_REG_PERF_DEL returns false; no custom action was performed
+	 * and we need to take the default action of dequeueing our event from
+	 * the right per-cpu hlist.
+	 */
+	if (!tp_event->class->reg(tp_event, TRACE_REG_PERF_DEL, p_event))
+		hlist_del_rcu(&p_event->hlist_entry);
 }
 
 void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)

@@ -307,14 +321,24 @@ perf_ftrace_function_call(unsigned long
 			  struct ftrace_ops *ops, struct pt_regs *pt_regs)
 {
 	struct ftrace_entry *entry;
-	struct hlist_head *head;
+	struct perf_event *event;
+	struct hlist_head head;
 	struct pt_regs regs;
 	int rctx;
 
-	head = this_cpu_ptr(event_function.perf_events);
-	if (hlist_empty(head))
+	if ((unsigned long)ops->private != smp_processor_id())
 		return;
 
+	event = container_of(ops, struct perf_event, ftrace_ops);
+
+	/*
+	 * @event->hlist entry is NULL (per INIT_HLIST_NODE), and all
+	 * the perf code does is hlist_for_each_entry_rcu(), so we can
+	 * get away with simply setting the @head.first pointer in order
+	 * to create a singular list.
+	 */
+	head.first = &event->hlist_entry;
+
 #define ENTRY_SIZE (ALIGN(sizeof(struct ftrace_entry) + sizeof(u32), \
 		    sizeof(u64)) - sizeof(u32))

kernel/trace/trace_event_perf.c perf_ftrace_function_call(unsigned long · line 357

Quoted 5 lines

kernel/trace/trace_event_perf.c +1 -1

@@ -330,7 +354,7 @@ perf_ftrace_function_call(unsigned long
 	entry->ip = ip;
 	entry->parent_ip = parent_ip;
 	perf_trace_buf_submit(entry, ENTRY_SIZE, rctx, TRACE_FN,
-			      1, &regs, head, NULL);
+			      1, &regs, &head, NULL);

nice idea.. I'll test it

jirka

Steven Rostedt

maintainer

2017-10-11 13:04 UTC #7 lore raw

Re: [PATCH 2/4] perf/ftrace: Fix function trace events

Quoted 8 lines · On Wed, 11 Oct 2017 13:59:54 +0200 Jiri Olsa <[email protected]> wrote:

kernel/trace/trace_event_perf.c perf_ftrace_function_call(unsigned long · line 357
Quoted 6 lines
kernel/trace/trace_event_perf.c +1 -1
@@ -330,7 +354,7 @@ perf_ftrace_function_call(unsigned long
 	entry->ip = ip;
 	entry->parent_ip = parent_ip;
 	perf_trace_buf_submit(entry, ENTRY_SIZE, rctx, TRACE_FN,
-			      1, &regs, head, NULL);
+			      1, &regs, &head, NULL);  
nice idea.. I'll test it

After you test it, want me to take this through my tree?

Also, we should address zhouchengming's concerns.

-- Steve

Jiri Olsa

2017-10-11 14:23 UTC #8 lore raw

Re: [PATCH 2/4] perf/ftrace: Fix function trace events

Quoted 12 lines · Steven Rostedt

Quoted 8 lines · On Wed, 11 Oct 2017 13:59:54 +0200 Jiri Olsa <[email protected]> wrote:
kernel/trace/trace_event_perf.c perf_ftrace_function_call(unsigned long · line 357
Quoted 6 lines
kernel/trace/trace_event_perf.c +1 -1
@@ -330,7 +354,7 @@ perf_ftrace_function_call(unsigned long
 	entry->ip = ip;
 	entry->parent_ip = parent_ip;
 	perf_trace_buf_submit(entry, ENTRY_SIZE, rctx, TRACE_FN,
-			      1, &regs, head, NULL);
+			      1, &regs, &head, NULL);  
nice idea.. I'll test it

Quoted 1 line

After you test it, want me to take this through my tree?

peter's call ;-)

Quoted 1 line

Also, we should address zhouchengming's concerns.

agreed.. I tested and it looks ok to me

jirka

Peter Zijlstra

author maintainer

2017-10-13 07:17 UTC #9 lore raw

Re: [PATCH 2/4] perf/ftrace: Fix function trace events

In reply to Peter Zijlstra

Updated version; Steve could you route these 4 patches, they're mostly kernel/trace/ related.

---
Subject: perf/ftrace: Fix function trace events
From: Peter Zijlstra <[email protected]>
Date: Tue Oct 10 17:15:47 CEST 2017

The end result is that we iterate events both on the trace hook and on the hlist, which results in reporting events multiple times.

Since function-trace cannot use the regular scheme, fix it the other way around, use singleton hlists.

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

---
 include/linux/trace_events.h    |    5 ++
 kernel/trace/trace_event_perf.c |   80 ++++++++++++++++++++++++----------------
 2 files changed, 54 insertions(+), 31 deletions(-)

include/linux/trace_events.h +5 -0

@@ -173,6 +173,11 @@ enum trace_reg {
 	TRACE_REG_PERF_UNREGISTER,
 	TRACE_REG_PERF_OPEN,
 	TRACE_REG_PERF_CLOSE,
+	/*
+	 * These (ADD/DEL) use a 'boolean' return value, where 1 (true) means a
+	 * custom action was taken and the default action is not to be
+	 * performed.
+	 */
 	TRACE_REG_PERF_ADD,
 	TRACE_REG_PERF_DEL,
 #endif

kernel/trace/trace_event_perf.c +49 ~~-31~~

135 lines not on this page · raw message

Steven Rostedt

maintainer

2017-10-16 22:15 UTC #10 lore raw

Re: [PATCH 2/4] perf/ftrace: Fix function trace events

Quoted 2 lines · On Fri, 13 Oct 2017 09:17:00 +0200 Peter Zijlstra <[email protected]> wrote:

Updated version; Steve could you route these 4 patches, they're mostly kernel/trace/ related.

I just pull these in. They are fine for 4.15 right? They don't need to go to stable do they?

-- Steve

Peter Zijlstra

author maintainer

2017-10-11 08:08 UTC #11 lore raw

PATCH 3/4 perf/ftrace: Small cleanup

In reply to Peter Zijlstra

ops->flags _should_ be 0 at this point, so setting the flag using bitwise or is a bit daft.

Requested-by Steven Rostedt <[email protected]>

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

---
 kernel/trace/trace_event_perf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/trace/trace_event_perf.c +1 -1

@@ -362,7 +362,7 @@ static int perf_ftrace_function_register
 {
 	struct ftrace_ops *ops = &event->ftrace_ops;
 
-	ops->flags   |= FTRACE_OPS_FL_RCU;
+	ops->flags   = FTRACE_OPS_FL_RCU;
 	ops->func    = perf_ftrace_function_call;
 	ops->private = (void *)(unsigned long)nr_cpu_ids;

Peter Zijlstra

author maintainer

2017-10-11 08:08 UTC #12 lore raw

PATCH 4/4 ftrace: Kill FTRACE_OPS_FL_PER_CPU

In reply to Peter Zijlstra

The one and only user of FTRACE_OPS_FL_PER_CPU is gone, remove the lot.

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

---
 include/linux/ftrace.h |   83 ++++++++-----------------------------------------
 kernel/trace/ftrace.c  |   55 +++-----------------------------
 2 files changed, 20 insertions(+), 118 deletions(-)

include/linux/ftrace.h +14 ~~-69~~

@@ -78,10 +78,6 @@ ftrace_func_t ftrace_ops_get_func(struct
  * ENABLED - set/unset when ftrace_ops is registered/unregistered
  * DYNAMIC - set when ftrace_ops is registered to denote dynamically
  *           allocated ftrace_ops which need special care
- * PER_CPU - set manualy by ftrace_ops user to denote the ftrace_ops
- *           could be controlled by following calls:
- *             ftrace_function_local_enable
- *             ftrace_function_local_disable
  * SAVE_REGS - The ftrace_ops wants regs saved at each function called
  *            and passed to the callback. If this flag is set, but the
  *            architecture does not support passing regs

@@ -125,21 +121,20 @@ ftrace_func_t ftrace_ops_get_func(struct
 enum {
 	FTRACE_OPS_FL_ENABLED			= 1 << 0,
 	FTRACE_OPS_FL_DYNAMIC			= 1 << 1,
-	FTRACE_OPS_FL_PER_CPU			= 1 << 2,
-	FTRACE_OPS_FL_SAVE_REGS			= 1 << 3,
-	FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED	= 1 << 4,
-	FTRACE_OPS_FL_RECURSION_SAFE		= 1 << 5,
-	FTRACE_OPS_FL_STUB			= 1 << 6,
-	FTRACE_OPS_FL_INITIALIZED		= 1 << 7,
-	FTRACE_OPS_FL_DELETED			= 1 << 8,
-	FTRACE_OPS_FL_ADDING			= 1 << 9,
-	FTRACE_OPS_FL_REMOVING			= 1 << 10,
-	FTRACE_OPS_FL_MODIFYING			= 1 << 11,
-	FTRACE_OPS_FL_ALLOC_TRAMP		= 1 << 12,
-	FTRACE_OPS_FL_IPMODIFY			= 1 << 13,
-	FTRACE_OPS_FL_PID			= 1 << 14,
-	FTRACE_OPS_FL_RCU			= 1 << 15,
-	FTRACE_OPS_FL_TRACE_ARRAY		= 1 << 16,
+	FTRACE_OPS_FL_SAVE_REGS			= 1 << 2,
+	FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED	= 1 << 3,
+	FTRACE_OPS_FL_RECURSION_SAFE		= 1 << 4,
+	FTRACE_OPS_FL_STUB			= 1 << 5,
+	FTRACE_OPS_FL_INITIALIZED		= 1 << 6,
+	FTRACE_OPS_FL_DELETED			= 1 << 7,
+	FTRACE_OPS_FL_ADDING			= 1 << 8,
+	FTRACE_OPS_FL_REMOVING			= 1 << 9,
+	FTRACE_OPS_FL_MODIFYING			= 1 << 10,
+	FTRACE_OPS_FL_ALLOC_TRAMP		= 1 << 11,
+	FTRACE_OPS_FL_IPMODIFY			= 1 << 12,
+	FTRACE_OPS_FL_PID			= 1 << 13,
+	FTRACE_OPS_FL_RCU			= 1 << 14,
+	FTRACE_OPS_FL_TRACE_ARRAY		= 1 << 15,
 };
 
 #ifdef CONFIG_DYNAMIC_FTRACE

@@ -172,7 +167,6 @@ struct ftrace_ops {
 	unsigned long			flags;
 	void				*private;
 	ftrace_func_t			saved_func;
-	int __percpu			*disabled;
 #ifdef CONFIG_DYNAMIC_FTRACE
 	struct ftrace_ops_hash		local_hash;
 	struct ftrace_ops_hash		*func_hash;

@@ -204,55 +198,6 @@ int register_ftrace_function(struct ftra
 int unregister_ftrace_function(struct ftrace_ops *ops);
 void clear_ftrace_function(void);
 
-/**
- * ftrace_function_local_enable - enable ftrace_ops on current cpu
- *
- * This function enables tracing on current cpu by decreasing
- * the per cpu control variable.
- * It must be called with preemption disabled and only on ftrace_ops
- * registered with FTRACE_OPS_FL_PER_CPU. If called without preemption
- * disabled, this_cpu_ptr will complain when CONFIG_DEBUG_PREEMPT is enabled.
- */
-static inline void ftrace_function_local_enable(struct ftrace_ops *ops)
-{
-	if (WARN_ON_ONCE(!(ops->flags & FTRACE_OPS_FL_PER_CPU)))
-		return;
-
-	(*this_cpu_ptr(ops->disabled))--;
-}
-
-/**
- * ftrace_function_local_disable - disable ftrace_ops on current cpu
- *
- * This function disables tracing on current cpu by increasing
- * the per cpu control variable.
- * It must be called with preemption disabled and only on ftrace_ops
- * registered with FTRACE_OPS_FL_PER_CPU. If called without preemption
- * disabled, this_cpu_ptr will complain when CONFIG_DEBUG_PREEMPT is enabled.
- */
-static inline void ftrace_function_local_disable(struct ftrace_ops *ops)
-{
-	if (WARN_ON_ONCE(!(ops->flags & FTRACE_OPS_FL_PER_CPU)))
-		return;
-
-	(*this_cpu_ptr(ops->disabled))++;
-}
-
-/**
- * ftrace_function_local_disabled - returns ftrace_ops disabled value
- *                                  on current cpu
- *
- * This function returns value of ftrace_ops::disabled on current cpu.
- * It must be called with preemption disabled and only on ftrace_ops
- * registered with FTRACE_OPS_FL_PER_CPU. If called without preemption
- * disabled, this_cpu_ptr will complain when CONFIG_DEBUG_PREEMPT is enabled.
- */
-static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
-{
-	WARN_ON_ONCE(!(ops->flags & FTRACE_OPS_FL_PER_CPU));
-	return *this_cpu_ptr(ops->disabled);
-}
-
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
 			struct ftrace_ops *op, struct pt_regs *regs);

kernel/trace/ftrace.c +6 ~~-49~~

116 lines not on this page · raw message