Re: [PATCHv2] perf tools: enable close-on-exec flag on perf file descriptor

[Arnaldo Carvalho de Melo <acme@ghostprotocols.net>]

Em Mon, Jan 13, 2014 at 11:09:30AM +0100, Yann Droneaud escreveu:
> +++ b/tools/perf/util/cloexec.c
> @@ -0,0 +1,54 @@
> +#include "util.h"
> +#include "../perf.h"
> +#include "cloexec.h"
> +
> +static unsigned long flag = PERF_FLAG_FD_CLOEXEC;
> +
> +static int perf_flag_probe(void)
> +{
> +	struct perf_event_attr attr;
> +	int fd;
> +	int err;
> +
> +	/* check cloexec flag */
> +	memset(&attr, 0, sizeof(attr));

If you do this you'll use attr.type = PERF_TYPE_HARDWARE and
attr.config then means PERF_COUNT_HW_CPU_CYCLES, which will fail
where we don't have a PMU, see perf_evsel__fallback(), perhaps it is
better for you to go with the fallback that is most likely supported in
all arches:

                evsel->attr.type   = PERF_TYPE_SOFTWARE;       // 1
                evsel->attr.config = PERF_COUNT_SW_CPU_CLOCK;  // 0

I.e. just do a:

	struct perf_event_attr attr = { .type = 1, }; and ditch the
memset (or do a attr.type = PERF_COUNT_SW_CPU_CLOCK;, up to you).

Thanks for doing the __perf_evsel__open fallbacking!

- Arnaldo

> +	fd = sys_perf_event_open(&attr, 0, -1, -1,
> +				 PERF_FLAG_FD_CLOEXEC);
> +	if (fd >= 0) {
> +		close(fd);
> +		return 1;
> +	}
> +
> +	if (errno != EINVAL) {
> +		err = errno;
> +		pr_warning("sys_perf_event_open() syscall returned "
> +			   "%d (%d: %s)\n", fd, err, strerror(err));
> +	}
> +
> +	/* not supported, confirm error related to PERF_FLAG_FD_CLOEXEC */
> +	memset(&attr, 0, sizeof(attr));
> +	fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
> +	if (fd >= 0) {
> +		close(fd);
> +		return 0;
> +	}
> +
> +	err = errno;
> +	die("sys_perf_event_open() syscall returned "
> +	    "%d (%d: %s)\n", fd, err, strerror(err));
> +
> +	return -1;
> +}
> +
> +unsigned long perf_event_open_cloexec_flag(void)
> +{
> +	static bool probed;
> +
> +	if (!probed) {
> +		if (perf_flag_probe() <= 0)
> +			flag = 0;
> +		probed = true;
> +	}
> +
> +	return flag;
> +}
> diff --git a/tools/perf/util/cloexec.h b/tools/perf/util/cloexec.h
> new file mode 100644
> index 000000000000..94a5a7d829d5
> --- /dev/null
> +++ b/tools/perf/util/cloexec.h
> @@ -0,0 +1,6 @@
> +#ifndef __PERF_CLOEXEC_H
> +#define __PERF_CLOEXEC_H
> +
> +unsigned long perf_event_open_cloexec_flag(void);
> +
> +#endif /* __PERF_CLOEXEC_H */
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ade8d9c1c431..ff845c7e7a4f 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -29,6 +29,7 @@ static struct {
>  	bool sample_id_all;
>  	bool exclude_guest;
>  	bool mmap2;
> +	bool cloexec;
>  } perf_missing_features;
>  
>  #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
> @@ -968,7 +969,7 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>  			      struct thread_map *threads)
>  {
>  	int cpu, thread;
> -	unsigned long flags = 0;
> +	unsigned long flags = PERF_FLAG_FD_CLOEXEC;
>  	int pid = -1, err;
>  	enum { NO_CHANGE, SET_TO_MAX, INCREASED_MAX } set_rlimit = NO_CHANGE;
>  
> @@ -977,11 +978,13 @@ static int __perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>  		return -ENOMEM;
>  
>  	if (evsel->cgrp) {
> -		flags = PERF_FLAG_PID_CGROUP;
> +		flags |= PERF_FLAG_PID_CGROUP;
>  		pid = evsel->cgrp->fd;
>  	}
>  
>  fallback_missing_features:
> +	if (perf_missing_features.cloexec)
> +		flags &= ~(unsigned long)PERF_FLAG_FD_CLOEXEC;
>  	if (perf_missing_features.mmap2)
>  		evsel->attr.mmap2 = 0;
>  	if (perf_missing_features.exclude_guest)
> @@ -1050,7 +1053,10 @@ try_fallback:
>  	if (err != -EINVAL || cpu > 0 || thread > 0)
>  		goto out_close;
>  
> -	if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {
> +	if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
> +		perf_missing_features.cloexec = true;
> +		goto fallback_missing_features;
> +	} else if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {
>  		perf_missing_features.mmap2 = true;
>  		goto fallback_missing_features;
>  	} else if (!perf_missing_features.exclude_guest &&
> diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
> index 104a47563d39..6483ef5df31b 100644
> --- a/tools/perf/util/record.c
> +++ b/tools/perf/util/record.c
> @@ -4,6 +4,7 @@
>  #include "parse-events.h"
>  #include "fs.h"
>  #include "util.h"
> +#include "cloexec.h"
>  
>  typedef void (*setup_probe_fn_t)(struct perf_evsel *evsel);
>  
> @@ -11,6 +12,7 @@ static int perf_do_probe_api(setup_probe_fn_t fn, int cpu, const char *str)
>  {
>  	struct perf_evlist *evlist;
>  	struct perf_evsel *evsel;
> +	unsigned long flags = perf_event_open_cloexec_flag();
>  	int err = -EAGAIN, fd;
>  
>  	evlist = perf_evlist__new();
> @@ -22,14 +24,14 @@ static int perf_do_probe_api(setup_probe_fn_t fn, int cpu, const char *str)
>  
>  	evsel = perf_evlist__first(evlist);
>  
> -	fd = sys_perf_event_open(&evsel->attr, -1, cpu, -1, 0);
> +	fd = sys_perf_event_open(&evsel->attr, -1, cpu, -1, flags);
>  	if (fd < 0)
>  		goto out_delete;
>  	close(fd);
>  
>  	fn(evsel);
>  
> -	fd = sys_perf_event_open(&evsel->attr, -1, cpu, -1, 0);
> +	fd = sys_perf_event_open(&evsel->attr, -1, cpu, -1, flags);
>  	if (fd < 0) {
>  		if (errno == EINVAL)
>  			err = -EINVAL;
> @@ -203,7 +205,8 @@ bool perf_evlist__can_select_event(struct perf_evlist *evlist, const char *str)
>  		cpu = evlist->cpus->map[0];
>  	}
>  
> -	fd = sys_perf_event_open(&evsel->attr, -1, cpu, -1, 0);
> +	fd = sys_perf_event_open(&evsel->attr, -1, cpu, -1,
> +				 perf_event_open_cloexec_flag());
>  	if (fd >= 0) {
>  		close(fd);
>  		ret = true;
> -- 
> 1.8.4.2