[lttng-dev] [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 at 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 at 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 at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com