perf, persistent: Kernel updates for perf tool integration

PATCH In review Robert Richter 5 participants 54 messages Jun 11, 2013 – Jun 27, 2013

Subsystems (5)

x86 perf events Tracing Scheduler +1 more subsystems

lore

Share & tools

lore

Robert Richter

author

2013-06-11 16:42 UTC #0 lore raw

v2 cover perf, persistent: Kernel updates for perf tool integration

This patch set implements out of the box support of perf tools for persistent events. For this the kernel must provide necessary information about existing persistent events via sysfs to userland. Persistent events are provided by the kernel with readonly event buffers. To allow the independent usage of the event buffers by any process without limiting other processes, multiple users of a single event must be handled too.

The basic concept is to use a pmu as an event container for persistent events. The pmu registers events in sysfs and provides format and event information for the userland. The persistent event framework requires to add events to the pmu dynamically.

With the information in sysfs userland knows about how to setup the perf_event attribute of a persistent event. Since a persistent event always has the persistent flag set, a way is needed to express this in sysfs. A new syntax is used for this. With 'attr<num>:<mask>' any bit in the attribute structure may be set in a similar way as using 'config<num>', but <num> is an index that points to the u64 value to change within the attribute.

For persistent events the persistent flag (bit 23 of flag field in struct perf_event_attr) needs to be set which is expressed in sysfs with "attr5:23". E.g. the mce_record event is described in sysfs as follows:

/sys/bus/event_source/devices/persistent/events/mce_record:persistent,config=106 /sys/bus/event_source/devices/persistent/format/persistent:attr5:23

Note that perf tools need to support the 'attr<num>' syntax that is added in a separate patch set. With it we are able to run perf tool commands to read persistent events, e.g.:

 # perf record -e persistent/mce_record/ sleep 10
 # perf top -e persistent/mce_record/

In general the new syntax is flexible to describe with sysfs any event to be setup by perf tools.

First patches contain also fixes and reworks made after reviewing code. Could be applied separately.

Patches base on Boris' patches which I have rebased to latest tip/perf/core. All patches can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git persistent-v2

I wonder if this patch set could be applied to a tip/perf topic branch? This would avoid reposting already reviewed patches.

Note: The perf tools patch set do not need to be reposted.

-Robert

Changes for V2:
* Merged minor changes into Boris' patches
* Included Boris' patches for review
* Document attr<index> syntax in sysfs ABI
* Adding cpu check to perf_get_persistent_event_fd()
* Rebased to latest tip/perf/core

Borislav Petkov (4):
perf, ring_buffer: Use same prefix
perf: Add persistent events
perf: Add persistent event facilities
MCE: Enable persistent event

Robert Richter (10):
perf, persistent: Rework struct pers_event_desc perf, persistent: Remove rb_put()
perf, persistent: Introduce get_persistent_event() perf, persistent: Reworking perf_get_persistent_event_fd() perf, persistent: Protect event lists with mutex perf, persistent: Avoid adding identical events perf, persistent: Implementing a persistent pmu perf, persistent: Name each persistent event perf, persistent: Exposing persistent events using sysfs perf, persistent: Allow multiple users for an event

 .../testing/sysfs-bus-event_source-devices-format  |  43 ++-
 arch/x86/kernel/cpu/mcheck/mce.c                   |  19 ++
 include/linux/perf_event.h                         |   9 +-
 include/uapi/linux/perf_event.h                    |   3 +-
 kernel/events/Makefile                             |   2 +-
 kernel/events/core.c                               |  56 ++--
 kernel/events/internal.h                           |   3 +
 kernel/events/persistent.c                         | 320 +++++++++++++++++++++
 kernel/events/ring_buffer.c                        |   7 +-
 9 files changed, 419 insertions(+), 43 deletions(-)
 create mode 100644 kernel/events/persistent.c

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:48 UTC #1 lore raw

PATCH v2 1/14 perf, ring_buffer: Use same prefix

From: Borislav Petkov <[email protected]>

Rename ring_buffer-handling functions consistently with the "rb_" prefix.

Signed-off-by Borislav Petkov <[email protected]>

Signed-off-by Robert Richter <[email protected]>

---
 kernel/events/core.c        | 37 +++++++++++++++++--------------------
 kernel/events/ring_buffer.c |  7 +++----
 2 files changed, 20 insertions(+), 24 deletions(-)

kernel/events/core.c +17 ~~-20~~

index a0780b3..b790ab6 100644
@@ -198,8 +198,7 @@ static void cpu_ctx_sched_in(struct perf_cpu_context *cpuctx,
 static void update_context_time(struct perf_event_context *ctx);
 static u64 perf_event_time(struct perf_event *event);
 
-static void ring_buffer_attach(struct perf_event *event,
-			       struct ring_buffer *rb);
+static void rb_attach(struct perf_event *event, struct ring_buffer *rb);
 
 void __weak perf_event_print_debug(void)	{ }

@@ -3022,7 +3021,7 @@ static void free_event_rcu(struct rcu_head *head)
 	kfree(event);
 }
 
-static void ring_buffer_put(struct ring_buffer *rb);
+static void rb_put(struct ring_buffer *rb);
 
 static void free_event(struct perf_event *event)
 {

@@ -3054,7 +3053,7 @@ static void free_event(struct perf_event *event)
 	}
 
 	if (event->rb) {
-		ring_buffer_put(event->rb);
+		rb_put(event->rb);
 		event->rb = NULL;
 	}

@@ -3299,8 +3298,8 @@ static unsigned int perf_poll(struct file *file, poll_table *wait)
 	 * t0: T1, rb = rcu_dereference(event->rb)
 	 * t1: T2, old_rb = event->rb
 	 * t2: T2, event->rb = new rb
-	 * t3: T2, ring_buffer_detach(old_rb)
-	 * t4: T1, ring_buffer_attach(rb1)
+	 * t3: T2, rb_detach(old_rb)
+	 * t4: T1, rb_attach(rb1)
 	 * t5: T1, poll_wait(event->waitq)
 	 *
 	 * To avoid this problem, we grab mmap_mutex in perf_poll()

@@ -3312,7 +3311,7 @@ static unsigned int perf_poll(struct file *file, poll_table *wait)
 	rcu_read_lock();
 	rb = rcu_dereference(event->rb);
 	if (rb) {
-		ring_buffer_attach(event, rb);
+		rb_attach(event, rb);
 		events = atomic_xchg(&rb->poll, 0);
 	}
 	rcu_read_unlock();

@@ -3617,8 +3616,7 @@ unlock:
 	return ret;
 }
 
-static void ring_buffer_attach(struct perf_event *event,
-			       struct ring_buffer *rb)
+static void rb_attach(struct perf_event *event, struct ring_buffer *rb)
 {
 	unsigned long flags;

@@ -3634,8 +3632,7 @@ unlock:
 	spin_unlock_irqrestore(&rb->event_lock, flags);
 }
 
-static void ring_buffer_detach(struct perf_event *event,
-			       struct ring_buffer *rb)
+static void rb_detach(struct perf_event *event, struct ring_buffer *rb)
 {
 	unsigned long flags;

@@ -3648,7 +3645,7 @@ static void ring_buffer_detach(struct perf_event *event,
 	spin_unlock_irqrestore(&rb->event_lock, flags);
 }
 
-static void ring_buffer_wakeup(struct perf_event *event)
+static void rb_wakeup(struct perf_event *event)
 {
 	struct ring_buffer *rb;

@@ -3672,7 +3669,7 @@ static void rb_free_rcu(struct rcu_head *rcu_head)
 	rb_free(rb);
 }
 
-static struct ring_buffer *ring_buffer_get(struct perf_event *event)
+static struct ring_buffer *rb_get(struct perf_event *event)
 {
 	struct ring_buffer *rb;

@@ -3687,7 +3684,7 @@ static struct ring_buffer *ring_buffer_get(struct perf_event *event)
 	return rb;
 }
 
-static void ring_buffer_put(struct ring_buffer *rb)
+static void rb_put(struct ring_buffer *rb)
 {
 	struct perf_event *event, *n;
 	unsigned long flags;

@@ -3724,10 +3721,10 @@ static void perf_mmap_close(struct vm_area_struct *vma)
 		atomic_long_sub((size >> PAGE_SHIFT) + 1, &user->locked_vm);
 		vma->vm_mm->pinned_vm -= event->mmap_locked;
 		rcu_assign_pointer(event->rb, NULL);
-		ring_buffer_detach(event, rb);
+		rb_detach(event, rb);
 		mutex_unlock(&event->mmap_mutex);
 
-		ring_buffer_put(rb);
+		rb_put(rb);
 		free_uid(user);
 	}
 }

