[PATCH lttng-ust v2 1/2] Keep perf context FD open for other architectures

[PATCH lttng-ust v2 1/2] Keep perf context FD open for other architectures

[Julien Desfossez]

Instead of closing the perf context after the page has been mapped, keep
it open so it can be used with the read() system call if the
architecture does not support direct access from user-space.

Signed-off-by: Julien Desfossez <jdesfossez@efficios.com>
---
 liblttng-ust/lttng-context-perf-counters.c | 53 +++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/liblttng-ust/lttng-context-perf-counters.c b/liblttng-ust/lttng-context-perf-counters.c
index 97ddf97..725b1b4 100644
--- a/liblttng-ust/lttng-context-perf-counters.c
+++ b/liblttng-ust/lttng-context-perf-counters.c
@@ -55,6 +55,7 @@ struct lttng_perf_counter_thread_field {
 	struct perf_event_mmap_page *pc;
 	struct cds_list_head thread_field_node;	/* Per-field list of thread fields (node) */
 	struct cds_list_head rcu_field_node;	/* RCU per-thread list of fields (node) */
+	int fd;					/* Perf FD */
 };
 
 struct lttng_perf_counter_thread {
@@ -131,24 +132,50 @@ int sys_perf_event_open(struct perf_event_attr *attr,
 }
 
 static
-struct perf_event_mmap_page *setup_perf(struct perf_event_attr *attr)
+int open_perf_fd(struct perf_event_attr *attr)
 {
-	void *perf_addr;
-	int fd, ret;
+	int fd;
 
 	fd = sys_perf_event_open(attr, 0, -1, -1, 0);
 	if (fd < 0)
-		return NULL;
+		return -1;
+
+	return fd;
+}
+
+static
+struct perf_event_mmap_page *setup_perf(
+		struct lttng_perf_counter_thread_field *thread_field)
+{
+	void *perf_addr;
 
 	perf_addr = mmap(NULL, sizeof(struct perf_event_mmap_page),
-			PROT_READ, MAP_SHARED, fd, 0);
+			PROT_READ, MAP_SHARED, thread_field->fd, 0);
 	if (perf_addr == MAP_FAILED)
-		return NULL;
+		perf_addr = NULL;
+
+	/* No need to keep the FD open on x86 */
+#if defined(__x86_64__) || defined(__i386__)
+	close_perf_fd(thread_field->fd);
+	thread_field->fd = -1;
+#endif
+
+end:
+	return perf_addr;
+}
+
+static
+void close_perf_fd(int fd)
+{
+	int ret;
+
+	if (fd < 0)
+		return;
+
 	ret = close(fd);
 	if (ret) {
 		perror("Error closing LTTng-UST perf memory mapping FD");
 	}
-	return perf_addr;
 }
 
 static
@@ -221,7 +248,9 @@ struct lttng_perf_counter_thread_field *
 	if (!thread_field)
 		abort();
 	thread_field->field = perf_field;
