linux-kernel - Re: [PATCH v2 4/6] perf evsel: Add/use accessor for tp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAP-5=fVGr5bq5MAnWvfyOKT0ggC0pjZ3uNHKtW2HP3pHHVYEVA@mail.gmail.com>
Date: Tue, 5 Nov 2024 13:04:20 -0800
From: Ian Rogers <irogers@...gle.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	Athira Jajeev <atrajeev@...ux.vnet.ibm.com>, James Clark <james.clark@...aro.org>, 
	Dominique Martinet <asmadeus@...ewreck.org>, Yang Li <yang.lee@...ux.alibaba.com>, 
	Colin Ian King <colin.i.king@...il.com>, Yang Jihong <yangjihong@...edance.com>, 
	"Steinar H. Gunderson" <sesse@...gle.com>, Oliver Upton <oliver.upton@...ux.dev>, 
	Ilkka Koskinen <ilkka@...amperecomputing.com>, Ze Gao <zegao2021@...il.com>, 
	Weilin Wang <weilin.wang@...el.com>, Ben Gainey <ben.gainey@....com>, 
	zhaimingbing <zhaimingbing@...s.chinamobile.com>, Zixian Cai <fzczx123@...il.com>, 
	Andi Kleen <ak@...ux.intel.com>, Paran Lee <p4ranlee@...il.com>, 
	Thomas Falcon <thomas.falcon@...el.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, 
	"Steven Rostedt (Google)" <rostedt@...dmis.org>
Subject: Re: [PATCH v2 4/6] perf evsel: Add/use accessor for tp_format

On Tue, Nov 5, 2024 at 11:52 AM Arnaldo Carvalho de Melo
<acme@...nel.org> 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@...nel.org> 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@...gle.com>
>
> > > > +++ 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