@@ -3881,7 +3878,7 @@ static const struct file_operations perf_fops = {
 
 void perf_event_wakeup(struct perf_event *event)
 {
-	ring_buffer_wakeup(event);
+	rb_wakeup(event);
 
 	if (event->pending_kill) {
 		kill_fasync(&event->fasync, SIGIO, event->pending_kill);

@@ -6567,7 +6564,7 @@ set:
 
 	if (output_event) {
 		/* get the rb we want to redirect to */
-		rb = ring_buffer_get(output_event);
+		rb = rb_get(output_event);
 		if (!rb)
 			goto unlock;
 	}

@@ -6575,13 +6572,13 @@ set:
 	old_rb = event->rb;
 	rcu_assign_pointer(event->rb, rb);
 	if (old_rb)
-		ring_buffer_detach(event, old_rb);
+		rb_detach(event, old_rb);
 	ret = 0;
 unlock:
 	mutex_unlock(&event->mmap_mutex);
 
 	if (old_rb)
-		ring_buffer_put(old_rb);
+		rb_put(old_rb);
 out:
 	return ret;
 }

kernel/events/ring_buffer.c +3 -4

index cd55144..b514c56 100644
@@ -212,8 +212,7 @@ void perf_output_end(struct perf_output_handle *handle)
 	rcu_read_unlock();
 }
 
-static void
-ring_buffer_init(struct ring_buffer *rb, long watermark, int flags)
+static void rb_init(struct ring_buffer *rb, long watermark, int flags)
 {
 	long max_size = perf_data_size(rb);

@@ -290,7 +289,7 @@ struct ring_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
 
 	rb->nr_pages = nr_pages;
 
-	ring_buffer_init(rb, watermark, flags);
+	rb_init(rb, watermark, flags);
 
 	return rb;

@@ -395,7 +394,7 @@ struct ring_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
 	rb->page_order = ilog2(nr_pages);
 	rb->nr_pages = !!nr_pages;
 
-	ring_buffer_init(rb, watermark, flags);
+	rb_init(rb, watermark, flags);
 
 	return rb;

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:42 UTC #2 lore raw

PATCH v2 2/14 perf: Add persistent events

In reply to Robert Richter

From: Borislav Petkov <[email protected]>

Add the needed pieces for persistent events which makes them process-agnostic. Also, make their buffers read-only when mmaping them from userspace.

While at it, do not return a void function, as caught by Fengguang's build robot.

Changes made by Robert Richter <[email protected]>:

* mmap should return EACCES error if fd can not be opened writable. This error code also helps userland to map buffers readonly on failure.

Signed-off-by Borislav Petkov <[email protected]>

[ Return -EACCES if mapped buffers must be readonly ]

Signed-off-by Robert Richter <[email protected]>

---
 include/uapi/linux/perf_event.h |  3 ++-
 kernel/events/core.c            | 10 +++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

include/uapi/linux/perf_event.h +2 -1

index fb104e5..6032361 100644
@@ -272,8 +272,9 @@ struct perf_event_attr {
 
 				exclude_callchain_kernel : 1, /* exclude kernel callchains */
 				exclude_callchain_user   : 1, /* exclude user callchains */
+				persistent     :  1, /* always-on event */
 
-				__reserved_1   : 41;
+				__reserved_1   : 40;
 
 	union {
 		__u32		wakeup_events;	  /* wakeup every n events */

kernel/events/core.c +9 -1

index b790ab6..a13e457 100644
@@ -3713,6 +3713,11 @@ static void perf_mmap_close(struct vm_area_struct *vma)
 {
 	struct perf_event *event = vma->vm_file->private_data;
 
+	if (event->attr.persistent) {
+		atomic_dec(&event->mmap_count);
+		return;
+	}
+
 	if (atomic_dec_and_mutex_lock(&event->mmap_count, &event->mmap_mutex)) {
 		unsigned long size = perf_data_size(event->rb);
 		struct user_struct *user = event->mmap_user;

@@ -3756,9 +3761,12 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
 	if (event->cpu == -1 && event->attr.inherit)
 		return -EINVAL;
 
-	if (!(vma->vm_flags & VM_SHARED))
+	if (!(vma->vm_flags & VM_SHARED) && !event->attr.persistent)
 		return -EINVAL;
 
+	if (event->attr.persistent && (vma->vm_flags & VM_WRITE))
+		return -EACCES;
+
 	vma_size = vma->vm_end - vma->vm_start;
 	nr_pages = (vma_size / PAGE_SIZE) - 1;

Signature · 1 line

1.8.1.1

Peter Zijlstra

maintainer

2013-06-24 09:28 UTC #3 lore raw

Re: [PATCH v2 02/14] perf: Add persistent events

Quoted 58 lines · Robert Richter

From: Borislav Petkov <[email protected]>
Add the needed pieces for persistent events which makes them process-agnostic. Also, make their buffers read-only when mmaping them from userspace.
While at it, do not return a void function, as caught by Fengguang's build robot.
Changes made by Robert Richter <[email protected]>:
* mmap should return EACCES error if fd can not be opened writable. This error code also helps userland to map buffers readonly on failure.
Signed-off-by Borislav Petkov <[email protected]>
[ Return -EACCES if mapped buffers must be readonly ]
Signed-off-by Robert Richter <[email protected]>
Signed-off-by Robert Richter <[email protected]>
---
 include/uapi/linux/perf_event.h |  3 ++-
 kernel/events/core.c            | 10 +++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)
include/uapi/linux/perf_event.h +2 -1
index fb104e5..6032361 100644
@@ -272,8 +272,9 @@ struct perf_event_attr {
 
 				exclude_callchain_kernel : 1, /* exclude kernel callchains */
 				exclude_callchain_user   : 1, /* exclude user callchains */
+				persistent     :  1, /* always-on event */
 
-				__reserved_1   : 41;
+				__reserved_1   : 40;
 
 	union {
 		__u32		wakeup_events;	  /* wakeup every n events */
kernel/events/core.c +5 -0
index b790ab6..a13e457 100644
@@ -3713,6 +3713,11 @@ static void perf_mmap_close(struct vm_area_struct *vma)
 {
 	struct perf_event *event = vma->vm_file->private_data;
 
+	if (event->attr.persistent) {
+		atomic_dec(&event->mmap_count);
+		return;
+	}
+
 	if (atomic_dec_and_mutex_lock(&event->mmap_count, &event->mmap_mutex)) {
 		unsigned long size = perf_data_size(event->rb);
 		struct user_struct *user = event->mmap_user;
@@ -3756,9 +3761,12 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
 	if (event->cpu == -1 && event->attr.inherit)
 		return -EINVAL;

kernel/events/core.c static int perf_mmap(struct file *file, struct vm_area_struct *vma) · line 3771

Quoted 10 lines

kernel/events/core.c +4 -1

 
-	if (!(vma->vm_flags & VM_SHARED))
+	if (!(vma->vm_flags & VM_SHARED) && !event->attr.persistent)
 		return -EINVAL;
 
+	if (event->attr.persistent && (vma->vm_flags & VM_WRITE))
+		return -EACCES;
+
 	vma_size = vma->vm_end - vma->vm_start;
 	nr_pages = (vma_size / PAGE_SIZE) - 1;

I find this patch incomplete for its asymmetric; there a dec, but not an implied inc.

Borislav Petkov

maintainer

2013-06-24 19:24 UTC #4 lore raw

Re: [PATCH v2 02/14] perf: Add persistent events

Quoted 2 lines · Peter Zijlstra

I find this patch incomplete for its asymmetric; there a dec, but not an implied inc.

There's one in perf_mmap_open.

Basically the idea was to pull up the ->mmap_count decrement (haha, sounds like excrement :-)) and not deallocate the ring buffers for a persistent event.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Robert Richter

author

2013-06-25 08:46 UTC #5 lore raw

Re: [PATCH v2 02/14] perf: Add persistent events

Quoted 9 lines · Borislav Petkov

Quoted 2 lines · Peter Zijlstra
I find this patch incomplete for its asymmetric; there a dec, but not an implied inc.
There's one in perf_mmap_open.
Basically the idea was to pull up the ->mmap_count decrement (haha, sounds like excrement :-)) and not deallocate the ring buffers for a persistent event.

The asymmetry comes from rb_alloc() in add_persistent_event_on_cpu() which is usually implemented in the mmap functions. We might better improve functions to attach/detach events and buffers for more symmetric code.

-Robert

Robert Richter

author

2013-06-11 16:47 UTC #6 lore raw

PATCH v2 3/14 perf: Add persistent event facilities

In reply to Robert Richter

From: Borislav Petkov <[email protected]>

Add a barebones implementation for registering persistent events with perf. For that, we don't destroy the buffers when they're unmapped; also, we map them read-only so that multiple agents can access them.

Also, we allocate the event buffers at event init time and not at mmap time so that we can log samples into them regardless of whether there are readers in userspace or not.

Changes made by Robert Richter <[email protected]>:

* Fixing wrongly determined attribute size.

* The default buffer size used to setup event buffers with perf tools is 512k. Using the same buffer size for persistent events. This also avoids failed mmap calls due to different buffer sizes.

* Improve error reporting.

* Returning -ENODEV if no file descriptor is found. An error code of -1 (-EPERM) is misleading in this case.

* Adding cpu check to perf_get_persistent_event_fd()

[ make percpu variable static ]

Reported-by Fengguang Wu <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

[ Fix attr size ]

[ Setting default buffer size to 512k as in perf tools ]

[ Print error code on failure when adding events ]

[ Return resonable error code ]

[ Adding cpu check to perf_get_persistent_event_fd() ]

Reported-by Jiri Olsa <[email protected]>

Signed-off-by Robert Richter <[email protected]>

---
 include/linux/perf_event.h |  16 +++-
 kernel/events/Makefile     |   2 +-
 kernel/events/core.c       |  13 ++--
 kernel/events/internal.h   |   4 +
 kernel/events/persistent.c | 181 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 207 insertions(+), 9 deletions(-)
 create mode 100644 kernel/events/persistent.c

include/linux/perf_event.h +14 -2

index 6fddac1..d2a42b7 100644
@@ -518,6 +518,13 @@ struct perf_output_handle {
 	int				page;
 };
 
+struct pers_event_desc {
+	struct perf_event_attr *attr;
+	struct perf_event *event;
+	struct list_head plist;
+	int fd;
+};
+
 #ifdef CONFIG_PERF_EVENTS
 
 extern int perf_pmu_register(struct pmu *pmu, char *name, int type);

@@ -750,7 +757,9 @@ extern void perf_event_enable(struct perf_event *event);
 extern void perf_event_disable(struct perf_event *event);
 extern int __perf_event_disable(void *info);
 extern void perf_event_task_tick(void);
-#else
+extern int perf_add_persistent_event(struct perf_event_attr *, unsigned);
+extern int perf_add_persistent_event_by_id(int id);
+#else /* !CONFIG_PERF_EVENTS */
 static inline void
 perf_event_task_sched_in(struct task_struct *prev,
 			 struct task_struct *task)			{ }

@@ -790,7 +799,10 @@ static inline void perf_event_enable(struct perf_event *event)		{ }
 static inline void perf_event_disable(struct perf_event *event)		{ }
 static inline int __perf_event_disable(void *info)			{ return -1; }
 static inline void perf_event_task_tick(void)				{ }
-#endif
+static inline int perf_add_persistent_event(struct perf_event_attr *attr,
+					    unsigned nr_pages)		{ return -EINVAL; }
+static inline int perf_add_persistent_event_by_id(int id)		{ return -EINVAL; }
+#endif /* !CONFIG_PERF_EVENTS */
 
 #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_NO_HZ_FULL)
 extern bool perf_event_can_stop_tick(void);

kernel/events/Makefile +1 -1

index 103f5d1..70990d5 100644
@@ -2,7 +2,7 @@ ifdef CONFIG_FUNCTION_TRACER
 CFLAGS_REMOVE_core.o = -pg
 endif
 
-obj-y := core.o ring_buffer.o callchain.o
+obj-y := core.o ring_buffer.o callchain.o persistent.o
 
 obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
 obj-$(CONFIG_UPROBES) += uprobes.o

kernel/events/core.c +7 -6

index a13e457..a9b6470 100644
@@ -3021,8 +3021,6 @@ static void free_event_rcu(struct rcu_head *head)
 	kfree(event);
 }
 
-static void rb_put(struct ring_buffer *rb);
-
 static void free_event(struct perf_event *event)
 {
 	irq_work_sync(&event->pending);

@@ -3398,8 +3396,6 @@ unlock:
 	return ret;
 }
 
-static const struct file_operations perf_fops;
-
 static inline int perf_fget_light(int fd, struct fd *p)
 {
 	struct fd f = fdget(fd);

@@ -3684,7 +3680,7 @@ static struct ring_buffer *rb_get(struct perf_event *event)
 	return rb;
 }
 
-static void rb_put(struct ring_buffer *rb)
+void rb_put(struct ring_buffer *rb)
 {
 	struct perf_event *event, *n;
 	unsigned long flags;

@@ -3866,7 +3862,7 @@ static int perf_fasync(int fd, struct file *filp, int on)
 	return 0;
 }
 
-static const struct file_operations perf_fops = {
+const struct file_operations perf_fops = {
 	.llseek			= no_llseek,
 	.release		= perf_release,
 	.read			= perf_read,

@@ -6623,6 +6619,9 @@ SYSCALL_DEFINE5(perf_event_open,
 	if (err)
 		return err;
 
+	if (attr.persistent)
+		return perf_get_persistent_event_fd(cpu, &attr);
+
 	if (!attr.exclude_kernel) {
 		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
 			return -EACCES;

@@ -7579,6 +7578,8 @@ void __init perf_event_init(void)
 	 */
 	BUILD_BUG_ON((offsetof(struct perf_event_mmap_page, data_head))
 		     != 1024);
+
+	persistent_events_init();
 }
 
 static int __init perf_event_sysfs_init(void)

kernel/events/internal.h +4 -0

index eb675c4..3b481be 100644
@@ -38,6 +38,7 @@ struct ring_buffer {
 extern void rb_free(struct ring_buffer *rb);
 extern struct ring_buffer *
 rb_alloc(int nr_pages, long watermark, int cpu, int flags);
+extern void rb_put(struct ring_buffer *rb);
 extern void perf_event_wakeup(struct perf_event *event);
 
 extern void

@@ -174,4 +175,7 @@ static inline bool arch_perf_have_user_stack_dump(void)
 #define perf_user_stack_pointer(regs) 0
 #endif /* CONFIG_HAVE_PERF_USER_STACK_DUMP */
 
+extern const struct file_operations perf_fops;
+extern int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr);
+extern void __init persistent_events_init(void);
 #endif /* _KERNEL_EVENTS_INTERNAL_H */

kernel/events/persistent.c new file +181 -0

181 lines not on this page · raw message

Signature · 1 line

1.8.1.1

Namhyung Kim

maintainer

2013-06-14 02:15 UTC #7 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Hi Robert and Boris,

Quoted 36 lines · Robert Richter

From: Borislav Petkov <[email protected]>
Add a barebones implementation for registering persistent events with perf. For that, we don't destroy the buffers when they're unmapped; also, we map them read-only so that multiple agents can access them.
Also, we allocate the event buffers at event init time and not at mmap time so that we can log samples into them regardless of whether there are readers in userspace or not.
Changes made by Robert Richter <[email protected]>:
* Fixing wrongly determined attribute size.
* The default buffer size used to setup event buffers with perf tools is 512k. Using the same buffer size for persistent events. This also avoids failed mmap calls due to different buffer sizes.
* Improve error reporting.
* Returning -ENODEV if no file descriptor is found. An error code of -1 (-EPERM) is misleading in this case.
* Adding cpu check to perf_get_persistent_event_fd()
[ make percpu variable static ]
Reported-by Fengguang Wu <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
[ Fix attr size ]
[ Setting default buffer size to 512k as in perf tools ]
[ Print error code on failure when adding events ]
[ Return resonable error code ]
[ Adding cpu check to perf_get_persistent_event_fd() ]
Reported-by Jiri Olsa <[email protected]>
Signed-off-by Robert Richter <[email protected]>
Signed-off-by Robert Richter <[email protected]>

[SNIP]

Quoted 20 lines

kernel/events/persistent.c new file +20 -0

+int perf_add_persistent_event_by_id(int id)
+{
+	struct perf_event_attr *attr;
+
+	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+
+	attr->sample_period	= 1;
+	attr->wakeup_events	= 1;
+	attr->sample_type	= PERF_SAMPLE_RAW;
+	attr->persistent	= 1;
+	attr->config		= id;
+	attr->type		= PERF_TYPE_TRACEPOINT;
+	attr->size		= sizeof(*attr);
+
+	return perf_add_persistent_event(attr, CPU_BUFFER_NR_PAGES);
+}
+
+int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)

kernel/events/persistent.c line 170

Quoted 10 lines

kernel/events/persistent.c new file +10 -0

+{
+	struct pers_event_desc *desc;
+
+	if (cpu >= (unsigned)nr_cpu_ids)
+		return -EINVAL;
+
+	list_for_each_entry(desc, &per_cpu(pers_events, cpu), plist)
+		if (desc->attr->config == attr->config)
+			return __alloc_persistent_event_fd(desc);
+

So it only supports tracepoint events. Don't we need to add other types of event?

Thanks,
Namhyung

Quoted 11 lines

kernel/events/persistent.c new file +11 -0

+	return -ENODEV;
+}
+
+
+void __init persistent_events_init(void)
+{
+	int i;
+
+	for_each_possible_cpu(i)
+		INIT_LIST_HEAD(&per_cpu(pers_events, i));
+}

Robert Richter

author

2013-06-14 07:20 UTC #8 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 6 lines · Namhyung Kim

Quoted 6 lines

kernel/events/persistent.c new file +6 -0

+int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
+{
+   struct pers_event_desc *desc;
+
+   if (cpu >= (unsigned)nr_cpu_ids)
+           return -EINVAL;

Quoted 8 lines

kernel/events/persistent.c line 170
Quoted 5 lines
kernel/events/persistent.c new file +5 -0
+
+   list_for_each_entry(desc, &per_cpu(pers_events, cpu), plist)
+           if (desc->attr->config == attr->config)
+                   return __alloc_persistent_event_fd(desc);
+
So it only supports tracepoint events. Don't we need to add other types of event?

We have choosen tracepoints as initial implementation since they are our first use case and its specifiers are unique already.

So this is for the initial implementation only. We can easily implement support for other event types later. The only thing we need to implement for this are unique identifiers for each persistent event. Since all events are described in sysfs this change can be done later without changing ABI or breaking userland.

The support for all event types is definitely on our list. We need this also for a later implemention of registering all system wide events as persistent events. This allows sharing events between processes and is one of Ingos use cases to enable persistent events at runtime.

-Robert

Peter Zijlstra

maintainer

2013-06-24 09:32 UTC #9 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

In reply to Robert Richter

Quoted 32 lines · Robert Richter

kernel/events/persistent.c new file +32 -0

+static struct perf_event *
+add_persistent_event_on_cpu(unsigned int cpu, struct perf_event_attr *attr,
+			    unsigned nr_pages)
+{
+	struct perf_event *event = ERR_PTR(-ENOMEM);
+	struct pers_event_desc *desc;
+	struct ring_buffer *buf;
+
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		goto out;
+
+	buf = rb_alloc(nr_pages, 0, cpu, 0);
+	if (!buf)
+		goto err_rb;
+
+	event = perf_event_create_kernel_counter(attr, cpu, NULL, NULL, NULL);
+	if (IS_ERR(event))
+		goto err_event;
+
+	rcu_assign_pointer(event->rb, buf);
+
+	desc->event = event;
+	desc->attr  = attr;
+
+	INIT_LIST_HEAD(&desc->plist);
+	list_add_tail(&desc->plist, &per_cpu(pers_events, cpu));
+
+	/* All workie, enable event now */
+	perf_event_enable(event);
+
+	goto out;

kernel/events/persistent.c line 54

Quoted 10 lines

kernel/events/persistent.c new file +10 -0

+
+ err_event:
+	rb_put(buf);
+
+ err_rb:
+	kfree(desc);
+
+ out:
+	return event;
+}

I generally disapprove of indented labels. None of the perf code has that and the tools are easy to 'fix'.

My quiltrc contains:

QUILT_DIFF_OPTS="-F ^[[:alpha:]\$_].*[^:]\$"

my .gitconfig contains:

[diff "default"]

                xfuncname = "^[[:alpha:]$_].*[^:]$"

Both avoid diff thinking labels are function names.

Robert Richter

author

2013-06-25 08:47 UTC #10 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 5 lines · Peter Zijlstra

I generally disapprove of indented labels. None of the perf code has that and the tools are easy to 'fix'.
My quiltrc contains:

Quoted 8 lines

QUILT_DIFF_OPTS="-F ^[[:alpha:]\$_].*[^:]\$"
my .gitconfig contains:
[diff "default"]
                xfuncname = "^[[:alpha:]$_].*[^:]$"
Both avoid diff thinking labels are function names.

Right, we will use your coding style. ;)

-Robert

Peter Zijlstra

maintainer

2013-06-24 09:45 UTC #11 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

In reply to Robert Richter

kernel/events/persistent.c line 62

Quoted 7 lines

kernel/events/persistent.c new file +7 -0

+static void del_persistent_event(int cpu, struct perf_event_attr *attr)
+{
+	struct pers_event_desc *desc, *tmp;
+	struct perf_event *event = NULL;
+
+	list_for_each_entry_safe(desc, tmp, &per_cpu(pers_events, cpu), plist) {
+		if (desc->attr->config == attr->config) {

attr->config might not be enough; you might also want to compare type, config1, config2, and possible bp_type as well; although I think we want to avoid having hwbp style persistent events, but then what do I know.

Quoted 21 lines

kernel/events/persistent.c new file +21 -0

+			event = desc->event;
+			break;
+		}
+	}
+
+	if (!event)
+		return;
+
+	list_del(&desc->plist);
+
+	perf_event_disable(event);
+	if (event->rb) {
+		rb_put(event->rb);
+		rcu_assign_pointer(event->rb, NULL);
+	}
+
+	perf_event_release_kernel(event);
+	put_unused_fd(desc->fd);
+	kfree(desc->attr);
+	kfree(desc);
+}

Robert Richter

author

2013-06-25 08:41 UTC #12 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 11 lines · Peter Zijlstra

kernel/events/persistent.c line 62
Quoted 7 lines
kernel/events/persistent.c new file +7 -0
+static void del_persistent_event(int cpu, struct perf_event_attr *attr)
+{
+	struct pers_event_desc *desc, *tmp;
+	struct perf_event *event = NULL;
+
+	list_for_each_entry_safe(desc, tmp, &per_cpu(pers_events, cpu), plist) {
+		if (desc->attr->config == attr->config) {
attr->config might not be enough; you might also want to compare type, config1, config2, and possible bp_type as well; although I think we want to avoid having hwbp style persistent events, but then what do I know.

[ I would call this what it is: perf_add_persistent_tracepoint(), or so :-) ]

We chosed a tracepoint-only implementation as a first try and since this is our use case. The goal is to have support for any event type.

I also was thinking of comparing the whole attr structure. But since attr are similar (or almost identical) only you can't compare each member of attr and need exceptions (e.g. cpu number). These exceptions are ugly and depending on the specific use case.

I rather tend to assign unique identifiers to each persistent event and then use attr->config to select the event, e.g.:

		if (desc->unique_id == attr->config) ...

Tracepoints have this unique id in it already, thus tp only first.

The change can be made without breaking ABI, since the event setup is described in sysfs.

-Robert

Quoted 5 lines

Quoted 5 lines
kernel/events/persistent.c new file +4 -0
+			event = desc->event;
+			break;
+		}
+	}

Peter Zijlstra

maintainer

2013-06-24 09:48 UTC #13 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

In reply to Robert Richter

Quoted 8 lines · Robert Richter

kernel/events/persistent.c new file +8 -0

+int perf_add_persistent_event_by_id(int id)
+{
+	struct perf_event_attr *attr;
+
+	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+

kernel/events/persistent.c line 158

Quoted 10 lines

kernel/events/persistent.c new file +10 -0

+	attr->sample_period	= 1;
+	attr->wakeup_events	= 1;
+	attr->sample_type	= PERF_SAMPLE_RAW;
+	attr->persistent	= 1;
+	attr->config		= id;
+	attr->type		= PERF_TYPE_TRACEPOINT;
+	attr->size		= sizeof(*attr);
+
+	return perf_add_persistent_event(attr, CPU_BUFFER_NR_PAGES);
+}

I would call this what it is: perf_add_persistent_tracepoint(), or so :-)

Borislav Petkov

maintainer

2013-06-24 19:26 UTC #14 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 14 lines · Peter Zijlstra

Quoted 13 lines · Robert Richter

kernel/events/persistent.c new file +13 -0

+int perf_add_persistent_event_by_id(int id)
+{
+	struct perf_event_attr *attr;
+
+	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+
+	attr->sample_period	= 1;
+	attr->wakeup_events	= 1;
+	attr->sample_type	= PERF_SAMPLE_RAW;
+	attr->persistent	= 1;
+	attr->config		= id;

Quoted 8 lines

kernel/events/persistent.c line 158
Quoted 5 lines
kernel/events/persistent.c new file +5 -0
+	attr->type		= PERF_TYPE_TRACEPOINT;
+	attr->size		= sizeof(*attr);
+
+	return perf_add_persistent_event(attr, CPU_BUFFER_NR_PAGES);
+}
I would call this what it is: perf_add_persistent_tracepoint(), or so :-)

While we're at it, can we agree on a shortened variant for "persistent"
- it is too much to type and makes function names uglily¹ long.

¹ look at me making up adverbs.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Peter Zijlstra

maintainer

2013-06-25 07:44 UTC #15 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 24 lines · Borislav Petkov

Quoted 14 lines · Peter Zijlstra
Quoted 13 lines · Robert Richter
kernel/events/persistent.c new file +13 -0
+int perf_add_persistent_event_by_id(int id)
+{
+	struct perf_event_attr *attr;
+
+	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+
+	attr->sample_period	= 1;
+	attr->wakeup_events	= 1;
+	attr->sample_type	= PERF_SAMPLE_RAW;
+	attr->persistent	= 1;
+	attr->config		= id;
Quoted 8 lines
kernel/events/persistent.c line 158
Quoted 5 lines
kernel/events/persistent.c new file +5 -0
+	attr->type		= PERF_TYPE_TRACEPOINT;
+	attr->size		= sizeof(*attr);
+
+	return perf_add_persistent_event(attr, CPU_BUFFER_NR_PAGES);
+}
I would call this what it is: perf_add_persistent_tracepoint(), or so :-)

Quoted 2 lines

While we're at it, can we agree on a shortened variant for "persistent"
- it is too much to type and makes function names uglily¹ long.

Elsewhere in this series you use 'pers' to shorten things; it reads a bit odd to me because 'pers' is the Dutch word for press (both meanings transfer) but that is just something I'll have to live with isn't it ;-)

As for tracepoint, it seems common to shorten that to tp; which always reminds me of toilet paper, but I suppose more people suffer from that.

Yielding: perf_add_pers_tp()

which I read as adding pressed toilet paper.. a well :-)

Robert Richter

author

2013-06-25 09:24 UTC #16 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 10 lines · Peter Zijlstra

Elsewhere in this series you use 'pers' to shorten things; it reads a bit odd to me because 'pers' is the Dutch word for press (both meanings transfer) but that is just something I'll have to live with isn't it ;-)
As for tracepoint, it seems common to shorten that to tp; which always reminds me of toilet paper, but I suppose more people suffer from that.
Yielding: perf_add_pers_tp()
which I read as adding pressed toilet paper.. a well :-)

Better we avoid this, don't wonat you misread the code. ;)

I also see 'pers_' not as an optimum since it could be mixed-up easily with 'perf_'. Maybe we take 'persist_' instead?

-Robert

Borislav Petkov

maintainer

2013-06-25 09:37 UTC #17 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 8 lines · Robert Richter

Quoted 7 lines · Peter Zijlstra
Elsewhere in this series you use 'pers' to shorten things; it reads a bit odd to me because 'pers' is the Dutch word for press (both meanings transfer) but that is just something I'll have to live with isn't it ;-)
As for tracepoint, it seems common to shorten that to tp; which always reminds me of toilet paper, but I suppose more people suffer from that.

