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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Wed, Aug 14, 2024 at 05:17:34PM +0200, 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.

So it is not applying anymore, Kan Liang changed that file, I'm fixing
it up:

⬢[acme@toolbox perf-tools-next]$        git am ./v3_20240814_vmolnaro_perf_test_record_sh_raise_limit_of_open_file_descriptors.mbx
Applying: perf test record.sh: Raise limit of open file descriptors
error: patch failed: tools/perf/tests/shell/record.sh:22
error: tools/perf/tests/shell/record.sh: patch does not apply
Patch failed at 0001 perf test record.sh: Raise limit of open file descriptors
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
⬢[acme@toolbox perf-tools-next]$

Doing it manually makes us go farther:

⬢[acme@toolbox perf-tools-next]$ patch -p1 < ./v3_20240814_vmolnaro_perf_test_record_sh_raise_limit_of_open_file_descriptors.mbx
patching file tools/perf/tests/shell/record.sh
Hunk #1 succeeded at 23 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 123 (offset 1 line).
Hunk #3 succeeded at 217 with fuzz 1 (offset 7 lines).
⬢[acme@toolbox perf-tools-next]$

After applying it with the resulting diff, I think we should not disable
those changes, just state the precise shell we think is needed for this
particular test:

⬢[acme@toolbox perf-tools-next]$ git diff
diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 28f5fe3b380e0b06..de913ebeb54ba220 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 # perf record tests
 # SPDX-License-Identifier: GPL-2.0
 
@@ -32,7 +32,6 @@ ulimit_supported=false
 # outside of testing, thus raising the limit to 16*nmb_cpus
 min_fd_limit=$(($(getconf _NPROCESSORS_ONLN) * 16))
 
-# shellcheck disable=SC3045 # ulimit is required for testing --threads option
 default_fd_limit=$(ulimit -Sn)
 if [ $? -eq 0 ]; then
   ulimit_supported=true
@@ -219,7 +218,6 @@ test_branch_counter() {
 
 # raise the limit of file descriptors to required minimum
 if [ $ulimit_supported ] && [ $default_fd_limit -lt $min_fd_limit ]; then
-  # shellcheck disable=SC3045 # ulimit is required for testing --threads option
   ulimit -Sn $min_fd_limit
 fi
 
@@ -231,7 +229,6 @@ test_branch_counter
 
 # restore the default value
 if [ $ulimit_supported ]; then
-  # shellcheck disable=SC3045 # ulimit is required for testing --threads option
   ulimit -Sn $default_fd_limit
 fi
 
⬢[acme@toolbox perf-tools-next]$

I.e. just state that /bin/bash is needed, like is done in multiple other
shell tests:

⬢[acme@toolbox perf-tools-next]$ head -1 tools/perf/tests/shell/*.sh | grep bash | wc -l
16
⬢[acme@toolbox perf-tools-next]$

Ok? I'm applying it with this change. Thanks,

- Arnaldo