Re: [PATCH v2 4/6] perf evsel: Add/use accessor for tp_format

[Ian Rogers]

On Tue, Nov 5, 2024 at 11:52 AM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
>
> On Tue, Nov 05, 2024 at 11:33:09AM -0800, Ian Rogers wrote:
> > On Tue, Nov 5, 2024 at 9:36 AM Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:
> > > On Sat, Nov 02, 2024 at 09:53:58AM -0700, Ian Rogers wrote:
> > > > Add an accessor function for tp_format. Rather than search+replace
> > > > uses try to use a variable and reuse it. Add additional NULL checks
> > > > when accessing/using the value. Make sure the PTR_ERR is nulled out on
> > > > error path in evsel__newtp_idx.
>
> > > > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
>
> > > > +++ b/tools/perf/builtin-kmem.c
> > > > @@ -772,8 +773,9 @@ static int parse_gfp_flags(struct evsel *evsel, struct perf_sample *sample,
> > > >       }
> > > >
> > > >       trace_seq_init(&seq);
> > > > -     tep_print_event(evsel->tp_format->tep,
> > > > -                     &seq, &record, "%s", TEP_PRINT_INFO);
> > > > +     tp_format = evsel__tp_format(evsel);
> > > > +     if (tp_format)
> > > > +             tep_print_event(tp_format->tep, &seq, &record, "%s", TEP_PRINT_INFO);
> > > >
> > > >       str = strtok_r(seq.buffer, " ", &pos);
> > > >       while (str) {
> > > > @@ -2011,14 +2013,15 @@ int cmd_kmem(int argc, const char **argv)
> > > >       }
> > > >
> > > >       if (kmem_page) {
> > > > -             struct evsel *evsel = evlist__find_tracepoint_by_name(session->evlist, "kmem:mm_page_alloc");
> > > > +             struct evsel *evsel = evlist__find_tracepoint_by_name(session->evlist,
> > > > +                                                                   "kmem:mm_page_alloc");
> > >
> > > Try to avoid these reflows to reduce patch size, please.
> >
> > Agreed, in this case check `checkpatch.pl` is complaining that the
> > line length is 109 columns.
>
> Ok, but it was like that before, your patch is not touching that line
> for some other reason, so it is unrelated to what you're doing, causing
> a distraction.
>
> Besides, even the documentation for checkpatch mentions that that is:
>
>   **LONG_LINE**
>     The line has exceeded the specified maximum length.
>     To use a different maximum line length, the --max-line-length=n option
>     may be added while invoking checkpatch.
>
>     Earlier, the default line length was 80 columns.  Commit bdc48fa11e46
>     ("checkpatch/coding-style: deprecate 80-column warning") increased the
>     limit to 100 columns.  This is not a hard limit either and it's
>     preferable to stay within 80 columns whenever possible.
>
>     See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
>
> ----------------
>
> So if it was there already, meaning the original author preferred it
> that way, its not touched by the feature that is being worked on in this
> patch, ends up being an extra hunk, just reflowing, a distraction.

Wow, respecting the author's preferences, sounds like a good thing to
be doing :-) Like I say it came up from checkpatch.pl as I changed the
next line and it was over by quite a way so I felt it was innocuous to
fix it. The variable on this line is used on the next line (the one I
changed) so it isn't as if they are entirely unrelated. I agreed to
put it back. Often reviews like this feel like you are saying
arbitrary changes were made for no reason, which I have a long
experience of avoiding doing. I also try to make sure I don't send
patches upstream that checkpath.pl warns about, its a shame whoever
authored the line didn't think similarly.

Thanks,
Ian