Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 31 Aug 2021 16:41:23 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Riccardo Mancini <rickyman7@gmail.com>
Cc:     Ian Rogers <irogers@google.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Jiri Olsa <jolsa@redhat.com>, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org
Subject: Re: [RFC PATCH v1 20/37] perf evsel: separate rlimit increase from
 evsel__open_cpu
Message-ID: <YS6F46/ONZzCWFnT@kernel.org>
References: <cover.1629490974.git.rickyman7@gmail.com>
 <2f256de8ec37b9809a5cef73c2fa7bce416af5d3.1629490974.git.rickyman7@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2f256de8ec37b9809a5cef73c2fa7bce416af5d3.1629490974.git.rickyman7@gmail.com>
Precedence: bulk

Em Sat, Aug 21, 2021 at 11:19:26AM +0200, Riccardo Mancini escreveu:
> This is a preparatory patch for the following patches with the goal to
> separate from evlist__open_cpu the actual opening (which could be
> performed in parallel), from the existing fallback mechanisms, which
> should be handled sequentially.
> 
> This patch separates the rlimit increase from evsel__open_cpu.
> 
> Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> ---
>  tools/perf/util/evsel.c | 50 ++++++++++++++++++++++++-----------------
>  tools/perf/util/evsel.h |  3 +++
>  2 files changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index c393bd992322d925..916930ea31450265 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1931,13 +1931,40 @@ bool evsel__detect_missing_features(struct evsel *evsel)
>  	}
>  }
>  
> +bool evsel__increase_rlimit(enum rlimit_action *set_rlimit)
> +{
> +	int old_errno;
> +	struct rlimit l;
> +
> +	if (*set_rlimit < INCREASED_MAX) {
> +

The above blank line is not needed
> +		old_errno = errno;

And here it should be used, I'm recycling it for you :-)

> +		if (getrlimit(RLIMIT_NOFILE, &l) == 0) {
> +			if (*set_rlimit == NO_CHANGE)
> +				l.rlim_cur = l.rlim_max;
> +			else {

Also when the else clause has {}, the if should have it too, I'm fixing
it.

I see that you just moved things around, but then this is a good time to
do these cosmetic changes.

Applying.

- Arnaldo

> +				l.rlim_cur = l.rlim_max + 1000;
> +				l.rlim_max = l.rlim_cur;
> +			}
> +			if (setrlimit(RLIMIT_NOFILE, &l) == 0) {
> +				(*set_rlimit) += 1;
> +				errno = old_errno;
> +				return true;
> +			}
> +		}
> +		errno = old_errno;
> +	}
> +
> +	return false;
> +}
> +
>  static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
>  		struct perf_thread_map *threads,
>  		int start_cpu, int end_cpu)
>  {
>  	int cpu, thread, nthreads;
>  	int pid = -1, err, old_errno;
> -	enum { NO_CHANGE, SET_TO_MAX, INCREASED_MAX } set_rlimit = NO_CHANGE;
> +	enum rlimit_action set_rlimit = NO_CHANGE;
>  
>  	err = __evsel__prepare_open(evsel, cpus, threads);
>  	if (err)
> @@ -2046,25 +2073,8 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
>  	 * perf stat needs between 5 and 22 fds per CPU. When we run out
>  	 * of them try to increase the limits.
>  	 */
> -	if (err == -EMFILE && set_rlimit < INCREASED_MAX) {
> -		struct rlimit l;
> -
> -		old_errno = errno;
> -		if (getrlimit(RLIMIT_NOFILE, &l) == 0) {
> -			if (set_rlimit == NO_CHANGE)
> -				l.rlim_cur = l.rlim_max;
> -			else {
> -				l.rlim_cur = l.rlim_max + 1000;
> -				l.rlim_max = l.rlim_cur;
> -			}
> -			if (setrlimit(RLIMIT_NOFILE, &l) == 0) {
> -				set_rlimit++;
> -				errno = old_errno;
> -				goto retry_open;
> -			}
> -		}
> -		errno = old_errno;
> -	}
> +	if (err == -EMFILE && evsel__increase_rlimit(&set_rlimit))
> +		goto retry_open;
>  
>  	if (err != -EINVAL || cpu > 0 || thread > 0)
>  		goto out_close;
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index a83fb7f69b1ead73..bf9abd9a5cbf9852 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -290,6 +290,9 @@ int evsel__prepare_open(struct evsel *evsel, struct perf_cpu_map *cpus,
>  		struct perf_thread_map *threads);
>  bool evsel__detect_missing_features(struct evsel *evsel);
>  
> +enum rlimit_action { NO_CHANGE, SET_TO_MAX, INCREASED_MAX };
> +bool evsel__increase_rlimit(enum rlimit_action *set_rlimit);
> +
>  struct perf_sample;
>  
>  void *evsel__rawptr(struct evsel *evsel, struct perf_sample *sample, const char *name);
> -- 
> 2.31.1

-- 

- Arnaldo