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] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZtFNGuwj0WzRQ8fd@google.com>
Date: Thu, 29 Aug 2024 21:39:54 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, 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>,
	Nick Terrell <terrelln@...com>, Yanteng Si <siyanteng@...ngson.cn>,
	Yicong Yang <yangyicong@...ilicon.com>,
	James Clark <james.clark@...aro.org>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 5/8] perf header: Allow attributes to be written after
 data

On Thu, Aug 29, 2024 at 02:42:38PM -0700, Ian Rogers wrote:
> On Thu, Aug 29, 2024 at 1:58 PM Arnaldo Carvalho de Melo
> <acme@...nel.org> wrote:
> >
> > On Thu, Aug 29, 2024 at 01:12:32PM -0700, Ian Rogers wrote:
> > > On Thu, Aug 29, 2024 at 12:31 PM Arnaldo Carvalho de Melo
> > > <acme@...nel.org> wrote:
> > > >
> > > > On Thu, Aug 29, 2024 at 08:01:51AM -0700, Ian Rogers wrote:
> > > > > With a file, to write data an offset needs to be known. Typically data
> > > > > follows the event attributes in a file. However, if processing a pipe
> > > > > the number of event attributes may not be known. It is convenient in
> > > > > that case to write the attributes after the data. Expand
> > > > > perf_session__do_write_header to allow this when the data offset and
> > > > > size are known.
> > > > >
> > > > > This approach may be useful for more than just taking a pipe file to
> > > > > write into a data file, `perf inject --itrace` will reserve and
> > > > > additional 8kb for attributes, which would be unnecessary if the
> > > > > attributes were written after the data.
> > > > >
> > > > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > > > > ---
> > > > >  tools/perf/util/header.c | 106 +++++++++++++++++++++++++--------------
> > > > >  1 file changed, 67 insertions(+), 39 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > > > > index 65c9086610cb..4eb39463067e 100644
> > > > > --- a/tools/perf/util/header.c
> > > > > +++ b/tools/perf/util/header.c
> > > > > @@ -3676,32 +3676,50 @@ int perf_header__write_pipe(int fd)
> > > > >  static int perf_session__do_write_header(struct perf_session *session,
> > > > >                                        struct evlist *evlist,
> > > > >                                        int fd, bool at_exit,
> > > > > -                                      struct feat_copier *fc)
> > > > > +                                      struct feat_copier *fc,
> > > > > +                                      bool write_attrs_after_data)
> > > > >  {
> > > > >       struct perf_file_header f_header;
> > > > > -     struct perf_file_attr   f_attr;
> > > > >       struct perf_header *header = &session->header;
> > > > >       struct evsel *evsel;
> > > > >       struct feat_fd ff = {
> > > > >               .fd = fd,
> > > > >       };
> > > > > -     u64 attr_offset;
> > > > > +     u64 attr_offset = sizeof(f_header), attr_size = 0;
> > > > >       int err;
> > > > >
> > > > > -     lseek(fd, sizeof(f_header), SEEK_SET);
> > > > > +     if (write_attrs_after_data && at_exit) {
> > > > > +             /*
> > > > > +              * Write features at the end of the file first so that
> > > > > +              * attributes may come after them.
> > > > > +              */
> > > > > +             if (!header->data_offset && header->data_size) {
> > > > > +                     pr_err("File contains data but offset unknown\n");
> > > > > +                     err = -1;
> > > > > +                     goto err_out;
> > > > > +             }
> > > > > +             header->feat_offset = header->data_offset + header->data_size;
> > > > > +             err = perf_header__adds_write(header, evlist, fd, fc);
> > > > > +             if (err < 0)
> > > > > +                     goto err_out;
> > > > > +             attr_offset = lseek(fd, 0, SEEK_CUR);
> > > > > +     } else {
> > > > > +             lseek(fd, attr_offset, SEEK_SET);
> > > > > +     }
> > > > >
> > > > >       evlist__for_each_entry(session->evlist, evsel) {
> > > > > -             evsel->id_offset = lseek(fd, 0, SEEK_CUR);
> > > > > -             err = do_write(&ff, evsel->core.id, evsel->core.ids * sizeof(u64));
> > > > > -             if (err < 0) {
> > > > > -                     pr_debug("failed to write perf header\n");
> > > > > -                     free(ff.buf);
> > > > > -                     return err;
> > > > > +             evsel->id_offset = attr_offset;
> > > > > +             /* Avoid writing at the end of the file until the session is exiting. */
> > > > > +             if (!write_attrs_after_data || at_exit) {
> > > > > +                     err = do_write(&ff, evsel->core.id, evsel->core.ids * sizeof(u64));
> > > > > +                     if (err < 0) {
> > > > > +                             pr_debug("failed to write perf header\n");
> > > > > +                             goto err_out;
> > > > > +                     }
> > > > >               }
> > > > > +             attr_offset += evsel->core.ids * sizeof(u64);
> > > >
> > > > So in the past we were using lseek(fd, 0, SEEK_CUR) to set the
> > > > evsel->id_offset, now you're assuming that do_write will honour the size
> > > > parameter, i.e. write evsel->core.ids * sizeof(u64), but:
> > > >
> > > > /* Return: 0 if succeeded, -ERR if failed. */
> > > > int do_write(struct feat_fd *ff, const void *buf, size_t size)
> > > > {
> > > >         if (!ff->buf)
> > > >                 return __do_write_fd(ff, buf, size);
> > > >         return __do_write_buf(ff, buf, size);
> > > > }
> > > >
> > > > And then:
> > > >
> > > > static int __do_write_fd(struct feat_fd *ff, const void *buf, size_t size)
> > > > {
> > > >         ssize_t ret = writen(ff->fd, buf, size);
> > > >
> > > >         if (ret != (ssize_t)size)
> > > >                 return ret < 0 ? (int)ret : -1;
> > > >         return 0;
> > > > }
> > > >
> > > > I see that writen calls ion that even has a BUG_ON() if it doesn't write
> > > > exactly the requested size bytes, I got distracted with __do_write_fd
> > > > extra check that ret != size returning ret if not negative...
> > > >
> > > > I.e. your code _should_ be equivalent due to the check in ion(), and
> > > > taking that as an assumption you reduce the number of lseek syscalls,
> > > > which is a good thing...
> > > >
> > > > I was just trying to see that the !write_attrs_after_data case was
> > > > _exactly_ the same as before, which it doesn't look like it is :-\
> > >
> > > I'm not seeing the difference. Before:
> >
> > You noticed the difference: before we used lseek to get the current
> > offset to use, afterwards we moved to doing plain math.
> >
> > So I had to check if we could assume that, and with the current code
> > structure, yes, we can assume that, so seems safe, but it is different
> > and if the assumption somehow breaks, as the code in __do_write_fd()
> > guard against (unneeded at the moment as ion has even a BUG_ON for that
> > not to happen), then the offset will not be where the data is.
> >
> > Using lseek() is more costly (syscalls) but it is the ultimate answer to
> > get where in the file the current offset is.
> >
> > So that is the difference I noticed.
> >
> > Doing multiple things in the same patch causes these reviewing delays,
> > doubts, its something we discussed multiple times in the past, and that
> > continue to cause these discussions.
> 
> Right, but it is something of an unfortunate coincidence of how the
> code is structured. The fact that writing the header updates
> data_offset which is a thing that other things depend upon while
> depending on its value itself, etc. - ie the function does more than
> just a write, it also sometimes computes the layout, has inbuilt
> assumptions on the values lseek will return, and so on. To get to this
> final structure took a fair few iterations and I've separated this
> change out from the bulk in the next change to keep the patch size
> down. I could have done a patch switching from lseeks to math, then a
> patch to add write_attrs_after_data. It probably would have yielded
> about 4 lines of shared code, more lines that would have been deleted,
> while creating quite a bit of work for me. Ideally when these
> functions were created there would have been far more liberal use of
> things like immutability, so side-effects are minimized. Yes I could
> refactor everything, but time..

Maybe I'm too naive but can we skip header updates on pipe data?  I'm
curious if this makes sense..

Thanks,
Namhyung


diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index a7c859db2e15..b36f84f29295 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -2341,6 +2341,9 @@ int cmd_inject(int argc, const char **argv)
        if (ret)
                goto out_delete;
 
+       if (data.is_pipe)
+               inject.is_pipe = true;
+
        if (!data.is_pipe && inject.output.is_pipe) {
                ret = perf_header__write_pipe(perf_data__fd(&inject.output));
                if (ret < 0) {


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