diff mbox series

perf script: print cgroup on the same line as comm

Message ID	20230718000737.49077-1-ivan@cloudflare.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Ivan Babrou <ivan@cloudflare.com> To: linux-perf-users@vger.kernel.org Cc: kernel-team@cloudflare.com, linux-kernel@vger.kernel.org, Ivan Babrou <ivan@cloudflare.com>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Ian Rogers <irogers@google.com>, Adrian Hunter <adrian.hunter@intel.com> Subject: [PATCH] perf script: print cgroup on the same line as comm Date: Mon, 17 Jul 2023 17:07:37 -0700 Message-ID: <20230718000737.49077-1-ivan@cloudflare.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	perf script: print cgroup on the same line as comm \| perf script: print cgroup on the same line as comm

Commit Message

Ivan Babrou July 18, 2023, 12:07 a.m. UTC

  Commit 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
added support for printing cgroup path in perf script output.

It was okay if you didn't want any stacks:

    $ sudo perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup
    jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
    jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service

With stacks it gets messier as cgroup is printed after the stack:

    $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
    jpegtran:23f4bf 3321915 [013] 404718.587488:
                    5c554 compress_output
                    570d9 jpeg_finish_compress
                    3476e jpegtran_main
                    330ee jpegtran::main
                    326e2 core::ops::function::FnOnce::call_once (inlined)
                    326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
    /idle.slice/polish.service
    jpegtran:23f4bf 3321915 [031] 404718.592073:
                    8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
                55af68e62fff [unknown]
    /idle.slice/polish.service

Let's instead print cgroup on the same line as comm:

    $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
    jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
                    5c554 compress_output
                    570d9 jpeg_finish_compress
                    3476e jpegtran_main
                    330ee jpegtran::main
                    326e2 core::ops::function::FnOnce::call_once (inlined)
                    326e2 std::sys_common::backtrace::__rust_begin_short_backtrace

    jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
                    8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
                55af68e62fff [unknown]

Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
Fixes: 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
---
 tools/perf/builtin-script.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Ivan Babrou July 28, 2023, 5:42 p.m. UTC | #1

