| 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
| ||
|
Message-ID: <CAP-5=fURe7yVy6OGWdKn1eSzsdfZPyvvc5fRMPeNAjukaWOe1w@mail.gmail.com>
Date: Thu, 29 Aug 2024 13:12:32 -0700
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>,
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 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:
```
lseek(fd, sizeof(f_header), SEEK_SET);
evlist__for_each_entry(session->evlist, evsel) {
evsel->id_offset = lseek(fd, 0, SEEK_CUR); // Initially at sizeof(f_header)
...
err = do_write(&ff, evsel->core.id, evsel->core.ids *
sizeof(u64)); // offset has advanced by "evsel->core.ids *
sizeof(u64)" bytes
...
}
```
After:
```
u64 attr_offset = sizeof(f_header)
...
else {
lseek(fd, attr_offset, SEEK_SET);
}
evlist__for_each_entry(session->evlist, evsel) {
evsel->id_offset = attr_offset;
...
err = do_write(&ff, evsel->core.id, evsel->core.ids * sizeof(u64));
...
attr_offset += evsel->core.ids * sizeof(u64);
}
```
The reason for using math rather than lseek (apart from reducing
syscalls) is that if we're writing the header at the beginning of
generating a file (usually done more to compute the
header->data_offset), we can't use lseek if writing at the end as we
don't know where the end of the data will be yet and don't write out
the bytes.
Thanks,
Ian
> - Arnaldo
>
> > }
> >
> > - attr_offset = lseek(ff.fd, 0, SEEK_CUR);
> > -
> > evlist__for_each_entry(evlist, evsel) {
> > if (evsel->core.attr.size < sizeof(evsel->core.attr)) {
> > /*
> > @@ -3711,40 +3729,46 @@ static int perf_session__do_write_header(struct perf_session *session,
> > */
> > evsel->core.attr.size = sizeof(evsel->core.attr);
> > }
> > - f_attr = (struct perf_file_attr){
> > - .attr = evsel->core.attr,
> > - .ids = {
> > - .offset = evsel->id_offset,
> > - .size = evsel->core.ids * sizeof(u64),
> > + /* Avoid writing at the end of the file until the session is exiting. */
> > + if (!write_attrs_after_data || at_exit) {
> > + struct perf_file_attr f_attr = {
> > + .attr = evsel->core.attr,
> > + .ids = {
> > + .offset = evsel->id_offset,
> > + .size = evsel->core.ids * sizeof(u64),
> > + }
> > + };
> > + err = do_write(&ff, &f_attr, sizeof(f_attr));
> > + if (err < 0) {
> > + pr_debug("failed to write perf header attribute\n");
> > + goto err_out;
> > }
> > - };
> > - err = do_write(&ff, &f_attr, sizeof(f_attr));
> > - if (err < 0) {
> > - pr_debug("failed to write perf header attribute\n");
> > - free(ff.buf);
> > - return err;
> > }
> > + attr_size += sizeof(struct perf_file_attr);
> > }
> >
> > - if (!header->data_offset)
> > - header->data_offset = lseek(fd, 0, SEEK_CUR);
> > + if (!header->data_offset) {
> > + if (write_attrs_after_data)
> > + header->data_offset = sizeof(f_header);
> > + else
> > + header->data_offset = attr_offset + attr_size;
> > + }
> > header->feat_offset = header->data_offset + header->data_size;
> >
> > - if (at_exit) {
> > + if (!write_attrs_after_data && at_exit) {
> > + /* Write features now feat_offset is known. */
> > err = perf_header__adds_write(header, evlist, fd, fc);
> > - if (err < 0) {
> > - free(ff.buf);
> > - return err;
> > - }
> > + if (err < 0)
> > + goto err_out;
> > }
> >
> > f_header = (struct perf_file_header){
> > .magic = PERF_MAGIC,
> > .size = sizeof(f_header),
> > - .attr_size = sizeof(f_attr),
> > + .attr_size = sizeof(struct perf_file_attr),
> > .attrs = {
> > .offset = attr_offset,
> > - .size = evlist->core.nr_entries * sizeof(f_attr),
> > + .size = attr_size,
> > },
> > .data = {
> > .offset = header->data_offset,
> > @@ -3757,21 +3781,24 @@ static int perf_session__do_write_header(struct perf_session *session,
> >
> > lseek(fd, 0, SEEK_SET);
> > err = do_write(&ff, &f_header, sizeof(f_header));
> > - free(ff.buf);
> > if (err < 0) {
> > pr_debug("failed to write perf header\n");
> > - return err;
> > + goto err_out;
> > + } else {
> > + lseek(fd, 0, SEEK_END);
> > + err = 0;
> > }
> > - lseek(fd, header->data_offset + header->data_size, SEEK_SET);
> > -
> > - return 0;
> > +err_out:
> > + free(ff.buf);
> > + return err;
> > }
> >
> > int perf_session__write_header(struct perf_session *session,
> > struct evlist *evlist,
> > int fd, bool at_exit)
> > {
> > - return perf_session__do_write_header(session, evlist, fd, at_exit, NULL);
> > + return perf_session__do_write_header(session, evlist, fd, at_exit, /*fc=*/NULL,
> > + /*write_attrs_after_data=*/false);
> > }
> >
> > size_t perf_session__data_offset(const struct evlist *evlist)
> > @@ -3793,7 +3820,8 @@ int perf_session__inject_header(struct perf_session *session,
> > int fd,
> > struct feat_copier *fc)
> > {
> > - return perf_session__do_write_header(session, evlist, fd, true, fc);
> > + return perf_session__do_write_header(session, evlist, fd, true, fc,
> > + /*write_attrs_after_data=*/false);
> > }
> >
> > static int perf_header__getbuffer64(struct perf_header *header,
> > --
> > 2.46.0.295.g3b9ea8a38a-goog
Powered by blists - more mailing lists