Date:   Wed, 1 Feb 2023 22:26:22 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Jiri Olsa <jolsa@kernel.org>, Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Ian Rogers <irogers@google.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        linux-perf-users@vger.kernel.org,
        Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH v2] perf script: Add 'cgroup' field for output
Message-ID: <Y9sRPi3hr/tvpyaM@kernel.org>
References: <20230126213610.3381147-1-namhyung@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230126213610.3381147-1-namhyung@kernel.org>
Precedence: bulk

Em Thu, Jan 26, 2023 at 01:36:10PM -0800, Namhyung Kim escreveu:
> There's no field for the cgroup, let's add one.  To do that, users need to
> specify --all-cgroup option for perf record to capture the cgroup info.
> 
>   $ perf record --all-cgroups -- true
> 
>   $ perf script -F comm,pid,cgroup
>             true 337112  /user.slice/user-657345.slice/user@657345.service/...
>             true 337112  /user.slice/user-657345.slice/user@657345.service/...
>             true 337112  /user.slice/user-657345.slice/user@657345.service/...
>             true 337112  /user.slice/user-657345.slice/user@657345.service/...
> 
> If it's recorded without the --all-cgroups, it'd complain.
> 
>   $ perf script -F comm,pid,cgroup
>   Samples for 'cycles:u' event do not have CGROUP attribute set. Cannot print 'cgroup' field.
>   Hint: run 'perf record --all-cgroups ...'

Thanks, applied.

- Arnaldo

 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/Documentation/perf-script.txt |  5 ++++-
>  tools/perf/builtin-script.c              | 22 +++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
> index 8d77182fbf31..a2ebadc9d948 100644
> --- a/tools/perf/Documentation/perf-script.txt
> +++ b/tools/perf/Documentation/perf-script.txt
> @@ -134,7 +134,7 @@ OPTIONS
>          srcline, period, iregs, uregs, brstack, brstacksym, flags, bpf-output,
>          brstackinsn, brstackinsnlen, brstackoff, callindent, insn, insnlen, synth,
>          phys_addr, metric, misc, srccode, ipc, data_page_size, code_page_size, ins_lat,
> -        machine_pid, vcpu.
> +        machine_pid, vcpu, cgroup.
>          Field list can be prepended with the type, trace, sw or hw,
>          to indicate to which event type the field list applies.
>          e.g., -F sw:comm,tid,time,ip,sym  and -F trace:time,cpu,trace
> @@ -231,6 +231,9 @@ OPTIONS
>  	perf inject to insert a perf.data file recorded inside a virtual machine into
>  	a perf.data file recorded on the host at the same time.
>  
> +	The cgroup fields requires sample having the cgroup id which is saved
> +	when "--all-cgroups" option is passed to 'perf record'.
> +
>  	Finally, a user may not set fields to none for all event types.
>  	i.e., -F "" is not allowed.
>  
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 69394ac0a20d..731821fd0d3e 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -59,6 +59,7 @@
>  #include "util/dlfilter.h"
>  #include "util/record.h"
>  #include "util/util.h"
> +#include "util/cgroup.h"
>  #include "perf.h"
>  
>  #include <linux/ctype.h>
> @@ -130,6 +131,7 @@ enum perf_output_field {
>  	PERF_OUTPUT_BRSTACKINSNLEN  = 1ULL << 36,
>  	PERF_OUTPUT_MACHINE_PID     = 1ULL << 37,
>  	PERF_OUTPUT_VCPU            = 1ULL << 38,
> +	PERF_OUTPUT_CGROUP          = 1ULL << 39,
>  };
>  
>  struct perf_script {
> @@ -200,6 +202,7 @@ struct output_option {
>  	{.str = "brstackinsnlen", .field = PERF_OUTPUT_BRSTACKINSNLEN},
>  	{.str = "machine_pid", .field = PERF_OUTPUT_MACHINE_PID},
>  	{.str = "vcpu", .field = PERF_OUTPUT_VCPU},
> +	{.str = "cgroup", .field = PERF_OUTPUT_CGROUP},
>  };
>  
>  enum {
> @@ -542,6 +545,12 @@ static int evsel__check_attr(struct evsel *evsel, struct perf_session *session)
>  	    evsel__check_stype(evsel, PERF_SAMPLE_WEIGHT_STRUCT, "WEIGHT_STRUCT", PERF_OUTPUT_INS_LAT))
>  		return -EINVAL;
>  
> +	if (PRINT_FIELD(CGROUP) &&
> +	    evsel__check_stype(evsel, PERF_SAMPLE_CGROUP, "CGROUP", PERF_OUTPUT_CGROUP)) {
> +		pr_err("Hint: run 'perf record --all-cgroups ...'\n");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2220,6 +2229,17 @@ static void process_event(struct perf_script *script,
>  	if (PRINT_FIELD(CODE_PAGE_SIZE))
>  		fprintf(fp, " %s", get_page_size_name(sample->code_page_size, str));
>  
> +	if (PRINT_FIELD(CGROUP)) {
> +		const char *cgrp_name;
> +		struct cgroup *cgrp = cgroup__find(machine->env,
> +						   sample->cgroup);
> +		if (cgrp != NULL)
> +			cgrp_name = cgrp->name;
> +		else
> +			cgrp_name = "unknown";
> +		fprintf(fp, " %s", cgrp_name);
> +	}
> +
>  	perf_sample__fprintf_ipc(sample, attr, fp);
>  
>  	fprintf(fp, "\n");
> @@ -3856,7 +3876,7 @@ int cmd_script(int argc, const char **argv)
>  		     "brstacksym,flags,data_src,weight,bpf-output,brstackinsn,"
>  		     "brstackinsnlen,brstackoff,callindent,insn,insnlen,synth,"
>  		     "phys_addr,metric,misc,srccode,ipc,tod,data_page_size,"
> -		     "code_page_size,ins_lat",
> +		     "code_page_size,ins_lat,machine_pid,vcpu,cgroup",
>  		     parse_output_fields),
>  	OPT_BOOLEAN('a', "all-cpus", &system_wide,
>  		    "system-wide collection from all CPUs"),
> -- 
> 2.39.1.456.gfc5497dd1b-goog
> 

-- 

- Arnaldo