Re: [PATCH v3 2/2] perf test: Add stat metrics --for-each-cgroup test

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

On Tue, May 19, 2026 at 10:09:57AM -0700, Namhyung Kim wrote:
> On Tue, May 19, 2026 at 08:13:02AM -0700, Ian Rogers wrote:
> > On Mon, May 18, 2026 at 10:54 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > On Mon, May 18, 2026 at 10:01:50PM -0700, Ian Rogers wrote:
> > > > +             # Parse each metric line
> > > > +             while read -r line; do
> > > > +                     [ -z "${line}" ] && continue
> > > > +
> > > > +                     local cycles_val
> > > > +                     cycles_val=$(echo "${line}" | cut -d, -f1)
> > > > +
> > > > +                     local event_name
> > > > +                     event_name=$(echo "${line}" | cut -d, -f3)
> > > > +
> > > > +                     # Find corresponding instructions event line
> > > > +                     local inst_event_name
> > > > +                     inst_event_name="${event_name/cpu-cycles/instructions}"
> > > > +                     inst_event_name="${inst_event_name/cycles/instructions}"
> > > > +
> > > > +                     local inst_line
> > > > +                     inst_line=$(echo "${output}" | \
> > > > +                             grep -F ",${cgrp}," | \
> > > > +                             grep -F "${inst_event_name}" || true)
> > > > +
> > > > +                     local inst_val
> > > > +                     inst_val=$(echo "${inst_line}" | cut -d, -f1)
> > > > +
> > > > +                     log_verbose "Cgroup '${cgrp}': event '${event_name}' \
> > > > +val '${cycles_val}', inst event '${inst_event_name}' val '${inst_val}'"

> > > Why not make log_verbose() take multiple arguments and align them
> > > properly?  Maybe you can just use 'echo'?  The same goes to the below
> > > lines.
 
> > This fixes checkpatch warnings on line length and the odd alignment
> > avoids spaces appearing in the output.
 
> How about this?

Ian posted a v4, are you ok with that now, Tested-by or Reviewed-by?

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> diff --git a/tools/perf/tests/shell/stat_metrics_cgrp.sh b/tools/perf/tests/shell/stat_metrics_cgrp.sh
> index d4226ee0ae9801cb..ecb2c053ea5158d4 100755
> --- a/tools/perf/tests/shell/stat_metrics_cgrp.sh
> +++ b/tools/perf/tests/shell/stat_metrics_cgrp.sh
> @@ -7,7 +7,7 @@ set -e
>  test_cgroups=
>  
>  log_verbose() {
> -	echo "$1"
> +	echo "$*"
>  }
>  
>  is_numeric_and_non_zero() {
> @@ -145,22 +145,22 @@ check_metric_reported()
>  				cycles_val=$(echo "${cycles_line}" | cut -d, -f1)
>  			fi
>  
> -			log_verbose "Cgroup '${cgrp}': event '${event_name}' \
> -val '${cycles_val}', inst val '${inst_val}'"
> +			log_verbose "Cgroup '${cgrp}': event '${event_name}'" \
> +				    "val '${cycles_val}', inst val '${inst_val}'"
>  
>  			# Only enforce metric check if both cycles and
>  			# instructions have non-zero numeric counts
>  			if is_numeric_and_non_zero "${cycles_val}" && \
>  			   is_numeric_and_non_zero "${inst_val}"
>  			then
> -				log_verbose "Enforcing metric check for cgroup '${cgrp}' \
> -event '${event_name}'"
> +				log_verbose "Enforcing metric check for cgroup '${cgrp}'" \
> +					    "event '${event_name}'"
>  				# Check for nan or nested in the metric value (7th field)
>  				# Actually we can just check the whole line for simplicity
>  				if echo "${line}" | grep -q -i -E ",nan,|,nested,"
>  				then
> -					echo "FAIL: Invalid metric value (nan/nested) \
> -for cgroup '${cgrp}'"
> +					echo "FAIL: Invalid metric value (nan/nested)" \
> +					     "for cgroup '${cgrp}'"
>  					echo "Line: ${line}"
>  					exit 1
>  				fi
> @@ -173,8 +173,8 @@ for cgroup '${cgrp}'"
>  					exit 1
>  				fi
>  			else
> -				log_verbose "Skipping metric check for cgroup '${cgrp}' \
> -event '${event_name}' (idle or not counted)"
> +				log_verbose "Skipping metric check for cgroup '${cgrp}'" \
> +					    "event '${event_name}' (idle or not counted)"
>  			fi
>  		done <<< "${cgrp_lines}"
>  	done