Quoted 8 lines

Quoted 3 lines
Yielding: perf_add_pers_tp()
which I read as adding pressed toilet paper.. a well :-)
Better we avoid this, don't wonat you misread the code. ;)
I also see 'pers_' not as an optimum since it could be mixed-up easily with 'perf_'. Maybe we take 'persist_' instead?

Yep, although it reads wrong:

perf_add_persist_event

Another not really optimal idea would be to call them sticky events:

perf_add_sticky_event.

Also funny.

Then there's

perf_add_prstnt_event

Also, not really great.

Hmmm.. I got nothing so far...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Robert Richter

author

2013-06-25 10:51 UTC #18 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 7 lines · Borislav Petkov

Quoted 2 lines · Robert Richter
I also see 'pers_' not as an optimum since it could be mixed-up easily with 'perf_'. Maybe we take 'persist_' instead?
Yep, although it reads wrong:
perf_add_persist_event

We could use persist_ as prefix for static functions and use the long versions for the interface only.

But all this is a bit bikeshedding. I am sure we find something.

-Robert

Borislav Petkov

maintainer

2013-06-25 15:29 UTC #19 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 5 lines · Robert Richter

Quoted 4 lines · Borislav Petkov
Quoted 2 lines · Robert Richter
I also see 'pers_' not as an optimum since it could be mixed-up easily with 'perf_'. Maybe we take 'persist_' instead?

Quoted 8 lines

Quoted 3 lines
Yep, although it reads wrong:
perf_add_persist_event
We could use persist_ as prefix for static functions and use the long versions for the interface only.
But all this is a bit bikeshedding. I am sure we find something.

How about

perf_add_pevent?

It is nice and short, although maybe too short but "pevent" is almost like a special term and the name shows that it is a special type of event, i.e. a p-event.

:-)

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Robert Richter

author

2013-06-25 16:14 UTC #20 lore raw

Re: [PATCH v2 03/14] perf: Add persistent event facilities

Quoted 7 lines · Borislav Petkov

How about
perf_add_pevent?
It is nice and short, although maybe too short but "pevent" is almost like a special term and the name shows that it is a special type of event, i.e. a p-event.

Fine with me, though it's more for internal usage. There is also use of it as name for event pointers. And, it is used in perf tools as event parser structure. But otherwise its ok.

-Robert

Robert Richter

author

2013-06-11 16:43 UTC #21 lore raw

PATCH v2 4/14 MCE: Enable persistent event

In reply to Robert Richter

From: Borislav Petkov <[email protected]>

... for MCEs collection.

Changes made by Robert Richter <[email protected]>:

The mce_record tracepoint needs tracepoints to be enabled. Fixing build error for no-tracepoints configs.

Signed-off-by Borislav Petkov <[email protected]>

[ Fix build error for no-tracepoints configs ]

Signed-off-by Robert Richter <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

arch/x86/kernel/cpu/mcheck/mce.c +18 -0

index 9239504..d421937 100644
@@ -1987,6 +1987,24 @@ int __init mcheck_init(void)
 	return 0;
 }
 
