Received-SPF: pass (google.com: domain of linux-kernel+bounces-160280-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <1e2bee89-0a4a-42e3-9f81-bd7296cfb351@intel.com>
Date: Fri, 26 Apr 2024 18:24:12 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V3] perf scripts python: Add a script to run instances of
 perf script in parallel
To: Arnaldo Carvalho de Melo <acme@kernel.org>,
 Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
 Ian Rogers <irogers@google.com>, linux-kernel@vger.kernel.org,
 linux-perf-users@vger.kernel.org
References: <20240423133248.10206-1-adrian.hunter@intel.com>
 <ZihFYbdrnarNFWOd@tassilo> <ZikT69GIsijZajoI@x1> <ZiuiiFdPhJIrjI7k@x1>
Content-Language: en-US
From: Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <ZiuiiFdPhJIrjI7k@x1>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 26/04/24 15:48, Arnaldo Carvalho de Melo wrote:
> On Wed, Apr 24, 2024 at 11:15:11AM -0300, Arnaldo Carvalho de Melo wrote:
>> On Tue, Apr 23, 2024 at 04:33:53PM -0700, Andi Kleen wrote:
>>> On Tue, Apr 23, 2024 at 04:32:48PM +0300, Adrian Hunter wrote:
>>>> The script is useful for Intel PT traces, that can be efficiently
>>>> decoded by perf script when split by CPU and/or time ranges. Running
>>>> jobs in parallel can decrease the overall decoding time.
> 
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> 
>>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> 
>> Thanks, applied, and added a note on testing it using:
> 
> Plus this, please check.
> 
> Committer testing:
> 
>   Ian reported that shellcheck found some issues, I installed it as there
>   are no warnings about it not being available, but when available it
>   fails the build with:
> 
>     TEST    /tmp/build/perf-tools-next/tests/shell/script.sh.shellcheck_log
>     CC      /tmp/build/perf-tools-next/util/header.o
> 
>   In tests/shell/script.sh line 20:
>                   rm -rf "${temp_dir}/"*
>                          ^-------------^ SC2115 (warning): Use "${var:?}" to ensure this never expands to /* .
> 
> 
>   In tests/shell/script.sh line 83:
>           output1_dir="${temp_dir}/output1"
>           ^---------^ SC2034 (warning): output1_dir appears unused. Verify use (or export if used externally).
> 
> 
>   In tests/shell/script.sh line 84:
>           output2_dir="${temp_dir}/output2"
>           ^---------^ SC2034 (warning): output2_dir appears unused. Verify use (or export if used externally).
> 
> 
>   In tests/shell/script.sh line 86:
>           python3 "${pp}" -o "${output_dir}" --jobs 4 --verbose -- perf script -i "${perf_data}"
>                               ^-----------^ SC2154 (warning): output_dir is referenced but not assigned (did you mean 'output1_dir'?).
> 
>   For more information:
>     https://www.shellcheck.net/wiki/SC2034 -- output1_dir appears unused. Verif...
>     https://www.shellcheck.net/wiki/SC2115 -- Use "${var:?}" to ensure this nev...
>     https://www.shellcheck.net/wiki/SC2154 -- output_dir is referenced but not ...
> 
> Did these fixes:
> 
>   -               rm -rf "${temp_dir}/"*
>   +               rm -rf "${temp_dir:?}/"*
> 
> And:
> 
>    @@ -83,8 +83,8 @@ test_parallel_perf()
>           output1_dir="${temp_dir}/output1"
>           output2_dir="${temp_dir}/output2"
>           perf record -o "${perf_data}" --sample-cpu uname
>   -       python3 "${pp}" -o "${output_dir}" --jobs 4 --verbose -- perf script -i "${perf_data}"
>   -       python3 "${pp}" -o "${output_dir}" --jobs 4 --verbose --per-cpu -- perf script -i "${perf_data}"
>   +       python3 "${pp}" -o "${output1_dir}" --jobs 4 --verbose -- perf script -i "${perf_data}"
>   +       python3 "${pp}" -o "${output2_dir}" --jobs 4 --verbose --per-cpu -- perf script -i "${perf_data}"
> 

Sorry, didn't have shellcheck installed!

Looks good. The shellcheck page refers to bash for "${var:?}"
but it is POSIX too.