On Mon, Jul 17, 2023 at 5:07 PM Ivan Babrou <ivan@cloudflare.com> wrote:
>
> Commit 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
> added support for printing cgroup path in perf script output.
>
> It was okay if you didn't want any stacks:
>
>     $ sudo perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup
>     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
>     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
>
> With stacks it gets messier as cgroup is printed after the stack:
>
>     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
>     jpegtran:23f4bf 3321915 [013] 404718.587488:
>                     5c554 compress_output
>                     570d9 jpeg_finish_compress
>                     3476e jpegtran_main
>                     330ee jpegtran::main
>                     326e2 core::ops::function::FnOnce::call_once (inlined)
>                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
>     /idle.slice/polish.service
>     jpegtran:23f4bf 3321915 [031] 404718.592073:
>                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
>                 55af68e62fff [unknown]
>     /idle.slice/polish.service
>
> Let's instead print cgroup on the same line as comm:
>
>     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
>     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
>                     5c554 compress_output
>                     570d9 jpeg_finish_compress
>                     3476e jpegtran_main
>                     330ee jpegtran::main
>                     326e2 core::ops::function::FnOnce::call_once (inlined)
>                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
>
>     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
>                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
>                 55af68e62fff [unknown]
>
> Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> Fixes: 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
> ---
>  tools/perf/builtin-script.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 200b3e7ea8da..517bf25750c8 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2199,6 +2199,17 @@ static void process_event(struct perf_script *script,
>         if (PRINT_FIELD(RETIRE_LAT))
>                 fprintf(fp, "%16" PRIu16, sample->retire_lat);
>
> +       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);
> +       }
> +
>         if (PRINT_FIELD(IP)) {
>                 struct callchain_cursor *cursor = NULL;
>
> @@ -2243,17 +2254,6 @@ 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");
> --
> 2.41.0
>

A friendly bump in case this slipped through the cracks.

Ian Rogers July 28, 2023, 5:57 p.m. UTC | #2

On Fri, Jul 28, 2023 at 10:42 AM Ivan Babrou <ivan@cloudflare.com> wrote:
>
> On Mon, Jul 17, 2023 at 5:07 PM Ivan Babrou <ivan@cloudflare.com> wrote:
> >
> > Commit 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
> > added support for printing cgroup path in perf script output.
> >
> > It was okay if you didn't want any stacks:
> >
> >     $ sudo perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup
> >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
> >
> > With stacks it gets messier as cgroup is printed after the stack:
> >
> >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> >     jpegtran:23f4bf 3321915 [013] 404718.587488:
> >                     5c554 compress_output
> >                     570d9 jpeg_finish_compress
> >                     3476e jpegtran_main
> >                     330ee jpegtran::main
> >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> >     /idle.slice/polish.service
> >     jpegtran:23f4bf 3321915 [031] 404718.592073:
> >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> >                 55af68e62fff [unknown]
> >     /idle.slice/polish.service
> >
> > Let's instead print cgroup on the same line as comm:
> >
> >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> >                     5c554 compress_output
> >                     570d9 jpeg_finish_compress
> >                     3476e jpegtran_main
> >                     330ee jpegtran::main
> >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> >
> >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
> >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> >                 55af68e62fff [unknown]
> >
> > Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> > Fixes: 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")

This change makes sense to me. Namhyung, wdyt?

Thanks,
Ian


> > ---
> >  tools/perf/builtin-script.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > index 200b3e7ea8da..517bf25750c8 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -2199,6 +2199,17 @@ static void process_event(struct perf_script *script,
> >         if (PRINT_FIELD(RETIRE_LAT))
> >                 fprintf(fp, "%16" PRIu16, sample->retire_lat);
> >
> > +       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);
> > +       }
> > +
> >         if (PRINT_FIELD(IP)) {
> >                 struct callchain_cursor *cursor = NULL;
> >
> > @@ -2243,17 +2254,6 @@ 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");
> > --
> > 2.41.0
> >
>
> A friendly bump in case this slipped through the cracks.

Ivan Babrou Aug. 7, 2023, 6:02 p.m. UTC | #3

On Fri, Jul 28, 2023 at 10:57 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Jul 28, 2023 at 10:42 AM Ivan Babrou <ivan@cloudflare.com> wrote:
> >
> > On Mon, Jul 17, 2023 at 5:07 PM Ivan Babrou <ivan@cloudflare.com> wrote:
> > >
> > > Commit 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
> > > added support for printing cgroup path in perf script output.
> > >
> > > It was okay if you didn't want any stacks:
> > >
> > >     $ sudo perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup
> > >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> > >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
> > >
> > > With stacks it gets messier as cgroup is printed after the stack:
> > >
> > >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> > >     jpegtran:23f4bf 3321915 [013] 404718.587488:
> > >                     5c554 compress_output
> > >                     570d9 jpeg_finish_compress
> > >                     3476e jpegtran_main
> > >                     330ee jpegtran::main
> > >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> > >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> > >     /idle.slice/polish.service
> > >     jpegtran:23f4bf 3321915 [031] 404718.592073:
> > >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> > >                 55af68e62fff [unknown]
> > >     /idle.slice/polish.service
> > >
> > > Let's instead print cgroup on the same line as comm:
> > >
> > >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> > >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> > >                     5c554 compress_output
> > >                     570d9 jpeg_finish_compress
> > >                     3476e jpegtran_main
> > >                     330ee jpegtran::main
> > >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> > >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> > >
> > >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
> > >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> > >                 55af68e62fff [unknown]
> > >
> > > Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> > > Fixes: 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
>
> This change makes sense to me. Namhyung, wdyt?
>
> Thanks,
> Ian

Hi Namhyung,

This is a really trivial patch and it would be good to get a word from you.

Thanks.

> > > ---
> > >  tools/perf/builtin-script.c | 22 +++++++++++-----------
> > >  1 file changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > > index 200b3e7ea8da..517bf25750c8 100644
> > > --- a/tools/perf/builtin-script.c
> > > +++ b/tools/perf/builtin-script.c
> > > @@ -2199,6 +2199,17 @@ static void process_event(struct perf_script *script,
> > >         if (PRINT_FIELD(RETIRE_LAT))
> > >                 fprintf(fp, "%16" PRIu16, sample->retire_lat);
> > >
> > > +       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);
> > > +       }
> > > +
> > >         if (PRINT_FIELD(IP)) {
> > >                 struct callchain_cursor *cursor = NULL;
> > >
> > > @@ -2243,17 +2254,6 @@ 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");
> > > --
> > > 2.41.0
> > >
> >
> > A friendly bump in case this slipped through the cracks.

Arnaldo Carvalho de Melo Aug. 8, 2023, 1:44 p.m. UTC | #4

Em Mon, Aug 07, 2023 at 11:02:01AM -0700, Ivan Babrou escreveu:
> On Fri, Jul 28, 2023 at 10:57 AM Ian Rogers <irogers@google.com> wrote:
> > On Fri, Jul 28, 2023 at 10:42 AM Ivan Babrou <ivan@cloudflare.com> wrote:
> > > On Mon, Jul 17, 2023 at 5:07 PM Ivan Babrou <ivan@cloudflare.com> wrote:
> > > > Commit 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
> > > > added support for printing cgroup path in perf script output.

> > > > It was okay if you didn't want any stacks:

> > > >     $ sudo perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup
> > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service

> > > > With stacks it gets messier as cgroup is printed after the stack:

> > > >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:
> > > >                     5c554 compress_output
> > > >                     570d9 jpeg_finish_compress
> > > >                     3476e jpegtran_main
> > > >                     330ee jpegtran::main
> > > >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> > > >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> > > >     /idle.slice/polish.service
> > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:
> > > >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> > > >                 55af68e62fff [unknown]
> > > >     /idle.slice/polish.service
> > > >
> > > > Let's instead print cgroup on the same line as comm:
> > > >
> > > >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> > > >                     5c554 compress_output
> > > >                     570d9 jpeg_finish_compress
> > > >                     3476e jpegtran_main
> > > >                     330ee jpegtran::main
> > > >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> > > >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> > > >
> > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
> > > >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> > > >                 55af68e62fff [unknown]
> > > >
> > > > Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> > > > Fixes: 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")

> > This change makes sense to me. Namhyung, wdyt?
 
> Hi Namhyung,
> 
> This is a really trivial patch and it would be good to get a word from you.

Hi, this solves the case for cgroup and I think it should be merged, but
what about the other fields that are being printed after the callchain
gets printed?

I looked and we would have to introduce a __sample__fprintf_sym that
didn't call sample__fprintf_callchain and use it in perf script's
process_event() then later call sample__fprintf_callchain after all the
fields that print on the same line.

Anyway, Namhyung, can I have your Acked-by for this patch to move things
forward at least for cgroups?

- Arnaldo

Arnaldo Carvalho de Melo Aug. 8, 2023, 1:48 p.m. UTC | #5

Em Tue, Aug 08, 2023 at 10:44:26AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Aug 07, 2023 at 11:02:01AM -0700, Ivan Babrou escreveu:
> > On Fri, Jul 28, 2023 at 10:57 AM Ian Rogers <irogers@google.com> wrote:
> > > On Fri, Jul 28, 2023 at 10:42 AM Ivan Babrou <ivan@cloudflare.com> wrote:
> > > > On Mon, Jul 17, 2023 at 5:07 PM Ivan Babrou <ivan@cloudflare.com> wrote:
> > > > > Commit 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
> > > > > added support for printing cgroup path in perf script output.
> 
> > > > > It was okay if you didn't want any stacks:
> 
> > > > >     $ sudo perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup
> > > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> > > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
> 
> > > > > With stacks it gets messier as cgroup is printed after the stack:
> 
> > > > >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> > > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:
> > > > >                     5c554 compress_output
> > > > >                     570d9 jpeg_finish_compress
> > > > >                     3476e jpegtran_main
> > > > >                     330ee jpegtran::main
> > > > >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> > > > >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> > > > >     /idle.slice/polish.service
> > > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:
> > > > >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> > > > >                 55af68e62fff [unknown]
> > > > >     /idle.slice/polish.service
> > > > >
> > > > > Let's instead print cgroup on the same line as comm:
> > > > >
> > > > >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> > > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> > > > >                     5c554 compress_output
> > > > >                     570d9 jpeg_finish_compress
> > > > >                     3476e jpegtran_main
> > > > >                     330ee jpegtran::main
> > > > >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> > > > >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> > > > >
> > > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
> > > > >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> > > > >                 55af68e62fff [unknown]
> > > > >
> > > > > Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> > > > > Fixes: 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
> 
> > > This change makes sense to me. Namhyung, wdyt?
>  
> > Hi Namhyung,
> > 
> > This is a really trivial patch and it would be good to get a word from you.
> 
> Hi, this solves the case for cgroup and I think it should be merged, but
> what about the other fields that are being printed after the callchain
> gets printed?
> 
> I looked and we would have to introduce a __sample__fprintf_sym that
> didn't call sample__fprintf_callchain and use it in perf script's
> process_event() then later call sample__fprintf_callchain after all the
> fields that print on the same line.

Nah, or simply moving sample__fprintf_sym() to the end of that function.

- Arnaldo
 
> Anyway, Namhyung, can I have your Acked-by for this patch to move things
> forward at least for cgroups?

Namhyung Kim Aug. 8, 2023, 1:49 p.m. UTC | #6

Hello,

Sorry for the late reply.

On Tue, Aug 8, 2023 at 10:44 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Mon, Aug 07, 2023 at 11:02:01AM -0700, Ivan Babrou escreveu:
> > On Fri, Jul 28, 2023 at 10:57 AM Ian Rogers <irogers@google.com> wrote:
> > > On Fri, Jul 28, 2023 at 10:42 AM Ivan Babrou <ivan@cloudflare.com> wrote:
> > > > On Mon, Jul 17, 2023 at 5:07 PM Ivan Babrou <ivan@cloudflare.com> wrote:
> > > > > Commit 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
> > > > > added support for printing cgroup path in perf script output.
>
> > > > > It was okay if you didn't want any stacks:
>
> > > > >     $ sudo perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup
> > > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> > > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
>
> > > > > With stacks it gets messier as cgroup is printed after the stack:
>
> > > > >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> > > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:
> > > > >                     5c554 compress_output
> > > > >                     570d9 jpeg_finish_compress
> > > > >                     3476e jpegtran_main
> > > > >                     330ee jpegtran::main
> > > > >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> > > > >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> > > > >     /idle.slice/polish.service
> > > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:
> > > > >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> > > > >                 55af68e62fff [unknown]
> > > > >     /idle.slice/polish.service
> > > > >
> > > > > Let's instead print cgroup on the same line as comm:
> > > > >
> > > > >     $ perf script --comms jpegtran:23f4bf -F comm,tid,cpu,time,cgroup,ip,sym
> > > > >     jpegtran:23f4bf 3321915 [013] 404718.587488:  /idle.slice/polish.service
> > > > >                     5c554 compress_output
> > > > >                     570d9 jpeg_finish_compress
> > > > >                     3476e jpegtran_main
> > > > >                     330ee jpegtran::main
> > > > >                     326e2 core::ops::function::FnOnce::call_once (inlined)
> > > > >                     326e2 std::sys_common::backtrace::__rust_begin_short_backtrace
> > > > >
> > > > >     jpegtran:23f4bf 3321915 [031] 404718.592073:  /idle.slice/polish.service
> > > > >                     8474d jsimd_encode_mcu_AC_first_prepare_sse2.PADDING
> > > > >                 55af68e62fff [unknown]
> > > > >
> > > > > Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> > > > > Fixes: 3fd7a168bf51 ("perf script: Add 'cgroup' field for output")
>
> > > This change makes sense to me. Namhyung, wdyt?
>
> > Hi Namhyung,
> >
> > This is a really trivial patch and it would be good to get a word from you.
>
> Hi, this solves the case for cgroup and I think it should be merged, but
> what about the other fields that are being printed after the callchain
> gets printed?
>
> I looked and we would have to introduce a __sample__fprintf_sym that
> didn't call sample__fprintf_callchain and use it in perf script's
> process_event() then later call sample__fprintf_callchain after all the
> fields that print on the same line.
>
> Anyway, Namhyung, can I have your Acked-by for this patch to move things
> forward at least for cgroups?

I'm ok with the change itself.  But I'm afraid other fields might be
changed too.  Anyway,

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

diff mbox series

Patch

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 200b3e7ea8da..517bf25750c8 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -2199,6 +2199,17 @@  static void process_event(struct perf_script *script,
 	if (PRINT_FIELD(RETIRE_LAT))
 		fprintf(fp, "%16" PRIu16, sample->retire_lat);
 
+	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);
+	}
+
 	if (PRINT_FIELD(IP)) {
 		struct callchain_cursor *cursor = NULL;
 
@@ -2243,17 +2254,6 @@  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");