+#ifdef CONFIG_TRACEPOINTS
+
+int __init mcheck_init_tp(void)
+{
+	if (perf_add_persistent_event_by_id(event_mce_record.event.type)) {
+		pr_err("Error adding MCE persistent event.\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+/*
+ * We can't run earlier because persistent events uses anon_inode_getfile and
+ * its anon_inode_mnt gets initialized as a fs_initcall.
+ */
+fs_initcall_sync(mcheck_init_tp);
+
+#endif /* CONFIG_TRACEPOINTS */
+
 /*
  * mce_syscore: PM support
  */

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:46 UTC #22 lore raw

PATCH v2 5/14 perf, persistent: Rework struct pers_event_desc

In reply to Robert Richter

From: Robert Richter <[email protected]>

Struct pers_event_desc is only used in kernel/events/persistent.c. Moving it there. Also, removing attr member as this is a copy of event->attr.

Signed-off-by Robert Richter <[email protected]>

---
 include/linux/perf_event.h |  7 -------
 kernel/events/persistent.c | 12 ++++++++----
 2 files changed, 8 insertions(+), 11 deletions(-)

include/linux/perf_event.h +0 -7

index d2a42b7..dc72c93 100644
@@ -518,13 +518,6 @@ struct perf_output_handle {
 	int				page;
 };
 
-struct pers_event_desc {
-	struct perf_event_attr *attr;
-	struct perf_event *event;
-	struct list_head plist;
-	int fd;
-};
-
 #ifdef CONFIG_PERF_EVENTS
 
 extern int perf_pmu_register(struct pmu *pmu, char *name, int type);

kernel/events/persistent.c +8 -4

index 53411b4..d7aaf95 100644
@@ -8,6 +8,12 @@
 /* 512 kiB: default perf tools memory size, see perf_evlist__mmap() */
 #define CPU_BUFFER_NR_PAGES	((512 * 1024) / PAGE_SIZE)
 
+struct pers_event_desc {
+	struct perf_event *event;
+	struct list_head plist;
+	int fd;
+};
+
 static DEFINE_PER_CPU(struct list_head, pers_events);
 
 static struct perf_event *

@@ -33,7 +39,6 @@ add_persistent_event_on_cpu(unsigned int cpu, struct perf_event_attr *attr,
 	rcu_assign_pointer(event->rb, buf);
 
 	desc->event = event;
-	desc->attr  = attr;
 
 	INIT_LIST_HEAD(&desc->plist);
 	list_add_tail(&desc->plist, &per_cpu(pers_events, cpu));

@@ -59,7 +64,7 @@ static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 	struct perf_event *event = NULL;
 
 	list_for_each_entry_safe(desc, tmp, &per_cpu(pers_events, cpu), plist) {
-		if (desc->attr->config == attr->config) {
+		if (desc->event->attr.config == attr->config) {
 			event = desc->event;
 			break;
 		}

@@ -78,7 +83,6 @@ static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 
 	perf_event_release_kernel(event);
 	put_unused_fd(desc->fd);
-	kfree(desc->attr);
 	kfree(desc);
 }

@@ -165,7 +169,7 @@ int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
 		return -EINVAL;
 
 	list_for_each_entry(desc, &per_cpu(pers_events, cpu), plist)
-		if (desc->attr->config == attr->config)
+		if (desc->event->attr.config == attr->config)
 			return __alloc_persistent_event_fd(desc);
 
 	return -ENODEV;

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:46 UTC #23 lore raw

PATCH v2 6/14 perf, persistent: Remove rb_put()

In reply to Robert Richter

From: Robert Richter <[email protected]>

rb_put() is called already in perf_event_release_kernel(), so no need to do the same in del_persistent_event(). We also don't need it in add_persistent_event_on_cpu() after a rework. Since there are no users of rb_put() anymore, we can make it private again to only events/core.c.

Signed-off-by Robert Richter <[email protected]>

---
 kernel/events/core.c       |  4 +++-
 kernel/events/internal.h   |  1 -
 kernel/events/persistent.c | 29 +++++++++++------------------
 3 files changed, 14 insertions(+), 20 deletions(-)

kernel/events/core.c +3 -1

index a9b6470..8f85caa 100644
@@ -3021,6 +3021,8 @@ static void free_event_rcu(struct rcu_head *head)
 	kfree(event);
 }
 
+static void rb_put(struct ring_buffer *rb);
+
 static void free_event(struct perf_event *event)
 {
 	irq_work_sync(&event->pending);

@@ -3680,7 +3682,7 @@ static struct ring_buffer *rb_get(struct perf_event *event)
 	return rb;
 }
 
-void rb_put(struct ring_buffer *rb)
+static void rb_put(struct ring_buffer *rb)
 {
 	struct perf_event *event, *n;
 	unsigned long flags;

kernel/events/internal.h +0 -1

index 3b481be..3647289 100644
@@ -38,7 +38,6 @@ struct ring_buffer {
 extern void rb_free(struct ring_buffer *rb);
 extern struct ring_buffer *
 rb_alloc(int nr_pages, long watermark, int cpu, int flags);
-extern void rb_put(struct ring_buffer *rb);
 extern void perf_event_wakeup(struct perf_event *event);
 
 extern void

kernel/events/persistent.c +11 ~~-18~~

index d7aaf95..e6a7664 100644
@@ -20,22 +20,22 @@ static struct perf_event *
 add_persistent_event_on_cpu(unsigned int cpu, struct perf_event_attr *attr,
 			    unsigned nr_pages)
 {
-	struct perf_event *event = ERR_PTR(-ENOMEM);
+	struct perf_event *event;
 	struct pers_event_desc *desc;
 	struct ring_buffer *buf;
 
 	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
 	if (!desc)
-		goto out;
-
-	buf = rb_alloc(nr_pages, 0, cpu, 0);
-	if (!buf)
-		goto err_rb;
+		return ERR_PTR(-ENOMEM);
 
 	event = perf_event_create_kernel_counter(attr, cpu, NULL, NULL, NULL);
 	if (IS_ERR(event))
 		goto err_event;
 
+	buf = rb_alloc(nr_pages, 0, cpu, 0);
+	if (!buf)
+		goto err_rb;
+
 	rcu_assign_pointer(event->rb, buf);
 
 	desc->event = event;

@@ -47,14 +47,12 @@ add_persistent_event_on_cpu(unsigned int cpu, struct perf_event_attr *attr,
 	perf_event_enable(event);
 
 	goto out;
-
- err_event:
-	rb_put(buf);
-
- err_rb:
+err_rb:
+	perf_event_release_kernel(event);
+	event = ERR_PTR(-ENOMEM);
+err_event:
 	kfree(desc);
-
- out:
+out:
 	return event;
 }

@@ -76,11 +74,6 @@ static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 	list_del(&desc->plist);
 
 	perf_event_disable(event);
-	if (event->rb) {
-		rb_put(event->rb);
-		rcu_assign_pointer(event->rb, NULL);
-	}
-
 	perf_event_release_kernel(event);
 	put_unused_fd(desc->fd);
 	kfree(desc);

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:43 UTC #24 lore raw

PATCH v2 7/14 perf, persistent: Introduce get_persistent_event()

In reply to Robert Richter

From: Robert Richter <[email protected]>

Reducing duplicate code by introducing function get_persistent_event() to get the descriptor of an persistent event.

Signed-off-by Robert Richter <[email protected]>

---
 kernel/events/persistent.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

kernel/events/persistent.c +22 ~~-14~~

index e6a7664..7d91871 100644
@@ -16,6 +16,19 @@ struct pers_event_desc {
 
 static DEFINE_PER_CPU(struct list_head, pers_events);
 
+static struct pers_event_desc
+*get_persistent_event(int cpu, struct perf_event_attr *attr)
+{
+	struct pers_event_desc *desc;
+
+	list_for_each_entry(desc, &per_cpu(pers_events, cpu), plist) {
+		if (desc->event->attr.config == attr->config)
+			return desc;
+	}
+
+	return NULL;
+}
+
 static struct perf_event *
 add_persistent_event_on_cpu(unsigned int cpu, struct perf_event_attr *attr,
 			    unsigned nr_pages)

@@ -58,18 +71,13 @@ out:
 
 static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 {
-	struct pers_event_desc *desc, *tmp;
-	struct perf_event *event = NULL;
-
-	list_for_each_entry_safe(desc, tmp, &per_cpu(pers_events, cpu), plist) {
-		if (desc->event->attr.config == attr->config) {
-			event = desc->event;
-			break;
-		}
-	}
+	struct pers_event_desc *desc;
+	struct perf_event *event;
 
-	if (!event)
+	desc = get_persistent_event(cpu, attr);
+	if (!desc)
 		return;
+	event = desc->event;
 
 	list_del(&desc->plist);

@@ -161,11 +169,11 @@ int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
 	if (cpu >= (unsigned)nr_cpu_ids)
 		return -EINVAL;
 
-	list_for_each_entry(desc, &per_cpu(pers_events, cpu), plist)
-		if (desc->event->attr.config == attr->config)
-			return __alloc_persistent_event_fd(desc);
+	desc = get_persistent_event(cpu, attr);
+	if (!desc)
+		return -ENODEV;
 
-	return -ENODEV;
+	return __alloc_persistent_event_fd(desc);
 }

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:43 UTC #25 lore raw

PATCH v2 8/14 perf, persistent: Reworking perf_get_persistent_event_fd()

In reply to Robert Richter

From: Robert Richter <[email protected]>

There is already a function anon_inode_getfd() that does already all the work. Reworking and simplifying code.

Signed-off-by Robert Richter <[email protected]>

---
 kernel/events/persistent.c | 35 +++++++----------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

kernel/events/persistent.c +7 ~~-28~~

index 7d91871..16ed47c 100644
@@ -87,33 +87,6 @@ static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 	kfree(desc);
 }
 
-static int __alloc_persistent_event_fd(struct pers_event_desc *desc)
-{
-	struct file *event_file = NULL;
-	int event_fd = -1;
-
-	event_fd = get_unused_fd();
-	if (event_fd < 0)
-		goto out;
-
-	event_file = anon_inode_getfile("[pers_event]", &perf_fops,
-					desc->event, O_RDONLY);
-	if (IS_ERR(event_file))
-		goto err_event_file;
-
-	desc->fd = event_fd;
-	fd_install(event_fd, event_file);
-
-	goto out;
-
-
- err_event_file:
-	put_unused_fd(event_fd);
-
- out:
-	return event_fd;
-}
-
 /*
  * Create and enable the persistent version of the perf event described by
  * @attr.

@@ -165,6 +138,7 @@ int perf_add_persistent_event_by_id(int id)
 int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
 {
 	struct pers_event_desc *desc;
+	int event_fd;
 
 	if (cpu >= (unsigned)nr_cpu_ids)
 		return -EINVAL;

@@ -173,7 +147,12 @@ int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
 	if (!desc)
 		return -ENODEV;
 
-	return __alloc_persistent_event_fd(desc);
+	event_fd = anon_inode_getfd("[pers_event]", &perf_fops,
+				desc->event, O_RDONLY);
+	if (event_fd >= 0)
+		desc->fd = event_fd;
+
+	return event_fd;
 }

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:45 UTC #26 lore raw

PATCH v2 9/14 perf, persistent: Protect event lists with mutex

In reply to Robert Richter

From: Robert Richter <[email protected]>

Protect esp. access to struct pers_event_desc *desc. There are race conditions possible where the descriptor could be removed from list while it is used.

Signed-off-by Robert Richter <[email protected]>

---
 kernel/events/persistent.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

kernel/events/persistent.c +24 -9

index 16ed47c..586cea5 100644
@@ -15,6 +15,7 @@ struct pers_event_desc {
 };
 
 static DEFINE_PER_CPU(struct list_head, pers_events);
+static DEFINE_PER_CPU(struct mutex, pers_events_lock);
 
 static struct pers_event_desc
 *get_persistent_event(int cpu, struct perf_event_attr *attr)

@@ -37,9 +38,13 @@ add_persistent_event_on_cpu(unsigned int cpu, struct perf_event_attr *attr,
 	struct pers_event_desc *desc;
 	struct ring_buffer *buf;
 
+	mutex_lock(&per_cpu(pers_events_lock, cpu));
+
 	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
-	if (!desc)
-		return ERR_PTR(-ENOMEM);
+	if (!desc) {
+		event = ERR_PTR(-ENOMEM);
+		goto out;
+	}
 
 	event = perf_event_create_kernel_counter(attr, cpu, NULL, NULL, NULL);
 	if (IS_ERR(event))

@@ -66,6 +71,7 @@ err_rb:
 err_event:
 	kfree(desc);
 out:
+	mutex_unlock(&per_cpu(pers_events_lock, cpu));
 	return event;
 }

@@ -74,9 +80,11 @@ static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 	struct pers_event_desc *desc;
 	struct perf_event *event;
 
+	mutex_lock(&per_cpu(pers_events_lock, cpu));
+
 	desc = get_persistent_event(cpu, attr);
 	if (!desc)
-		return;
+		goto out;
 	event = desc->event;
 
 	list_del(&desc->plist);

@@ -85,6 +93,8 @@ static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 	perf_event_release_kernel(event);
 	put_unused_fd(desc->fd);
 	kfree(desc);
+out:
+	mutex_unlock(&per_cpu(pers_events_lock, cpu));
 }
 
 /*

@@ -138,28 +148,33 @@ int perf_add_persistent_event_by_id(int id)
 int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
 {
 	struct pers_event_desc *desc;
-	int event_fd;
+	int event_fd = -ENODEV;
 
 	if (cpu >= (unsigned)nr_cpu_ids)
 		return -EINVAL;
 
+	mutex_lock(&per_cpu(pers_events_lock, cpu));
+
 	desc = get_persistent_event(cpu, attr);
 	if (!desc)
-		return -ENODEV;
+		goto out;
 
 	event_fd = anon_inode_getfd("[pers_event]", &perf_fops,
 				desc->event, O_RDONLY);
 	if (event_fd >= 0)
 		desc->fd = event_fd;
+out:
+	mutex_unlock(&per_cpu(pers_events_lock, cpu));
 
 	return event_fd;
 }
 
-
 void __init persistent_events_init(void)
 {
-	int i;
+	int cpu;
 
-	for_each_possible_cpu(i)
-		INIT_LIST_HEAD(&per_cpu(pers_events, i));
+	for_each_possible_cpu(cpu) {
+		INIT_LIST_HEAD(&per_cpu(pers_events, cpu));
+		mutex_init(&per_cpu(pers_events_lock, cpu));
+	}
 }

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:45 UTC #27 lore raw

PATCH v2 10/14 perf, persistent: Avoid adding identical events

In reply to Robert Richter

From: Robert Richter <[email protected]>

Check if an event already exists before adding it.

Signed-off-by Robert Richter <[email protected]>

---
 kernel/events/persistent.c | 6 ++++++
 1 file changed, 6 insertions(+)

kernel/events/persistent.c +6 -0

index 586cea5..4fcd071 100644
@@ -40,6 +40,12 @@ add_persistent_event_on_cpu(unsigned int cpu, struct perf_event_attr *attr,
 
 	mutex_lock(&per_cpu(pers_events_lock, cpu));
 
+	desc = get_persistent_event(cpu, attr);
+	if (desc) {
+		event = ERR_PTR(-EEXIST);
+		goto out;
+	}
+
 	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
 	if (!desc) {
 		event = ERR_PTR(-ENOMEM);

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:43 UTC #28 lore raw

PATCH v2 11/14 perf, persistent: Implementing a persistent pmu

In reply to Robert Richter

From: Robert Richter <[email protected]>

We want to use the kernel's pmu design to later expose persistent events via sysfs to userland. Initially implement a persistent pmu.

The format syntax is introduced allowing to set bits anywhere in struct perf_event_attr. This is used in this case to set the persistent flag (attr5:23). The syntax is attr<num> where num is the index of the u64 array in struct perf_event_attr. Otherwise syntax is same as for config<num>.

Patches that implement this functionality for perf tools are sent in a separate patchset.

Signed-off-by Robert Richter <[email protected]>

---
 kernel/events/persistent.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

kernel/events/persistent.c +35 -0

index 4fcd071..97c57c9 100644
@@ -175,10 +175,45 @@ out:
 	return event_fd;
 }
 
+PMU_FORMAT_ATTR(persistent, "attr5:23");
+
+static struct attribute *persistent_format_attrs[] = {
+	&format_attr_persistent.attr,
+	NULL,
+};
+
+static struct attribute_group persistent_format_group = {
+	.name = "format",
+	.attrs = persistent_format_attrs,
+};
+
+static const struct attribute_group *persistent_attr_groups[] = {
+	&persistent_format_group,
+	NULL,
+};
+
+static struct pmu persistent_pmu;
+
+static int persistent_pmu_init(struct perf_event *event)
+{
+	if (persistent_pmu.type != event->attr.type)
+		return -ENOENT;
+
+	/* Not a persistent event. */
+	return -EFAULT;
+}
+
+static struct pmu persistent_pmu = {
+	.event_init	= persistent_pmu_init,
+	.attr_groups	= persistent_attr_groups,
+};
+
 void __init persistent_events_init(void)
 {
 	int cpu;
 
+	perf_pmu_register(&persistent_pmu, "persistent", -1);
+
 	for_each_possible_cpu(cpu) {
 		INIT_LIST_HEAD(&per_cpu(pers_events, cpu));
 		mutex_init(&per_cpu(pers_events_lock, cpu));

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:44 UTC #29 lore raw

PATCH v2 12/14 perf, persistent: Name each persistent event

In reply to Robert Richter

From: Robert Richter <[email protected]>

For later adding persistent events to sysfs we need a name for each event. Adding a name to each persistent event.

Signed-off-by Robert Richter <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |  3 ++-
 include/linux/perf_event.h       |  4 ++--
 kernel/events/persistent.c       | 30 +++++++++++++++++++++++++-----
 3 files changed, 29 insertions(+), 8 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +2 -1

index d421937..833eb7a 100644
@@ -1991,7 +1991,8 @@ int __init mcheck_init(void)
 
 int __init mcheck_init_tp(void)
 {
-	if (perf_add_persistent_event_by_id(event_mce_record.event.type)) {
+	if (perf_add_persistent_event_by_id(event_mce_record.name,
+					    event_mce_record.event.type)) {
 		pr_err("Error adding MCE persistent event.\n");
 		return -EINVAL;
 	}

include/linux/perf_event.h +2 -2

index dc72c93..06b4357b 100644
@@ -751,7 +751,7 @@ extern void perf_event_disable(struct perf_event *event);
 extern int __perf_event_disable(void *info);
 extern void perf_event_task_tick(void);
 extern int perf_add_persistent_event(struct perf_event_attr *, unsigned);
-extern int perf_add_persistent_event_by_id(int id);
+extern int perf_add_persistent_event_by_id(char *name, int id);
 #else /* !CONFIG_PERF_EVENTS */
 static inline void
 perf_event_task_sched_in(struct task_struct *prev,

@@ -794,7 +794,7 @@ static inline int __perf_event_disable(void *info)			{ return -1; }
 static inline void perf_event_task_tick(void)				{ }
 static inline int perf_add_persistent_event(struct perf_event_attr *attr,
 					    unsigned nr_pages)		{ return -EINVAL; }
-static inline int perf_add_persistent_event_by_id(int id)		{ return -EINVAL; }
+static inline int perf_add_persistent_event_by_id(char *name, int id)	{ return -EINVAL; }
 #endif /* !CONFIG_PERF_EVENTS */
 
 #if defined(CONFIG_PERF_EVENTS) && defined(CONFIG_NO_HZ_FULL)

kernel/events/persistent.c +25 -5

index 97c57c9..96201c1 100644
@@ -14,6 +14,11 @@ struct pers_event_desc {
 	int fd;
 };
 
+struct pers_event {
+	char				*name;
+	struct perf_event_attr		attr;
+};
+
 static DEFINE_PER_CPU(struct list_head, pers_events);
 static DEFINE_PER_CPU(struct mutex, pers_events_lock);

@@ -132,14 +137,20 @@ unwind:
 	return PTR_ERR(event);
 }
 
-int perf_add_persistent_event_by_id(int id)
+int perf_add_persistent_event_by_id(char* name, int id)
 {
-	struct perf_event_attr *attr;
+	struct pers_event	*event;
+	struct perf_event_attr	*attr;
+	int ret = -ENOMEM;
 
-	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-	if (!attr)
+	event = kzalloc(sizeof(*event), GFP_KERNEL);
+	if (!event)
 		return -ENOMEM;
+	event->name = kstrdup(name, GFP_KERNEL);
+	if (!event->name)
+		goto fail;
 
+	attr = &event->attr;
 	attr->sample_period	= 1;
 	attr->wakeup_events	= 1;
 	attr->sample_type	= PERF_SAMPLE_RAW;

@@ -148,7 +159,16 @@ int perf_add_persistent_event_by_id(int id)
 	attr->type		= PERF_TYPE_TRACEPOINT;
 	attr->size		= sizeof(*attr);
 
-	return perf_add_persistent_event(attr, CPU_BUFFER_NR_PAGES);
+	ret = perf_add_persistent_event(attr, CPU_BUFFER_NR_PAGES);
+	if (ret)
+		goto fail;
+
+	return 0;
+fail:
+	kfree(event->name);
+	kfree(event);
+
+	return ret;
 }
 
 int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)

Signature · 1 line

1.8.1.1

Robert Richter

author

2013-06-11 16:43 UTC #30 lore raw

PATCH v2 13/14 perf, persistent: Exposing persistent events using sysfs

In reply to Robert Richter

From: Robert Richter <[email protected]>

Expose persistent events in the system to userland using sysfs. Perf tools are able to read existing pmu events from sysfs. Now we use a persistent pmu as an event container containing all registered persistent events of the system. This patch adds dynamically registration of persistent events to sysfs. E.g. something like this:

/sys/bus/event_source/devices/persistent/events/mce_record:persistent,config=106 /sys/bus/event_source/devices/persistent/format/persistent:attr5:23

Perf tools need to support the attr<num> syntax that is added in a separate patch set. With it we are able to run perf tool commands to read persistent events, e.g.:

 # perf record -e persistent/mce_record/ sleep 10
 # perf top -e persistent/mce_record/

[ Document attr<index> syntax in sysfs ABI ]

Reported-by Jiri Olsa <[email protected]>

Signed-off-by Robert Richter <[email protected]>

---
 .../testing/sysfs-bus-event_source-devices-format  | 43 ++++++++++++-----
 kernel/events/persistent.c                         | 55 +++++++++++++++++++++-
 2 files changed, 86 insertions(+), 12 deletions(-)

Documentation/ABI/testing/sysfs-bus-event_source-devices-format +32 ~~-11~~

index 77f47ff..47b7353 100644
@@ -1,13 +1,14 @@
-Where:		/sys/bus/event_source/devices/<dev>/format
+Where:		/sys/bus/event_source/devices/<pmu>/format/<name>
 Date:		January 2012
-Kernel Version: 3.3
+Kernel Version:	3.3
+		3.12	(added attr<index>:<bits>)
 Contact:	Jiri Olsa <[email protected]>
-Description:
-		Attribute group to describe the magic bits that go into
-		perf_event_attr::config[012] for a particular pmu.
-		Each attribute of this group defines the 'hardware' bitmask
-		we want to export, so that userspace can deal with sane
-		name/value pairs.
+
+Description:	Define formats for bit ranges in perf_event_attr
+
+		Attribute group to describe the magic bits that go
+		into struct perf_event_attr for a particular pmu. Bit
+		range may be any bit mask of an u64 (bits 0 to 63).
 
 		Userspace must be prepared for the possibility that attributes
 		define overlapping bit ranges. For example:

@@ -15,6 +16,26 @@ Description:
 			attr2 = 'config:0-7'
 			attr3 = 'config:12-35'
 
-		Example: 'config1:1,6-10,44'
-		Defines contents of attribute that occupies bits 1,6-10,44 of
-		perf_event_attr::config1.
+		Syntax			Description
+
+		config[012]*:<bits>	Each attribute of this group
+					defines the 'hardware' bitmask
+					we want to export, so that
+					userspace can deal with sane
+					name/value pairs.
+
+		attr<index>:<bits>	Set any field of the event
+					attribute. The index is a
+					decimal number that specifies
+					the u64 value to be set within
+					struct perf_event_attr.
+
+		Examples:
+
+		'config1:1,6-10,44'	Defines contents of attribute
+					that occupies bits 1,6-10,44
+					of perf_event_attr::config1.
+
+		'attr5:23'		Define the persistent event
+					flag (bit 23 of the attribute
+					flags)

kernel/events/persistent.c +54 -1

92 lines not on this page · raw message

Signature · 1 line

1.8.1.1

Namhyung Kim

maintainer

2013-06-14 02:36 UTC #31 lore raw

Re: [PATCH v2 13/14] perf, persistent: Exposing persistent events using sysfs

Quoted 23 lines · Robert Richter

From: Robert Richter <[email protected]>
Expose persistent events in the system to userland using sysfs. Perf tools are able to read existing pmu events from sysfs. Now we use a persistent pmu as an event container containing all registered persistent events of the system. This patch adds dynamically registration of persistent events to sysfs. E.g. something like this:
/sys/bus/event_source/devices/persistent/events/mce_record:persistent,config=106 /sys/bus/event_source/devices/persistent/format/persistent:attr5:23
Perf tools need to support the attr<num> syntax that is added in a separate patch set. With it we are able to run perf tool commands to read persistent events, e.g.:
 # perf record -e persistent/mce_record/ sleep 10
 # perf top -e persistent/mce_record/
[ Document attr<index> syntax in sysfs ABI ]
Reported-by Jiri Olsa <[email protected]>
Signed-off-by Robert Richter <[email protected]>
Signed-off-by Robert Richter <[email protected]>
---

[SNIP]

kernel/events/persistent.c static struct attribute_group persistent_format_group = { · line 250

Quoted 8 lines

kernel/events/persistent.c +8 -0

+static int pers_event_sysfs_register(struct pers_event *event)
+{
+	struct device_attribute *attr = &event->sysfs.attr;
+	int idx;
+
+	*attr = (struct device_attribute)__ATTR(, 0444, pers_event_sysfs_show,
+						NULL);
+	attr->attr.name = event->name;

When I added another persistent event with this API, I got an WARNING from lockdep like this:

[    0.432506] BUG: key ffff88040946f140 not in .data!
[    0.432581] ------------[ cut here ]------------
[    0.432656] WARNING: at /home/namhyung/project/linux/kernel/lockdep.c:2987 lockdep_init_map+0x53d/0x570()
[    0.432763] DEBUG_LOCKS_WARN_ON(1)

I guess we need the following line here:

	sysfs_attr_init(&attr->attr);

Thanks,
Namhyung

Quoted 21 lines

kernel/events/persistent.c +18 -0

+
+	/* add sysfs attr to events: */
+	for (idx = 0; idx < MAX_EVENTS; idx++) {
+		if (!cmpxchg(persistent_events_attr + idx, NULL, &attr->attr))
+			break;
+	}
+
+	if (idx >= MAX_EVENTS)
+		return -ENOSPC;
+	if (!idx)
+		EVENTS_GROUP = &persistent_events_group;
+	if (!persistent_pmu.dev)
+		return 0;	/* sysfs not yet initialized */
+	if (idx)
+		return sysfs_update_group(&persistent_pmu.dev->kobj,
+					EVENTS_GROUP);
+	return sysfs_create_group(&persistent_pmu.dev->kobj, EVENTS_GROUP);
+}
 
 static int persistent_pmu_init(struct perf_event *event)
 {

Robert Richter

author

2013-06-14 08:57 UTC #32 lore raw

Re: [PATCH v2 13/14] perf, persistent: Exposing persistent events using sysfs

Quoted 18 lines · Namhyung Kim

kernel/events/persistent.c static struct attribute_group persistent_format_group = { · line 250

Quoted 8 lines

kernel/events/persistent.c +8 -0

+static int pers_event_sysfs_register(struct pers_event *event)
+{
+	struct device_attribute *attr = &event->sysfs.attr;
+	int idx;
+
+	*attr = (struct device_attribute)__ATTR(, 0444, pers_event_sysfs_show,
+						NULL);
+	attr->attr.name = event->name;

When I added another persistent event with this API, I got an WARNING from lockdep like this:

[    0.432506] BUG: key ffff88040946f140 not in .data!
[    0.432581] ------------[ cut here ]------------
[    0.432656] WARNING: at /home/namhyung/project/linux/kernel/lockdep.c:2987 lockdep_init_map+0x53d/0x570()
[    0.432763] DEBUG_LOCKS_WARN_ON(1)

Quoted 3 lines

I guess we need the following line here:
	sysfs_attr_init(&attr->attr);

Yes, added your change. Thanks Namhyung for reviewing and testing.

-Robert

Robert Richter

author

2013-06-11 16:43 UTC #33 lore raw

PATCH v2 14/14 perf, persistent: Allow multiple users for an event

In reply to Robert Richter

From: Robert Richter <[email protected]>

Usually a fd close leads to the release of the event too. For persistent events this is different as the events should be permanently enabled in the system. Using reference counting to avoid releasing an event during a fd close. This also allows it to have multiple users (open file descriptors) for a single persistent event.

While at this, we don't need desc->fd any longer. The fd is attached to a task and reference counting keeps the event. Removing desc->fd.

Signed-off-by Robert Richter <[email protected]>

---
 kernel/events/persistent.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

kernel/events/persistent.c +36 ~~-10~~

index 8be7c05..dd20b55 100644
@@ -11,7 +11,6 @@
 struct pers_event_desc {
 	struct perf_event *event;
 	struct list_head plist;
-	int fd;
 };
 
 struct pers_event {

@@ -88,6 +87,18 @@ out:
 	return event;
 }
 
+static void detach_persistent_event(struct pers_event_desc *desc)
+{
+	list_del(&desc->plist);
+	kfree(desc);
+}
+
+static void release_persistent_event(struct perf_event *event)
+{
+	perf_event_disable(event);
+	perf_event_release_kernel(event);
+}
+
 static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 {
 	struct pers_event_desc *desc;

@@ -100,12 +111,14 @@ static void del_persistent_event(int cpu, struct perf_event_attr *attr)
 		goto out;
 	event = desc->event;
 
-	list_del(&desc->plist);
-
-	perf_event_disable(event);
-	perf_event_release_kernel(event);
-	put_unused_fd(desc->fd);
-	kfree(desc);
+	/*
+	 * We primarily want to remove desc from the list. If there
+	 * are no open files, the refcount is 0 and we need to release
+	 * the event too.
+	 */
+	detach_persistent_event(desc);
+	if (atomic_long_dec_and_test(&event->refcount))
+		release_persistent_event(event);
 out:
 	mutex_unlock(&per_cpu(pers_events_lock, cpu));
 }

@@ -182,6 +195,7 @@ fail:
 int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
 {
 	struct pers_event_desc *desc;
+	struct perf_event *event;
 	int event_fd = -ENODEV;
 
 	if (cpu >= (unsigned)nr_cpu_ids)

@@ -190,13 +204,25 @@ int perf_get_persistent_event_fd(unsigned cpu, struct perf_event_attr *attr)
 	mutex_lock(&per_cpu(pers_events_lock, cpu));
 
 	desc = get_persistent_event(cpu, attr);
-	if (!desc)
+
+	/* Increment refcount to keep event on put_event() */
+	if (!desc || !atomic_long_inc_not_zero(&desc->event->refcount))
 		goto out;
 
 	event_fd = anon_inode_getfd("[pers_event]", &perf_fops,
 				desc->event, O_RDONLY);
-	if (event_fd >= 0)
-		desc->fd = event_fd;
+
+	if (event_fd < 0) {
+		event = desc->event;
+		if (WARN_ON(atomic_long_dec_and_test(&event->refcount))) {
+			/*
+			 * May not happen since decrementing refcount is
+			 * protected by pers_events_lock.
+			 */
+			detach_persistent_event(desc);
+			release_persistent_event(event);
+		}
+	}
 out:
 	mutex_unlock(&per_cpu(pers_events_lock, cpu));

Signature · 1 line

1.8.1.1

Peter Zijlstra

maintainer

2013-06-24 10:08 UTC #34 lore raw

In reply to Robert Richter

Quoted 39 lines · Robert Richter

This patch set implements out of the box support of perf tools for persistent events. For this the kernel must provide necessary information about existing persistent events via sysfs to userland. Persistent events are provided by the kernel with readonly event buffers. To allow the independent usage of the event buffers by any process without limiting other processes, multiple users of a single event must be handled too.
The basic concept is to use a pmu as an event container for persistent events. The pmu registers events in sysfs and provides format and event information for the userland. The persistent event framework requires to add events to the pmu dynamically.
With the information in sysfs userland knows about how to setup the perf_event attribute of a persistent event. Since a persistent event always has the persistent flag set, a way is needed to express this in sysfs. A new syntax is used for this. With 'attr<num>:<mask>' any bit in the attribute structure may be set in a similar way as using 'config<num>', but <num> is an index that points to the u64 value to change within the attribute.
For persistent events the persistent flag (bit 23 of flag field in struct perf_event_attr) needs to be set which is expressed in sysfs with "attr5:23". E.g. the mce_record event is described in sysfs as follows:
/sys/bus/event_source/devices/persistent/events/mce_record:persistent,config=106 /sys/bus/event_source/devices/persistent/format/persistent:attr5:23
Note that perf tools need to support the 'attr<num>' syntax that is added in a separate patch set. With it we are able to run perf tool commands to read persistent events, e.g.:
 # perf record -e persistent/mce_record/ sleep 10
 # perf top -e persistent/mce_record/
In general the new syntax is flexible to describe with sysfs any event to be setup by perf tools.

Quoted 8 lines

First patches contain also fixes and reworks made after reviewing code. Could be applied separately.
Patches base on Boris' patches which I have rebased to latest tip/perf/core. All patches can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git persistent-v2

OK, so I gave up on reading the patches :/ and went and looked at the git tree. There's just too much needless churn in the patches.

As already said somewhere else; get_persistent_event() -- although that wasn't the name at the time -- needs a better match function.

+ int perf_add_persistent_event(struct perf_event_attr *attr, unsigned nr_pages)
+ {
+ struct perf_event *event;
+ int i;
+
+ for_each_possible_cpu(i) {
+ event = add_persistent_event_on_cpu(i, attr, nr_pages);
+ if (IS_ERR(event))
+ goto unwind;
+ }
+ return 0;
+
+ unwind:
+ pr_err("%s: Error adding persistent event on cpu %d: %ld\n",
+ __func__, i, PTR_ERR(event));
+
+ while (--i >= 0)
+ del_persistent_event(i, attr);
+
+ return PTR_ERR(event);
+ }

This assumes cpu_possible_mask doesn't have holes in; I'm fairly sure we cannot assume such.

Furthermore; while the function is exposed and thus looks like a generic usable thing; however it looks to 'forget' making a sysfs entry, making it look like something incomplete.

Only the ill named perf_add_persistent_event_by_id() function looks something that's usable outside of this.

What's with MAX_EVENTS ?

Robert Richter

author

2013-06-25 10:46 UTC #35 lore raw

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines
git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git persistent-v2
OK, so I gave up on reading the patches :/ and went and looked at the git tree. There's just too much needless churn in the patches.

Ok, we will rework the patches to have a final patch set hiding all reworks. I was hoping we could work on a public branch since this eases developing code with multiple people working on it. This also shows how code matures. But this seems not to fly.

Quoted 22 lines

+ int perf_add_persistent_event(struct perf_event_attr *attr, unsigned nr_pages)
+ {

(quoted diff) +19 -0

+ 	struct perf_event *event;
+ 	int i;
+ 
+ 	for_each_possible_cpu(i) {
+ 		event = add_persistent_event_on_cpu(i, attr, nr_pages);
+ 		if (IS_ERR(event))
+ 			goto unwind;
+ 	}
+ 	return 0;
+ 
+ unwind:
+ 	pr_err("%s: Error adding persistent event on cpu %d: %ld\n",
+ 		__func__, i, PTR_ERR(event));
+ 
+ 	while (--i >= 0)
+ 		del_persistent_event(i, attr);
+ 
+ 	return PTR_ERR(event);
+ }

Quoted 2 lines

This assumes cpu_possible_mask doesn't have holes in; I'm fairly sure we cannot assume such.

Right, needs to be fixed.

Quoted 6 lines

Furthermore; while the function is exposed and thus looks like a generic usable thing; however it looks to 'forget' making a sysfs entry, making it look like something incomplete.
Only the ill named perf_add_persistent_event_by_id() function looks something that's usable outside of this.

Right, the name should be actually perf_add_persistent_tp() or so.

And, most of the function should be merged with perf_add_persistent_event(). Maybe perf_add_persistent_event_by_id() could be removed completly leaving perf_add_persistent_tp() as wrapper for tracepoints.

Quoted 1 line

What's with MAX_EVENTS ?

It is the total number of sysfs entries that can currently be registered dynamically. The problem with an unlimited event number is that the sysfs attr list is a fixed array. We need to resize the array which involves copying entries including locking etc. This was left for later. ;)

Btw, another thing left for later is cpu hotplug code. I know this needs to be implemented. But we want to see something running first. Do you know what happens to system-wide events if a cpu is offline? It looks like cpu_function_call() in perf_install_in_context() simply ignores to install an event on an offline cpu. Not sure if it is later enabled while bringing the cpu online. Quick looking at the code seems to be not.

-Robert

Peter Zijlstra

maintainer

2013-06-24 10:22 UTC #36 lore raw

In reply to Robert Richter

Quoted 3 lines · Robert Richter

Note that perf tools need to support the 'attr<num>' syntax that is added in a separate patch set. With it we are able to run perf tool commands to read persistent events, e.g.:

where is this patch? I can't find it.

I also find attr<num>:<bit> a bit weird. So far we've used <perf_event_attr::fieldname>:<bits>, so while initializing anonymous unions is a bit difficult with 'older' GCCs and we cannot actually do:

struct perf_event_attr {
	...
	union {
		u64 flags;
		u64 disabled : 1,
		    ...
		    __reserved_1 : X;
	}
	...
};

We could fake it in userspace by allowing things like: flags:23. It would not be a much worse hack than attr<num>:<bit> I suppose.

Robert Richter

author

2013-06-25 16:56 UTC #37 lore raw

Quoted 6 lines · Peter Zijlstra

Quoted 3 lines · Robert Richter
Note that perf tools need to support the 'attr<num>' syntax that is added in a separate patch set. With it we are able to run perf tool commands to read persistent events, e.g.:
where is this patch? I can't find it.

I posted it with that subject:

[PATCH 0/4] perf tools: Persistent events, changes for perf tool integration

Quoted 15 lines

I also find attr<num>:<bit> a bit weird. So far we've used <perf_event_attr::fieldname>:<bits>, so while initializing anonymous unions is a bit difficult with 'older' GCCs and we cannot actually do:
struct perf_event_attr {
	...
	union {
		u64 flags;
		u64 disabled : 1,
		    ...
		    __reserved_1 : X;
	}
	...
};

Quoted 2 lines

We could fake it in userspace by allowing things like: flags:23. It would not be a much worse hack than attr<num>:<bit> I suppose.

A goal is to be able to entirely describe the complete attr structure with sysfs. perf tools should work out-of-the-box for every kind of event. For this particular case a 'flags' syntax would be ok, but not if you like to fill in other attr members.

With attr<num>:<bit> you are flexible to put in *everything* into attr. And for better readability there are abstractions as formats like:

/sys/bus/event_source/devices/persistent/format/persistent:attr5:23

Not sure if the event parser handles recursion already, but we could also support the 'flag' format you describe that way:

/sys/bus/event_source/devices/persistent/format/flags:attr5:0-63 /sys/bus/event_source/devices/persistent/format/persistent:flags:23

We could even list events for other pmus with small changes in the kernel, e.g:

/sys/bus/event_source/devices/persistent/format/type:attr0:0-31 /sys/bus/event_source/devices/persistent/events/some_tracepoint:type=2,config=<tp_id>

The example is a bit weird. But in general we can translate struct perf_event_attr to a string and vice versa. And perf tools understand how to set up every bit of the event attr structure.

-Robert

Peter Zijlstra

maintainer

2013-06-24 10:24 UTC #38 lore raw

In reply to Robert Richter

Quoted 13 lines · Robert Richter

The basic concept is to use a pmu as an event container for persistent events. The pmu registers events in sysfs and provides format and event information for the userland. The persistent event framework requires to add events to the pmu dynamically.
With the information in sysfs userland knows about how to setup the perf_event attribute of a persistent event. Since a persistent event always has the persistent flag set, a way is needed to express this in sysfs. A new syntax is used for this. With 'attr<num>:<mask>' any bit in the attribute structure may be set in a similar way as using 'config<num>', but <num> is an index that points to the u64 value to change within the attribute.

Quoted 8 lines

For persistent events the persistent flag (bit 23 of flag field in struct perf_event_attr) needs to be set which is expressed in sysfs with "attr5:23". E.g. the mce_record event is described in sysfs as follows:
/sys/bus/event_source/devices/persistent/events/mce_record:persistent,config=106 /sys/bus/event_source/devices/persistent/format/persistent:attr5:23

Ok so it took me a little to 'get' this PMU 'abuse', but I suppose I can go along with it ;-)

On the specifics though; it again seems to hard assume persistent events are for tracepoints only -- either make this very explicit or remove it.

Peter Zijlstra

maintainer

2013-06-24 15:26 UTC #39 lore raw

In reply to Robert Richter

On Tue, Jun 11, 2013 at 06:42:26PM +0200, Robert Richter wrote:

Oh and what Boris and Ingo said; persistent events should 'persist' and not be tied to particular processes. I'm not sure about the entire eventfs thing; but the proposed sysfs thing should definitely work for now.

Ingo Molnar

2013-06-24 19:45 UTC #40 lore raw

Quoted 6 lines · Peter Zijlstra

On Tue, Jun 11, 2013 at 06:42:26PM +0200, Robert Richter wrote:
Oh and what Boris and Ingo said; persistent events should 'persist' and not be tied to particular processes. I'm not sure about the entire eventfs thing; but the proposed sysfs thing should definitely work for now.

I'm fine with a sysfs approach as well, as long as it's correct and obvious enough to use.

Thanks,

Ingo

Robert Richter

author

2013-06-25 17:57 UTC #41 lore raw

Quoted 5 lines · Ingo Molnar

Quoted 2 lines · Peter Zijlstra
Oh and what Boris and Ingo said; persistent events should 'persist' and not be tied to particular processes.

Fine with me too. But we need an answer how to create/release persistent events.

It's easy to add them as in-kernel events in the kernel. But, how could persistent events be created from userspace without inventing too much new? As for standard events this could be done with the perf syscall too. The event is created by a process and buffers are also setup using the event's file descriptor. After the setup we detach the event from the process. The event is persistent now. We could release an event the same way by attaching it to a process an then closing it.

Detaching an event from a process basically means the event is not released after the process is closing the event or is killed itself. Sharing an event between processes is very important for this. Sharing requires the event buffer to be accessed readonly and the event's ref count needs to keep track of all attached processes. As long as a process is using the event it is not released.

But what options there are to detach the event from all processes and make it persistent? We could just put the creating process to sleep as long as the event should be persistent. This seems not to be an option for you. There could be other ways to just increase the refcount. We could use a ftrace-like approach and modify the refcount by:

Detach:

 # echo opened_event > /sys/bus/event_source/devices/persistent/detached

Release:

 # echo '!opened_event' > /sys/bus/event_source/devices/persistent/detached

And Ingo's proposal using eventfs somehow:

/sys/fs/events/user/PID/

And/or cgroups?

Are there other suggestions?

Quoted 6 lines

Quoted 3 lines
I'm not sure about the entire
eventfs thing; but the proposed sysfs thing should definitely work for now.
I'm fine with a sysfs approach as well, as long as it's correct and obvious enough to use.

Glad to hear this. :)

Thanks,

-Robert

Borislav Petkov

maintainer

2013-06-25 19:16 UTC #42 lore raw

Quoted 2 lines · Robert Richter

But what options there are to detach the event from all processes and make it persistent?

Something like this:

	ioctl(fd, PERF_EVENT_IOC_DETACH, 0);

I guess this could simply set the persistent flag so that the rest of the perf code knows not to destroy event buffers etc.

I don't have an idea about the reattaching though because you don't have a file descriptor there.

Maybe for that we could really use the sys_perf_event_open() with flags set to PERF_FLAG_PERSISTENT to note that we want to reattach to the persistent event instead of opening a new one.

Something to that effect...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Robert Richter

author

2013-06-26 08:12 UTC #43 lore raw

Quoted 11 lines · Borislav Petkov

Quoted 2 lines · Robert Richter
But what options there are to detach the event from all processes and make it persistent?
Something like this:
	ioctl(fd, PERF_EVENT_IOC_DETACH, 0);
I guess this could simply set the persistent flag so that the rest of the perf code knows not to destroy event buffers etc.

Quoted 6 lines

I don't have an idea about the reattaching though because you don't have a file descriptor there.
Maybe for that we could really use the sys_perf_event_open() with flags set to PERF_FLAG_PERSISTENT to note that we want to reattach to the persistent event instead of opening a new one.

We get a new fd by opening the persistent event with the syscall. There would be 2 new ioctls:

ioctl(fd, PERF_EVENT_IOC_DETACH, 0);
ioctl(fd, PERF_EVENT_IOC_ATTACH, 0);

This would be fine and reuses existing infrastructure.

-Robert

Borislav Petkov

maintainer

2013-06-26 08:24 UTC #44 lore raw

Quoted 7 lines · Robert Richter

We get a new fd by opening the persistent event with the syscall. There would be 2 new ioctls:
ioctl(fd, PERF_EVENT_IOC_DETACH, 0);
ioctl(fd, PERF_EVENT_IOC_ATTACH, 0);
This would be fine and reuses existing infrastructure.

Well, how are you going to say that you want to open an already existing persistent event or your want to create exactly the same persistent event? Are we even going to allow identical persistent events to coexist?

The answers to those questions gives us the implementation.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Ingo Molnar

2013-06-26 09:46 UTC #45 lore raw

Quoted 7 lines · Borislav Petkov

Quoted 6 lines · Robert Richter
We get a new fd by opening the persistent event with the syscall. There would be 2 new ioctls:
ioctl(fd, PERF_EVENT_IOC_DETACH, 0);
ioctl(fd, PERF_EVENT_IOC_ATTACH, 0);

Quoted 6 lines

Quoted 1 line
This would be fine and reuses existing infrastructure.
Well, how are you going to say that you want to open an already existing persistent event or your want to create exactly the same persistent event? Are we even going to allow identical persistent events to coexist?

If already existing persistent events show up somewhere in sysfs (or in a separate pseudofilesystem) then an open() of them [given sufficient privileges of the caller, etc.] could attach to them.

Thanks,

Ingo

Borislav Petkov

maintainer

2013-06-26 09:56 UTC #46 lore raw

Quoted 3 lines · Ingo Molnar

If already existing persistent events show up somewhere in sysfs (or in a separate pseudofilesystem) then an open() of them [given sufficient privileges of the caller, etc.] could attach to them.

Yep, I think we want to say sys_perf_event_open(attr.persistent=1) which would give you a file descriptor to an already existing persistent event with attr.config=<id>.

If it doesn't exist, it creates it.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Robert Richter

author

2013-06-26 10:11 UTC #47 lore raw

In reply to Borislav Petkov

Quoted 7 lines · Borislav Petkov

Quoted 6 lines · Robert Richter
We get a new fd by opening the persistent event with the syscall. There would be 2 new ioctls:
ioctl(fd, PERF_EVENT_IOC_DETACH, 0);
ioctl(fd, PERF_EVENT_IOC_ATTACH, 0);

Quoted 6 lines

Quoted 1 line
This would be fine and reuses existing infrastructure.
Well, how are you going to say that you want to open an already existing persistent event or your want to create exactly the same persistent event? Are we even going to allow identical persistent events to coexist?

Here is the scenario:

Creating a persistent event from userspace:

* A process opens a system-wide event with the syscall and gets a fd.
* The process mmaps the buffer.
* The process does an ioctl to detach the process which increases the events and buffers refcount. The event is listed as 'persistent' in sysfs with a unique id.
* The process closes the fd. Event and buffer remain in the system since the refcounts are not zero.

Opening a persistent event:

* A process scans sysfs for persistent events.
* To open the event it sets up the event attr according to sysfs.
* The persistent event is opened with the syscall, the process gets a new fd of the event.
* The process attaches to the event buffer with mmap.

Releasing a persistent event:

* A process opens a persistent event and gets a fd.
* The process does an ioctl to attach the process which decreases the refcounts. The sysfs entry is removed.
* The process closes the fd.
* After all processes that are tied to the event closed their event's fds, the persistent event and its buffer is released.

Sounds like a plan?

-Robert

Ingo Molnar

2013-06-26 11:45 UTC #48 lore raw

Quoted 8 lines · Robert Richter

Quoted 7 lines · Borislav Petkov
Quoted 6 lines · Robert Richter
We get a new fd by opening the persistent event with the syscall. There would be 2 new ioctls:
ioctl(fd, PERF_EVENT_IOC_DETACH, 0);
ioctl(fd, PERF_EVENT_IOC_ATTACH, 0);

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
This would be fine and reuses existing infrastructure.
Well, how are you going to say that you want to open an already existing persistent event or your want to create exactly the same persistent event? Are we even going to allow identical persistent events to coexist?
Here is the scenario:

Looks mostly good - with a few suggestions:

Quoted 4 lines

Creating a persistent event from userspace:
* A process opens a system-wide event with the syscall and gets a fd.

Should this really be limited to system-wide events?

Quoted 11 lines

* The process mmaps the buffer.
* The process does an ioctl to detach the process which increases the events and buffers refcount. The event is listed as 'persistent' in sysfs with a unique id.
* The process closes the fd. Event and buffer remain in the system since the refcounts are not zero.
Opening a persistent event:
* A process scans sysfs for persistent events.
* To open the event it sets up the event attr according to sysfs.

Basically it would just put some ID (found in sysfs) into the attr and set attr.persistent=1 - not any other information, right?

If it knows the ID straight away (the user told it, or it remembers it from some other file such as a temporary file, etc.) then it does not even have to scan sysfs.

[ How about to additional logic: attr.persistent=1 && attr.config==0 means
a new persistent event is created straight away - no ioctl is needed to detach it explicitly. ]

Quoted 3 lines

* The persistent event is opened with the syscall, the process gets a new fd of the event.
* The process attaches to the event buffer with mmap.

Yes. And gets the pre-existing event and mmap buffer.

Quoted 10 lines

Releasing a persistent event:
* A process opens a persistent event and gets a fd.
* The process does an ioctl to attach the process which decreases the refcounts. The sysfs entry is removed.
* The process closes the fd.
* After all processes that are tied to the event closed their event's fds, the persistent event and its buffer is released.
Sounds like a plan?

It does :-)

I'm sure there will be some details going down that path, but it looks workable at first glance.

Note, for tracing the PERF_FLAG_FD_OUTPUT method of multiplexing multiple events onto a single mmap buffers is probably useful (also usable via the PERF_EVENT_IOC_SET_OUTPUT ioctl()), so please make sure the scheme works naturally with that model as well, not just with 1:1 event+buffer mappings.

See the uses of PERF_EVENT_IOC_SET_OUTPUT in tools/perf/.

Thanks,

Ingo

Ingo Molnar

2013-06-26 12:25 UTC #49 lore raw

Quoted 7 lines · Ingo Molnar

Note, for tracing the PERF_FLAG_FD_OUTPUT method of multiplexing multiple events onto a single mmap buffers is probably useful (also usable via the PERF_EVENT_IOC_SET_OUTPUT ioctl()), so please make sure the scheme works naturally with that model as well, not just with 1:1 event+buffer mappings.
See the uses of PERF_EVENT_IOC_SET_OUTPUT in tools/perf/.

Note that another facility that would be very useful for tracing is PeterZ's and tglx's patch that enables multiple tracepoints to be attached to a single event.

See the 2+ years old (bitrotten and unfinished) WIP patch below.

It adds a PERF_EVENT_IOC_ADD_TP ioctl() that adds a new tracepoint to an existing event. This makes perf based tracing scale up to an arbitrary number of tracepoints in essence.

Thanks,

Ingo

------------------>
Subject: perf-tracepoint-idr.patch
From: Thomas Gleixner <[email protected]>
Date: Wed, 24 Nov 2010 12:09:26 +0100

Signed-off-by Thomas Gleixner <[email protected]>

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

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

---
 include/linux/ftrace_event.h    |   10 
 include/linux/perf_event.h      |    9 
 include/linux/sched.h           |    9 
 include/trace/ftrace.h          |    4 
 kernel/events/core.c            |  407 ++++++++++++++++++++++++++++++++++++++--
 kernel/trace/trace_event_perf.c |   95 +++------
 kernel/trace/trace_kprobe.c     |   10 
 kernel/trace/trace_output.c     |  116 +++--------
 kernel/trace/trace_syscalls.c   |    8 
 9 files changed, 498 insertions(+), 170 deletions(-)

include/linux/ftrace_event.h +4 -6

Index: linux/include/linux/ftrace_event.h
===================================================================
@@ -87,8 +87,6 @@ struct trace_event_functions {
 };
 
 struct trace_event {
-	struct hlist_node		node;
-	struct list_head		list;
 	int				type;
 	struct trace_event_functions	*funcs;
 };

@@ -194,7 +192,6 @@ struct ftrace_event_call {
 
 #ifdef CONFIG_PERF_EVENTS
 	int				perf_refcount;
-	struct hlist_head __percpu	*perf_events;
 #endif
 };

@@ -263,8 +260,9 @@ struct perf_event;
 
 DECLARE_PER_CPU(struct pt_regs, perf_trace_regs);
 
-extern int  perf_trace_init(struct perf_event *event);
+extern int  perf_trace_init(struct perf_event *event, int event_id);
 extern void perf_trace_destroy(struct perf_event *event);
+extern void perf_trace_destroy_id(int id);
 extern int  perf_trace_add(struct perf_event *event, int flags);
 extern void perf_trace_del(struct perf_event *event, int flags);
 extern int  ftrace_profile_set_filter(struct perf_event *event, int event_id,

@@ -275,9 +273,9 @@ extern void *perf_trace_buf_prepare(int 
 
 static inline void
 perf_trace_buf_submit(void *raw_data, int size, int rctx, u64 addr,
-		       u64 count, struct pt_regs *regs, void *head)
+		       u64 count, struct pt_regs *regs, int id)
 {
-	perf_tp_event(addr, count, raw_data, size, regs, head, rctx);
+	perf_tp_event(addr, count, raw_data, size, regs, rctx, id);
 }
 #endif

include/linux/perf_event.h +8 -1

Index: linux/include/linux/perf_event.h
===================================================================
@@ -247,6 +247,7 @@ struct perf_event_attr {
 #define PERF_EVENT_IOC_PERIOD		_IOW('$', 4, __u64)
 #define PERF_EVENT_IOC_SET_OUTPUT	_IO ('$', 5)
 #define PERF_EVENT_IOC_SET_FILTER	_IOW('$', 6, char *)
+#define PERF_EVENT_IOC_ADD_TP		_IO ('$', 7)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,

@@ -568,6 +569,11 @@ struct hw_perf_event {
 			struct task_struct		*bp_target;
 		};
 #endif
+		/*
+		 * Same fudge as for breakpoints, trace-events needs
+		 * it too,.. convert the bp crap over..
+		 */
+		struct task_struct *event_target;
 	};
 	int				state;
 	local64_t			prev_count;

@@ -859,6 +865,7 @@ struct perf_event {
 #ifdef CONFIG_EVENT_TRACING
 	struct ftrace_event_call	*tp_event;
 	struct event_filter		*filter;
+	struct perf_tp_idr		tp_idr;
 #endif
 
 #ifdef CONFIG_CGROUP_PERF

@@ -1133,7 +1140,7 @@ static inline bool perf_paranoid_kernel(
 extern void perf_event_init(void);
 extern void perf_tp_event(u64 addr, u64 count, void *record,
 			  int entry_size, struct pt_regs *regs,
-			  struct hlist_head *head, int rctx);
+			  int rctx, int id);
 extern void perf_bp_event(struct perf_event *event, void *data);
 
 #ifndef perf_misc_flags

include/linux/sched.h +9 -0

Index: linux/include/linux/sched.h
===================================================================
@@ -82,6 +82,7 @@ struct sched_param {
 #include <linux/rculist.h>
 #include <linux/rtmutex.h>
 
+#include <linux/idr.h>
 #include <linux/time.h>
 #include <linux/param.h>
 #include <linux/resource.h>

@@ -1199,6 +1200,11 @@ enum perf_event_task_context {
 	perf_nr_task_contexts,
 };
 
+struct perf_tp_idr {
+	struct mutex	lock;
+	struct idr	idr;
+};
+
 struct task_struct {
 	volatile long state;	/* -1 unrunnable, 0 runnable, >0 stopped */
 	void *stack;

@@ -1485,6 +1491,9 @@ struct task_struct {
 	struct perf_event_context *perf_event_ctxp[perf_nr_task_contexts];
 	struct mutex perf_event_mutex;
 	struct list_head perf_event_list;
+#ifdef CONFIG_EVENT_TRACING
+	struct perf_tp_idr *perf_tp_idr;
+#endif
 #endif
 #ifdef CONFIG_NUMA
 	struct mempolicy *mempolicy;	/* Protected by alloc_lock */

include/trace/ftrace.h +1 -3

Index: linux/include/trace/ftrace.h
===================================================================
@@ -708,7 +708,6 @@ perf_trace_##call(void *__data, proto)		
 	struct ftrace_raw_##call *entry;				\
 	struct pt_regs __regs;						\
 	u64 __addr = 0, __count = 1;					\
-	struct hlist_head *head;					\
 	int __entry_size;						\
 	int __data_size;						\
 	int rctx;							\

@@ -733,9 +732,8 @@ perf_trace_##call(void *__data, proto)		
 									\
 	{ assign; }							\
 									\
-	head = this_cpu_ptr(event_call->perf_events);			\
 	perf_trace_buf_submit(entry, __entry_size, rctx, __addr,	\
-		__count, &__regs, head);				\
+		__count, &__regs, event_call->event.type);		\
 }
 
 /*

kernel/events/core.c +391 ~~-16~~

576 lines not on this page · raw message

kernel/trace/trace_event_perf.c +40 ~~-55~~

148 lines not on this page · raw message

kernel/trace/trace_kprobe.c +4 -6

Index: linux/kernel/trace/trace_kprobe.c
===================================================================
@@ -1659,7 +1659,6 @@ static __kprobes void kprobe_perf_func(s
 	struct trace_probe *tp = container_of(kp, struct trace_probe, rp.kp);
 	struct ftrace_event_call *call = &tp->call;
 	struct kprobe_trace_entry_head *entry;
-	struct hlist_head *head;
 	int size, __size, dsize;
 	int rctx;

@@ -1679,8 +1678,8 @@ static __kprobes void kprobe_perf_func(s
 	memset(&entry[1], 0, dsize);
 	store_trace_args(sizeof(*entry), tp, regs, (u8 *)&entry[1], dsize);
 
-	head = this_cpu_ptr(call->perf_events);
-	perf_trace_buf_submit(entry, size, rctx, entry->ip, 1, regs, head);
+	perf_trace_buf_submit(entry, size, rctx, entry->ip, 1, regs,
+			      call->event.type);
 }
 
 /* Kretprobe profile handler */

@@ -1690,7 +1689,6 @@ static __kprobes void kretprobe_perf_fun
 	struct trace_probe *tp = container_of(ri->rp, struct trace_probe, rp);
 	struct ftrace_event_call *call = &tp->call;
 	struct kretprobe_trace_entry_head *entry;
-	struct hlist_head *head;
 	int size, __size, dsize;
 	int rctx;

@@ -1710,8 +1708,8 @@ static __kprobes void kretprobe_perf_fun
 	entry->ret_ip = (unsigned long)ri->ret_addr;
 	store_trace_args(sizeof(*entry), tp, regs, (u8 *)&entry[1], dsize);
 
-	head = this_cpu_ptr(call->perf_events);
-	perf_trace_buf_submit(entry, size, rctx, entry->ret_ip, 1, regs, head);
+	perf_trace_buf_submit(entry, size, rctx, entry->ret_ip, 1,
+			      regs, call->event.type);
 }
 
 static int probe_perf_enable(struct ftrace_event_call *call)

kernel/trace/trace_output.c +39 ~~-77~~

173 lines not on this page · raw message

kernel/trace/trace_syscalls.c +2 -6

Index: linux/kernel/trace/trace_syscalls.c
===================================================================
@@ -499,7 +499,6 @@ static void perf_syscall_enter(void *ign
 {
 	struct syscall_metadata *sys_data;
 	struct syscall_trace_enter *rec;
-	struct hlist_head *head;
 	int syscall_nr;
 	int rctx;
 	int size;

@@ -530,8 +529,7 @@ static void perf_syscall_enter(void *ign
 	syscall_get_arguments(current, regs, 0, sys_data->nb_args,
 			       (unsigned long *)&rec->args);
 
-	head = this_cpu_ptr(sys_data->enter_event->perf_events);
-	perf_trace_buf_submit(rec, size, rctx, 0, 1, regs, head);
+	perf_trace_buf_submit(rec, size, rctx, 0, 1, regs, rec->ent.type);
 }
 
 int perf_sysenter_enable(struct ftrace_event_call *call)

@@ -573,7 +571,6 @@ static void perf_syscall_exit(void *igno
 {
 	struct syscall_metadata *sys_data;
 	struct syscall_trace_exit *rec;
-	struct hlist_head *head;
 	int syscall_nr;
 	int rctx;
 	int size;

@@ -606,8 +603,7 @@ static void perf_syscall_exit(void *igno
 	rec->nr = syscall_nr;
 	rec->ret = syscall_get_return_value(current, regs);
 
-	head = this_cpu_ptr(sys_data->exit_event->perf_events);
-	perf_trace_buf_submit(rec, size, rctx, 0, 1, regs, head);
+	perf_trace_buf_submit(rec, size, rctx, 0, 1, regs, rec->ent.type);
 }
 
 int perf_sysexit_enable(struct ftrace_event_call *call)

Robert Richter

author

2013-06-26 12:44 UTC #50 lore raw

In reply to Ingo Molnar

Quoted 6 lines · Ingo Molnar

Quoted 3 lines · Robert Richter
Creating a persistent event from userspace:
* A process opens a system-wide event with the syscall and gets a fd.
Should this really be limited to system-wide events?

It must not necessarily be restricted to system-wide events. Limiting it is just to make it easier in the beginning, we don't need to think about what happens if a process dies and permissions in case of per-task events, etc (didn't thought about it yet ;).

Also, a persistent event is currently per-system, meaning there is one entry only for the same kind of event scheduled on all cpus. This keeps event handling easy (e.g. no need to export the cpu in the event's sysfs entry, just the flag and id) but also has some drawbacks (handling of multiple events per entry). Probably it's better to have a 1:1 mapping.

Quoted 12 lines

Quoted 11 lines
* The process mmaps the buffer.
* The process does an ioctl to detach the process which increases the events and buffers refcount. The event is listed as 'persistent' in sysfs with a unique id.
* The process closes the fd. Event and buffer remain in the system since the refcounts are not zero.
Opening a persistent event:
* A process scans sysfs for persistent events.
* To open the event it sets up the event attr according to sysfs.

Quoted 6 lines

Basically it would just put some ID (found in sysfs) into the attr and set attr.persistent=1 - not any other information, right?
If it knows the ID straight away (the user told it, or it remembers it from some other file such as a temporary file, etc.) then it does not even have to scan sysfs.

Yes, there is a unique id which we could also return with the ioctl or so. sysfs is esp. to let perf tools and the event parser know about how to setup the events. It might be also useful if the syscall setup changes in the future for these kind of events, then we just modify the sysfs entry.

Quoted 3 lines

[ How about to additional logic: attr.persistent=1 && attr.config==0 means
a new persistent event is created straight away - no ioctl is needed to detach it explicitly. ]

That's correct. We could also do the following:

To connect to an existing event:

attr.type=<persistent-pmu> && attr.config==<event-id>

(This might be harder to implement except the persistent event pmu type will be fix, PERF_TYPE_PERSISTENT=6.)

To create a new persistent event:

attr.persistent=1 && attr=<some event setup: pmu, config, flags, etc>

Quoted 5 lines

Quoted 3 lines
* The persistent event is opened with the syscall, the process gets a new fd of the event.
* The process attaches to the event buffer with mmap.
Yes. And gets the pre-existing event and mmap buffer.

That's what I mean.

A problem here is that mmap'ed buffer size (number of pages) must be be equal to the pre-existing buffer size and thus to be known somehow.

Quoted 9 lines

Quoted 9 lines
Releasing a persistent event:
* A process opens a persistent event and gets a fd.
* The process does an ioctl to attach the process which decreases the refcounts. The sysfs entry is removed.
* The process closes the fd.
* After all processes that are tied to the event closed their event's fds, the persistent event and its buffer is released.

Quoted 6 lines

Quoted 1 line
Sounds like a plan?
It does :-)
I'm sure there will be some details going down that path, but it looks workable at first glance.

Yes, there will be some 'implementation details', but it should work.

Quoted 7 lines

Note, for tracing the PERF_FLAG_FD_OUTPUT method of multiplexing multiple events onto a single mmap buffers is probably useful (also usable via the PERF_EVENT_IOC_SET_OUTPUT ioctl()), so please make sure the scheme works naturally with that model as well, not just with 1:1 event+buffer mappings.
See the uses of PERF_EVENT_IOC_SET_OUTPUT in tools/perf/.

Yes, thanks for this hint. I wasn't aware of this feature yet.

Thanks for your comments. Will start reworking the patches into this direction.

-Robert

Namhyung Kim

maintainer

2013-06-27 05:46 UTC #51 lore raw

Hi Robert,

Quoted 9 lines · Robert Richter

Quoted 3 lines · Ingo Molnar
[ How about to additional logic: attr.persistent=1 && attr.config==0 means
a new persistent event is created straight away - no ioctl is needed to detach it explicitly. ]
That's correct. We could also do the following:
To connect to an existing event:

Quoted 8 lines

attr.type=<persistent-pmu> && attr.config==<event-id>
(This might be harder to implement except the persistent event pmu type will be fix, PERF_TYPE_PERSISTENT=6.)
To create a new persistent event:
attr.persistent=1 && attr=<some event setup: pmu, config, flags, etc>

How about using 2 bits for perfsistent flag, 1 for connecting to an existing one, 2 for creating new one.

Quoted 10 lines

Quoted 5 lines
Quoted 3 lines
* The persistent event is opened with the syscall, the process gets a new fd of the event.
* The process attaches to the event buffer with mmap.
Yes. And gets the pre-existing event and mmap buffer.
That's what I mean.
A problem here is that mmap'ed buffer size (number of pages) must be be equal to the pre-existing buffer size and thus to be known somehow.

What about also exporting the buffer size via sysfs pmu directory?

Thanks,
Namhyung

Borislav Petkov

maintainer

2013-06-27 08:35 UTC #52 lore raw

Quoted 2 lines · Namhyung Kim

How about using 2 bits for perfsistent flag, 1 for connecting to an existing one, 2 for creating new one.

No need since persistent events don't need to be duplicated. Think of a
tracepoint: the samples you get there are the same, no matter how many times you enable it.

So, if you open a persistent event which doesn't exist, it gets created. If you open an existing one, you get read-only access to its buffers. No need for two bits. Actually, with the detach/attach ioctl we don't even need a single flag but having it makes the implementation a lot simpler.

Quoted 4 lines

Quoted 2 lines
A problem here is that mmap'ed buffer size (number of pages) must be be equal to the pre-existing buffer size and thus to be known somehow.
What about also exporting the buffer size via sysfs pmu directory?

Yes, we've been discussing buffer size. The simplest thing is to hardcode the buffer size so that it is implicitly known to all agents. However, I don't know whether there is a use case where different buffer sizes actually make sense. I'd tend to do the simplest thing initially.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Ingo Molnar

2013-06-27 08:51 UTC #53 lore raw

Quoted 10 lines · Borislav Petkov

Quoted 5 lines
Quoted 3 lines
A problem here is that mmap'ed buffer size (number of pages) must be be equal to the pre-existing buffer size and thus to be known somehow.
What about also exporting the buffer size via sysfs pmu directory?
Yes, we've been discussing buffer size. The simplest thing is to hardcode the buffer size so that it is implicitly known to all agents. However, I don't know whether there is a use case where different buffer sizes actually make sense. I'd tend to do the simplest thing initially.

Btw., in terms of testing and design focus I'd suggest concentrating not on rare and relatively singular events like RAS MCE events, but on a more 'everyday tracing' flow of events:

- large, per cpu trace buffers

- all events output into a single trace buffer to get a coherent trace

- [ possibly revive the tracepoint-multiplexing patch from peterz/tglx, to be able to get a rich selection of tracepoints. ]

That is I think the workflow most people will be interested in:

- they have a workload to analyze and they want to do some 'tracing' to understand it better or to pinpoint a problem.

- based on the problem they want to trace a selection of tracepoints, as easily and quickly as possible.

- we could possibly provide a few 'groups' of tracepoints for typical uses - for example scheduler tracing, or MM tracing, or IO tracing, etc.), so people wouldn't have to specify a dozen tracepoints but could symbolically refer to any of these pre-cooked groups. [this is probably a tooling detail.]

- they want to persistently trace into a generously sized trace buffer, without any tool running while the collection period lasts.

- to refine the result they'd like to stop/start tracing, reset/clear the tracebuffer for failed attempts, and generally see how large the tracebuffer is.

- and extract/analyze traces - both in a detailed form and in summary fashion.

- they possibly want to save it to a perf.data and have equivalent facilities of analysis.

If that workflow works well then the RAS angle will work well too.

Thanks,

Ingo