-	thread_field->pc = setup_perf(&perf_field->attr);
+	thread_field->fd = open_perf_fd(&perf_field->attr);
+	if (thread_field->fd >= 0)
+		thread_field->pc = setup_perf(thread_field);
 	/* Note: thread_field->pc can be NULL if setup_perf() fails. */
 	ust_lock_nocheck();
 	cds_list_add_rcu(&thread_field->rcu_field_node,
@@ -293,6 +322,7 @@ static
 void lttng_destroy_perf_thread_field(
 		struct lttng_perf_counter_thread_field *thread_field)
 {
+	close_perf_fd(thread_field->fd);
 	unmap_perf_page(thread_field->pc);
 	cds_list_del_rcu(&thread_field->rcu_field_node);
 	cds_list_del(&thread_field->thread_field_node);
@@ -341,7 +371,6 @@ int lttng_add_perf_counter_to_ctx(uint32_t type,
 {
 	struct lttng_ctx_field *field;
 	struct lttng_perf_counter_field *perf_field;
-	struct perf_event_mmap_page *tmp_pc;
 	char *name_alloc;
 	int ret;
 
@@ -389,12 +418,12 @@ int lttng_add_perf_counter_to_ctx(uint32_t type,
 	field->u.perf_counter = perf_field;
 
 	/* Ensure that this perf counter can be used in this process. */
-	tmp_pc = setup_perf(&perf_field->attr);
-	if (!tmp_pc) {
+	ret = open_perf_fd(&perf_field->attr);
+	if (ret < 0) {
 		ret = -ENODEV;
 		goto setup_error;
 	}
-	unmap_perf_page(tmp_pc);
+	close_perf_fd(ret);
 
 	/*
 	 * Contexts can only be added before tracing is started, so we
-- 
1.9.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-ust v2 1/2] Keep perf context FD open for other architectures

[Mathieu Desnoyers]

----- On Jun 23, 2016, at 6:52 PM, Julien Desfossez jdesfossez@efficios.com wrote:

> Instead of closing the perf context after the page has been mapped, keep
> it open so it can be used with the read() system call if the
> architecture does not support direct access from user-space.
> 
> Signed-off-by: Julien Desfossez <jdesfossez@efficios.com>
> ---
> liblttng-ust/lttng-context-perf-counters.c | 53 +++++++++++++++++++++++-------
> 1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/liblttng-ust/lttng-context-perf-counters.c
> b/liblttng-ust/lttng-context-perf-counters.c
> index 97ddf97..725b1b4 100644
> --- a/liblttng-ust/lttng-context-perf-counters.c
> +++ b/liblttng-ust/lttng-context-perf-counters.c
> @@ -55,6 +55,7 @@ struct lttng_perf_counter_thread_field {
> 	struct perf_event_mmap_page *pc;
> 	struct cds_list_head thread_field_node;	/* Per-field list of thread fields
> 	(node) */
> 	struct cds_list_head rcu_field_node;	/* RCU per-thread list of fields (node) */
> +	int fd;					/* Perf FD */
> };
> 
> struct lttng_perf_counter_thread {
> @@ -131,24 +132,50 @@ int sys_perf_event_open(struct perf_event_attr *attr,
> }
> 
> static
> -struct perf_event_mmap_page *setup_perf(struct perf_event_attr *attr)
> +int open_perf_fd(struct perf_event_attr *attr)
> {
> -	void *perf_addr;
> -	int fd, ret;
> +	int fd;
> 
> 	fd = sys_perf_event_open(attr, 0, -1, -1, 0);
> 	if (fd < 0)
> -		return NULL;
> +		return -1;
> +
> +	return fd;
> +}
> +
> +static
> +struct perf_event_mmap_page *setup_perf(
> +		struct lttng_perf_counter_thread_field *thread_field)
> +{
> +	void *perf_addr;
> 
> 	perf_addr = mmap(NULL, sizeof(struct perf_event_mmap_page),
> -			PROT_READ, MAP_SHARED, fd, 0);
> +			PROT_READ, MAP_SHARED, thread_field->fd, 0);
> 	if (perf_addr == MAP_FAILED)
> -		return NULL;
> +		perf_addr = NULL;
> +
> +	/* No need to keep the FD open on x86 */
> +#if defined(__x86_64__) || defined(__i386__)
> +	close_perf_fd(thread_field->fd);
> +	thread_field->fd = -1;
> +#endif

No #if/#endif whatsoever in the code please. We don't want lttng
to end up looking like glibc. ;)

If needed, move that at the beginning of the file with e.g.

#if defined(__x86_64__) || defined(__i386__)
static bool arch_perf_use_read(void)
{
  return true;
}
#else
static bool arch_perf_use_read(void)
{
  return false;
}
#endif

> +
> +end:
> +	return perf_addr;
> +}
> +
> +static
> +void close_perf_fd(int fd)
> +{
> +	int ret;
> +
> +	if (fd < 0)
> +		return;
> +
> 	ret = close(fd);
> 	if (ret) {

You can remove those extra { }.

> 		perror("Error closing LTTng-UST perf memory mapping FD");
> 	}
> -	return perf_addr;
> }
> 
> static
> @@ -221,7 +248,9 @@ struct lttng_perf_counter_thread_field *
> 	if (!thread_field)
> 		abort();
> 	thread_field->field = perf_field;
> -	thread_field->pc = setup_perf(&perf_field->attr);
> +	thread_field->fd = open_perf_fd(&perf_field->attr);
> +	if (thread_field->fd >= 0)
> +		thread_field->pc = setup_perf(thread_field);
> 	/* Note: thread_field->pc can be NULL if setup_perf() fails. */

Do we need to update this comment to state that thread_field->fd can
also be -1 if open_perf_fd fails ?

Thanks,

Mathieu

> 	ust_lock_nocheck();
> 	cds_list_add_rcu(&thread_field->rcu_field_node,
> @@ -293,6 +322,7 @@ static
> void lttng_destroy_perf_thread_field(
> 		struct lttng_perf_counter_thread_field *thread_field)
> {
> +	close_perf_fd(thread_field->fd);
> 	unmap_perf_page(thread_field->pc);
> 	cds_list_del_rcu(&thread_field->rcu_field_node);
> 	cds_list_del(&thread_field->thread_field_node);
> @@ -341,7 +371,6 @@ int lttng_add_perf_counter_to_ctx(uint32_t type,
> {
> 	struct lttng_ctx_field *field;
> 	struct lttng_perf_counter_field *perf_field;
> -	struct perf_event_mmap_page *tmp_pc;
> 	char *name_alloc;
> 	int ret;
> 
> @@ -389,12 +418,12 @@ int lttng_add_perf_counter_to_ctx(uint32_t type,
> 	field->u.perf_counter = perf_field;
> 
> 	/* Ensure that this perf counter can be used in this process. */
> -	tmp_pc = setup_perf(&perf_field->attr);
> -	if (!tmp_pc) {
> +	ret = open_perf_fd(&perf_field->attr);
> +	if (ret < 0) {
> 		ret = -ENODEV;
> 		goto setup_error;
> 	}
> -	unmap_perf_page(tmp_pc);
> +	close_perf_fd(ret);
> 
> 	/*
> 	 * Contexts can only be added before tracing is started, so we
> --
> 1.9.1
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev