Re: [PATCH v1 5/8] perf header: Allow attributes to be written after data

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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@google.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 :-\

- 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