Re: [PATCH v2] perf test record.sh: Raise limit of open file descriptors

Re: [PATCH v2] perf test record.sh: Raise limit of open file descriptors

[Arnaldo Carvalho de Melo]

On Mon, Aug 12, 2024 at 10:16:10AM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, May 21, 2024 at 11:27:56AM +0200, Michael Petlan wrote:
> > On Mon, 29 Apr 2024, vmolnaro@redhat.com wrote:
> > > From: Veronika Molnarova <vmolnaro@redhat.com>
> > > 
> > > Subtest for system-wide record with '--threads=cpu' option fails due
> > > to a limit of open file descriptors on systems with 128 or more CPUs
> > > as the default limit is set to 1024.
> > > 
> > > The number of open file descriptors should be slightly above
> > > nmb_events*nmb_cpus + nmb_cpus(for perf.data.n) + 4*nmb_cpus(for pipes),
> > > which equals 8*nmb_cpus. Therefore, temporarily raise the limit to
> > > 16*nmb_cpus for the test.
> > > 
> > > Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>
> > 
> > Acked-by: Michael Petlan <mpetlan@redhat.com>
> 
> b4 got confused, I'll pick the rigth version manually...

And you forgot to add the perf tools reviewers, as listed in
MAINTAINERS, you can get the list of people who should be on the Cc
using:

