On Mon, 14 Jun 2021 10:50:02 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
> When a trace buffer data are written in the trace file, the buffer
> option in the file metadata is updated with the file offset of the
> tracing data. Hide this logic into the trace-cmd library.
> Added new APIs:
> tracecmd_add_buffer_description()
> tracecmd_write_buffers_description()
> Changed APIs:
> tracecmd_append_buffer_cpu_data()
> Removed APIs:
> tracecmd_add_buffer_option()
So I was going to add this patch with some other ones from this series,
but I'm holding off just due to the names.
We are not writing a description, we are only adding name and cpus. We
could change that to tracecmd_add_buffer_info() and tracecmd_write_buffer_info() ?
As "info" is just information about the buffer, and name / cpu is that.
But "description" usually means a bit more in depth information.
Even though the write_buffer_info() could be doing more than one
buffer, I think it still sounds best keeping it singular, as it is
writing the buffer_info section.
>
> This internal refactoring is needed for changes, related to compression
> of the options sections of the trace file.
>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@xxxxxxxxx>
> ---
> .../include/private/trace-cmd-private.h | 8 +-
> lib/trace-cmd/trace-output.c | 84 +++++++++++++++++--
> tracecmd/trace-record.c | 16 ++--
> 3 files changed, 85 insertions(+), 23 deletions(-)
>
> diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
> index ee73325c..cbb578ec 100644
> --- a/lib/trace-cmd/include/private/trace-cmd-private.h
> +++ b/lib/trace-cmd/include/private/trace-cmd-private.h
> @@ -295,8 +295,8 @@ struct tracecmd_option *
> tracecmd_add_option_v(struct tracecmd_output *handle,
> unsigned short id, const struct iovec *vector, int count);
>
> -struct tracecmd_option *tracecmd_add_buffer_option(struct tracecmd_output *handle,
> - const char *name, int cpus);
> +int tracecmd_add_buffer_description(struct tracecmd_output *handle, const char *name, int cpus);
> +int tracecmd_write_buffers_description(struct tracecmd_output *handle);
>
> int tracecmd_write_cpus(struct tracecmd_output *handle, int cpus);
> int tracecmd_write_cmdlines(struct tracecmd_output *handle);
> @@ -312,9 +312,7 @@ int tracecmd_write_cpu_data(struct tracecmd_output *handle,
> int tracecmd_append_cpu_data(struct tracecmd_output *handle,
> int cpus, char * const *cpu_data_files);
> int tracecmd_append_buffer_cpu_data(struct tracecmd_output *handle,
> - struct tracecmd_option *option,
> - int cpus, char * const *cpu_data_files);
> -
> + const char *name, int cpus, char * const *cpu_data_files);
> struct tracecmd_output *tracecmd_get_output_handle_fd(int fd);
>
> /* --- Reading the Fly Recorder Trace --- */
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 7c7d3d76..8f8ca164 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -44,6 +44,14 @@ struct tracecmd_option {
> struct list_head list;
> };
>
> +struct tracecmd_buffer {
> + int cpus;
> + void *name;
> + tsize_t offset;
> + struct tracecmd_option *option;
> + struct list_head list;
> +};
You can add another tab to the above structure to make it look better:
struct tracecmd_buffer {
int cpus;
void *name;
tsize_t offset;
struct tracecmd_option *option;
struct list_head list;
};
-- Steve
> +
> enum {
> OUTPUT_FL_SEND_META = (1 << 0),
> };
> @@ -63,6 +71,7 @@ struct tracecmd_output {
> struct tracecmd_compression *compress;
>
> struct list_head options;
> + struct list_head buffers;
> struct tracecmd_msg_handle *msg_handle;
> char *trace_clock;
> };
> @@ -189,6 +198,7 @@ bool tracecmd_get_quiet(struct tracecmd_output *handle)
> void tracecmd_output_free(struct tracecmd_output *handle)
> {
> struct tracecmd_option *option;
> + struct tracecmd_buffer *buffer;
>
> if (!handle)
> return;
> @@ -199,6 +209,13 @@ void tracecmd_output_free(struct tracecmd_output *handle)
> if (handle->pevent)
> tep_unref(handle->pevent);
>
> + while (!list_empty(&handle->buffers)) {
> + buffer = container_of(handle->buffers.next,
> + struct tracecmd_buffer, list);
> + list_del(&buffer->list);
> + free(buffer->name);
> + free(buffer);
> + }
> while (!list_empty(&handle->options)) {
> option = container_of(handle->options.next,
> struct tracecmd_option, list);
> @@ -1071,6 +1088,7 @@ create_file_fd(int fd, struct tracecmd_input *ihandle,
> goto out_free;
>
> list_head_init(&handle->options);
> + list_head_init(&handle->buffers);
>
> buf[0] = 23;
> buf[1] = 8;
> @@ -1369,9 +1387,8 @@ int tracecmd_append_options(struct tracecmd_output *handle)
> return 0;
> }
>
> -struct tracecmd_option *
> -tracecmd_add_buffer_option(struct tracecmd_output *handle, const char *name,
> - int cpus)
> +static struct tracecmd_option *
> +add_buffer_option(struct tracecmd_output *handle, const char *name, int cpus)
> {
> struct tracecmd_option *option;
> char *buf;
> @@ -1399,6 +1416,53 @@ tracecmd_add_buffer_option(struct tracecmd_output *handle, const char *name,
> return option;
> }
>
> +int tracecmd_add_buffer_description(struct tracecmd_output *handle, const char *name, int cpus)
> +{
> + struct tracecmd_buffer *buf;
> +
> + buf = calloc(1, sizeof(struct tracecmd_buffer));
> + if (!buf)
> + return -1;
> + buf->name = strdup(name);
> + buf->cpus = cpus;
> + if (!buf->name) {
> + free(buf);
> + return -1;
> + }
> + list_add_tail(&buf->list, &handle->buffers);
> + return 0;
> +}
> +
> +int tracecmd_write_buffers_description(struct tracecmd_output *handle)
> +{
> + struct tracecmd_option *option;
> + struct tracecmd_buffer *buf;
> +
> + list_for_each_entry(buf, &handle->buffers, list) {
> + option = add_buffer_option(handle, buf->name, buf->cpus);
> + if (!option)
> + return -1;
> + buf->option = option;
> + }
> + return 0;
> +}
> +
> +static tsize_t get_buffer_file_offset(struct tracecmd_output *handle, const char *name)
> +{
> + struct tracecmd_buffer *buf;
> +
> + list_for_each_entry(buf, &handle->buffers, list) {
> + if (strlen(name) == strlen(buf->name) && !strcmp(name, buf->name)) {
> + if (handle->file_version >= 7)
> + return buf->offset;
> + if (!buf->option)
> + break;
> + return buf->option->offset;
> + }
> + }
> + return 0;
> +}
> +
> int tracecmd_write_cmdlines(struct tracecmd_output *handle)
> {
> int ret;
> @@ -1643,18 +1707,23 @@ int tracecmd_append_cpu_data(struct tracecmd_output *handle,
> }
>
> int tracecmd_append_buffer_cpu_data(struct tracecmd_output *handle,
> - struct tracecmd_option *option,
> - int cpus, char * const *cpu_data_files)
> + const char *name, int cpus, char * const *cpu_data_files)
> {
> + tsize_t b_offset;
> tsize_t offset;
> stsize_t ret;
>
> + b_offset = get_buffer_file_offset(handle, name);
> + if (!b_offset) {
> + tracecmd_warning("Cannot find description for buffer %s\n", name);
> + return -1;
> + }
> offset = lseek64(handle->fd, 0, SEEK_CUR);
>
> /* Go to the option data, where will write the offest */
> - ret = lseek64(handle->fd, option->offset, SEEK_SET);
> + ret = lseek64(handle->fd, b_offset, SEEK_SET);
> if (ret == (off64_t)-1) {
> - tracecmd_warning("could not seek to %lld\n", option->offset);
> + tracecmd_warning("could not seek to %lld\n", b_offset);
> return -1;
> }
>
> @@ -1713,6 +1782,7 @@ struct tracecmd_output *tracecmd_get_output_handle_fd(int fd)
> handle->page_size = tracecmd_page_size(ihandle);
> handle->file_version = tracecmd_get_in_file_version(ihandle);
> list_head_init(&handle->options);
> + list_head_init(&handle->buffers);
>
> if (!tracecmd_get_file_compress_proto(ihandle, &cname, &cver)) {
> handle->compress = tracecmd_compress_alloc(cname, cver, handle->fd,
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index d3362e5b..eff6f2f0 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -4152,7 +4152,6 @@ static void touch_file(const char *file)
> }
>
> static void append_buffer(struct tracecmd_output *handle,
> - struct tracecmd_option *buffer_option,
> struct buffer_instance *instance,
> char **temp_files)
> {
> @@ -4180,7 +4179,7 @@ static void append_buffer(struct tracecmd_output *handle,
> touch_file(temp_files[i]);
> }
>
> - tracecmd_append_buffer_cpu_data(handle, buffer_option,
> + tracecmd_append_buffer_cpu_data(handle, tracefs_instance_get_name(instance->tracefs),
> cpu_count, temp_files);
>
> for (i = 0; i < instance->cpu_count; i++) {
> @@ -4441,7 +4440,6 @@ static void write_guest_file(struct buffer_instance *instance)
>
> static void record_data(struct common_record_context *ctx)
> {
> - struct tracecmd_option **buffer_options;
> struct tracecmd_output *handle;
> struct buffer_instance *instance;
> bool local = false;
> @@ -4512,9 +4510,6 @@ static void record_data(struct common_record_context *ctx)
> }
>
> if (buffers) {
> - buffer_options = malloc(sizeof(*buffer_options) * buffers);
> - if (!buffer_options)
> - die("Failed to allocate buffer options");
> i = 0;
> for_each_instance(instance) {
> int cpus = instance->cpu_count != local_cpu_count ?
> @@ -4522,10 +4517,9 @@ static void record_data(struct common_record_context *ctx)
>
> if (instance->msg_handle)
> continue;
> -
> - buffer_options[i++] = tracecmd_add_buffer_option(handle,
> - tracefs_instance_get_name(instance->tracefs),
> - cpus);
> + tracecmd_add_buffer_description(handle,
> + tracefs_instance_get_name(instance->tracefs),
> + cpus);
> add_buffer_stat(handle, instance);
> }
> }
> @@ -4560,7 +4554,7 @@ static void record_data(struct common_record_context *ctx)
> if (instance->msg_handle)
> continue;
> print_stat(instance);
> - append_buffer(handle, buffer_options[i++], instance, temp_files);
> + append_buffer(handle, instance, temp_files);
> }
> }
>
|