⬢[acme@toolbox perf-tools-next]$ scripts/get_maintainer.pl tools/perf/tests/shell/record.sh
Peter Zijlstra <peterz@infradead.org> (supporter:PERFORMANCE EVENTS SUBSYSTEM)
Ingo Molnar <mingo@redhat.com> (supporter:PERFORMANCE EVENTS SUBSYSTEM)
Arnaldo Carvalho de Melo <acme@kernel.org> (supporter:PERFORMANCE EVENTS SUBSYSTEM,commit_signer:3/4=75%)
Namhyung Kim <namhyung@kernel.org> (supporter:PERFORMANCE EVENTS SUBSYSTEM,commit_signer:1/4=25%)
Mark Rutland <mark.rutland@arm.com> (reviewer:PERFORMANCE EVENTS SUBSYSTEM)
Alexander Shishkin <alexander.shishkin@linux.intel.com> (reviewer:PERFORMANCE EVENTS SUBSYSTEM)
Jiri Olsa <jolsa@kernel.org> (reviewer:PERFORMANCE EVENTS SUBSYSTEM)
Ian Rogers <irogers@google.com> (reviewer:PERFORMANCE EVENTS SUBSYSTEM)
Adrian Hunter <adrian.hunter@intel.com> (reviewer:PERFORMANCE EVENTS SUBSYSTEM,commit_signer:1/4=25%,authored:1/4=25%,added_lines:7/55=13%,removed_lines:1/1=100%)
"Liang, Kan" <kan.liang@linux.intel.com> (reviewer:PERFORMANCE EVENTS SUBSYSTEM,commit_signer:1/4=25%,authored:1/4=25%,added_lines:30/55=55%)
Athira Rajeev <atrajeev@linux.vnet.ibm.com> (commit_signer:1/4=25%,authored:1/4=25%)
Veronika Molnarova <vmolnaro@redhat.com> (authored:1/4=25%,added_lines:17/55=31%)
linux-perf-users@vger.kernel.org (open list:PERFORMANCE EVENTS SUBSYSTEM)
linux-kernel@vger.kernel.org (open list:PERFORMANCE EVENTS SUBSYSTEM)
⬢[acme@toolbox perf-tools-next]$


 
> ⬢[acme@toolbox perf-tools-next]$ b4 am -ctsl --cc-trailers 20240429085721.10122-1-vmolnaro@redhat.com
> Grabbing thread from lore.kernel.org/all/20240429085721.10122-1-vmolnaro@redhat.com/t.mbox.gz
> Checking for newer revisions
> Grabbing search results from lore.kernel.org
> Analyzing 7 messages in the thread
> WARNING: duplicate messages found at index 1
>    Subject 1: perf test record.sh: Raise limit of open file descriptors
>    Subject 2: perf test record.sh: Raise limit of open file descriptors
>   2 is not a reply... assume additional patch
> Looking for additional code-review trailers on lore.kernel.org
> Will use the latest revision: v2
> You can pick other revisions using the -vN flag
> Checking attestation on all messages, may take a moment...
> ---
>   ✓ [PATCH v2] perf test record.sh: Raise limit of open file descriptors
>     + Link: https://lore.kernel.org/r/20240311081127.7652-1-vmolnaro@redhat.com (✓ DKIM/kernel.org)
>     + Acked-by: Michael Petlan <mpetlan@redhat.com> (✓ DKIM/redhat.com)
>     + Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>   ✓ [PATCH v2] perf test record.sh: Raise limit of open file descriptors
>     + Acked-by: Michael Petlan <mpetlan@redhat.com> (✓ DKIM/redhat.com)
>     + Link: https://lore.kernel.org/r/20240429085721.10122-1-vmolnaro@redhat.com
>     + Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>   ---
>   ✓ Signed: DKIM/redhat.com
> ---
> Total patches: 2
> ---
>  Link: https://lore.kernel.org/r/20240311081127.7652-1-vmolnaro@redhat.com
>  Base: not specified
>        git am ./v2_20240311_vmolnaro_perf_test_record_sh_raise_limit_of_open_file_descriptors.mbx
> ⬢[acme@toolbox perf-tools-next]$
> 
> > > ---
> > > Reworked the patch as the testfile has already changed since the patch
> > > was sent causing the applying to fail, now applying upstream without
> > > problems.
> > >  tools/perf/tests/shell/record.sh | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> > > index 3d1a7759a7b2..344c0e40ebe8 100755
> > > --- a/tools/perf/tests/shell/record.sh
> > > +++ b/tools/perf/tests/shell/record.sh
> > > @@ -21,6 +21,15 @@ testprog="perf test -w thloop"
> > >  cpu_pmu_dir="/sys/bus/event_source/devices/cpu*"
> > >  br_cntr_file="/caps/branch_counter_nr"
> > >  br_cntr_output="branch stack counters"
> > > +default_fd_limit=$(ulimit -n)
> > > +# With option --threads=cpu the number of open file descriptors should be
> > > +# equal to sum of:    nmb_cpus * nmb_events (2+dummy),
> > > +#                     nmb_threads for perf.data.n (equal to nmb_cpus) and
> > > +#                     2*nmb_cpus of pipes = 4*nmb_cpus (each pipe has 2 ends)
> > > +# All together it needs 8*nmb_cpus file descriptors plus some are also used
> > > +# outside of testing, thus raising the limit to 16*nmb_cpus
> > > +min_fd_limit=$(($(getconf _NPROCESSORS_ONLN) * 16))
> > > +
> > >  
> > >  cleanup() {
> > >    rm -rf "${perfdata}"
> > > @@ -190,11 +199,19 @@ test_branch_counter() {
> > >    echo "Basic branch counter test [Success]"
> > >  }
> > >  
> > > +# raise the limit of file descriptors to minimum
> > > +if [[ $default_fd_limit -lt $min_fd_limit ]]; then
> > > +       ulimit -n $min_fd_limit
> > > +fi
> > > +
> > >  test_per_thread
> > >  test_register_capture
> > >  test_system_wide
> > >  test_workload
> > >  test_branch_counter
> > >  
> > > +# restore the default value
> > > +ulimit -n $default_fd_limit
> > > +
> > >  cleanup
> > >  exit $err
> > > -- 
> > > 2.43.0
> > > 
> > > 
> > 

Re: [PATCH v2] perf test record.sh: Raise limit of open file descriptors

[Arnaldo Carvalho de Melo]

On Mon, Aug 12, 2024 at 10:21:36AM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Aug 12, 2024 at 10:16:10AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Tue, May 21, 2024 at 11:27:56AM +0200, Michael Petlan wrote:
> > > On Mon, 29 Apr 2024, vmolnaro@redhat.com wrote:
> > > > From: Veronika Molnarova <vmolnaro@redhat.com>

> > > > Subtest for system-wide record with '--threads=cpu' option fails due
> > > > to a limit of open file descriptors on systems with 128 or more CPUs
> > > > as the default limit is set to 1024.

> > > > The number of open file descriptors should be slightly above
> > > > nmb_events*nmb_cpus + nmb_cpus(for perf.data.n) + 4*nmb_cpus(for pipes),
> > > > which equals 8*nmb_cpus. Therefore, temporarily raise the limit to
> > > > 16*nmb_cpus for the test.

> > > > Signed-off-by: Veronika Molnarova <vmolnaro@redhat.com>

> > > Acked-by: Michael Petlan <mpetlan@redhat.com>

> > b4 got confused, I'll pick the rigth version manually...

Also please install ShellCheck, with it installed, as I do, you'll get
this warning, that we should address:

  CC      /tmp/build/perf-tools-next/util/synthetic-events.o

In tests/shell/record.sh line 24:
default_fd_limit=$(ulimit -n)
                          ^-- SC3045 (warning): In POSIX sh, ulimit -n is undefined.


In tests/shell/record.sh line 203:
if [[ $default_fd_limit -lt $min_fd_limit ]]; then
   ^-- SC3010 (warning): In POSIX sh, [[ ]] is undefined.


In tests/shell/record.sh line 204:
       ulimit -n $min_fd_limit
              ^-- SC3045 (warning): In POSIX sh, ulimit -n is undefined.


In tests/shell/record.sh line 214:
ulimit -n $default_fd_limit
       ^-- SC3045 (warning): In POSIX sh, ulimit -n is undefined.

For more information:
  https://www.shellcheck.net/wiki/SC3010 -- In POSIX sh, [[ ]] is undefined.
  https://www.shellcheck.net/wiki/SC3045 -- In POSIX sh, ulimit -n is undefin...
make[4]: *** [tests/Build:90: /tmp/build/perf-tools-next/tests/shell/record.sh.shellcheck_log] Error 1
make[3]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:158: tests] Error 2
make[2]: *** [Makefile.perf:777: /tmp/build/perf-tools-next/perf-test-in.o] Error 2
  CC      /tmp/build/perf-tools-next/util/data.o
make[2]: *** Waiting for unfinished jobs....