From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ABF80C4338F for ; Thu, 19 Aug 2021 18:54:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7B8AF6109E for ; Thu, 19 Aug 2021 18:54:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231500AbhHSSy5 (ORCPT ); Thu, 19 Aug 2021 14:54:57 -0400 Received: from mail.kernel.org ([198.145.29.99]:41428 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229907AbhHSSy5 (ORCPT ); Thu, 19 Aug 2021 14:54:57 -0400 Received: from oasis.local.home (cpe-66-24-58-225.stny.res.rr.com [66.24.58.225]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 5F950610A0; Thu, 19 Aug 2021 18:54:20 +0000 (UTC) Date: Thu, 19 Aug 2021 14:54:12 -0400 From: Steven Rostedt To: "Tzvetomir Stoyanov (VMware)" Cc: linux-trace-devel@vger.kernel.org Subject: Re: [PATCH v2 48/87] trace-cmd library: Read extended BUFFER option Message-ID: <20210819145412.689ce6b8@oasis.local.home> In-Reply-To: <20210729050959.12263-49-tz.stoyanov@gmail.com> References: <20210729050959.12263-1-tz.stoyanov@gmail.com> <20210729050959.12263-49-tz.stoyanov@gmail.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-trace-devel@vger.kernel.org On Thu, 29 Jul 2021 08:09:20 +0300 "Tzvetomir Stoyanov (VMware)" wrote: > The BUFFER option is extended in trace file version 7. It holds CPU > metadata related to the recorded CPU traces. Also, there is a new > BUFFER_LAT option for describing the latency trace data. A new logic > is implemented for these new options. > In trace file version 7, the top buffer is saved as other buffers in the > file, no special treatment. But saving the top buffer in the list of > buffers in the input handler causes problems. It breaks the legacy logic > of trace-cmd library users, which have special logic for trace buffers > processing. That's why "top_buffer" member is added in the input handler > structure, to hold the top buffer. > > Signed-off-by: Tzvetomir Stoyanov (VMware) > --- > lib/trace-cmd/trace-input.c | 116 ++++++++++++++++++++++++++++++------ > 1 file changed, 98 insertions(+), 18 deletions(-) > > diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c > index e7f97561..57ae535a 100644 > --- a/lib/trace-cmd/trace-input.c > +++ b/lib/trace-cmd/trace-input.c > @@ -74,9 +74,19 @@ struct cpu_data { > int pipe_fd; > }; > > +struct cpu_file_data { > + int cpu; > + unsigned long long offset; > + unsigned long long size; > +}; > + > struct input_buffer_instance { > char *name; > size_t offset; > + char *clock; > + bool latency; > + int cpus; > + struct cpu_file_data *cpu_data; > }; > > struct ts_offset_sample { > @@ -155,6 +165,7 @@ struct tracecmd_input { > char * uname; > char * version; > char * trace_clock; > + struct input_buffer_instance top_buffer; > struct input_buffer_instance *buffers; > int parsing_failures; > struct guest_trace_info *guest; > @@ -2904,6 +2915,80 @@ tracecmd_search_task_map(struct tracecmd_input *handle, > return lib; > } > > +#define save_read_number(R, C) \ > + do { \ > + if ((C) > size) \ > + return -1; \ > + (R) = tep_read_number(handle->pevent, (data + rsize), (C)); \ > + rsize += (C); \ > + size -= (C); \ > + } while (0) > + > +#define save_read_string(R) \ > + do { \ > + if (size < 1) \ > + return -1; \ > + (R) = strdup(data + rsize); \ > + if (!(R)) \ > + return -1; \ > + rsize += (strlen((R)) + 1); \ > + size -= (strlen((R)) + 1); \ > + if (size < 0) \ > + return -1; \ > + } while (0) The above should be inline functions, not macros. Just pass the address of the values you want to modify. > + > +static int handle_buffer_option(struct tracecmd_input *handle, > + unsigned short id, char *data, int size) > +{ > + struct input_buffer_instance *buff; > + unsigned long long offset; > + int rsize = 0; > + char *name; > + int i; > + > + save_read_number(offset, 8); > + save_read_string(name); > + > + if (*name == '\0') { > + /* top buffer */ > + buff = &handle->top_buffer; > + } else { > + buff = realloc(handle->buffers, sizeof(*handle->buffers) * (handle->nr_buffers + 1)); > + if (!buff) { > + free(name); > + return -1; > + } > + handle->buffers = buff; > + handle->nr_buffers++; > + > + buff = &handle->buffers[handle->nr_buffers - 1]; > + } > + memset(buff, 0, sizeof(struct input_buffer_instance)); > + buff->name = name; > + buff->offset = offset; > + if (handle->file_version < 7) > + return 0; > + > + /* file version >= 7 specific data */ > + save_read_string(buff->clock); > + if (id == TRACECMD_OPTION_BUFFER) { > + save_read_number(buff->cpus, 4); > + if (!buff->cpus) > + return 0; > + buff->cpu_data = calloc(buff->cpus, sizeof(struct cpu_file_data)); > + if (!buff->cpu_data) > + return -1; > + for (i = 0; i < buff->cpus; i++) { > + save_read_number(buff->cpu_data[i].cpu, 4); > + save_read_number(buff->cpu_data[i].offset, 8); > + save_read_number(buff->cpu_data[i].size, 8); > + } > + } else { > + buff->latency = true; > + } > + return 0; > +} > + > static int handle_options(struct tracecmd_input *handle) > { > long long offset; > @@ -2911,7 +2996,6 @@ static int handle_options(struct tracecmd_input *handle) > unsigned int size; > unsigned short id, flags; > char *cpustats = NULL; > - struct input_buffer_instance *buffer; > struct hook_list *hook; > bool comperss = false; > char *buf; > @@ -3016,21 +3100,10 @@ static int handle_options(struct tracecmd_input *handle) > handle->cpustats = cpustats; > break; > case TRACECMD_OPTION_BUFFER: > - /* A buffer instance is saved at the end of the file */ > - handle->nr_buffers++; > - handle->buffers = realloc(handle->buffers, > - sizeof(*handle->buffers) * handle->nr_buffers); > - if (!handle->buffers) > - return -ENOMEM; > - buffer = &handle->buffers[handle->nr_buffers - 1]; > - buffer->name = strdup(buf + 8); > - if (!buffer->name) { > - free(handle->buffers); > - handle->buffers = NULL; > - return -ENOMEM; > - } > - offset = *(unsigned long long *)buf; > - buffer->offset = tep_read_number(handle->pevent, &offset, 8); > + case TRACECMD_OPTION_BUFFER_LAT: > + ret = handle_buffer_option(handle, option, buf, size); > + if (ret < 0) > + goto out; > break; > case TRACECMD_OPTION_TRACECLOCK: > if (!handle->ts2secs) > @@ -3825,9 +3898,15 @@ void tracecmd_close(struct tracecmd_input *handle) > handle->sections = handle->sections->next; > free(del_sec); > } > - for (i = 0; i < handle->nr_buffers; i++) > - free(handle->buffers[i].name); > > + free(handle->top_buffer.name); > + free(handle->top_buffer.clock); > + free(handle->top_buffer.cpu_data); > + for (i = 0; i < handle->nr_buffers; i++) { > + free(handle->buffers[i].name); > + free(handle->buffers[i].clock); > + free(handle->buffers[i].cpu_data); > + } The freeing of the buffer should be a function that encapsulates this, where all you need to do is: free_buffer(&handle->top_buffer); for (i = 0; i < handle->nr_buffers; i++) free_buffer(&handle->buffers[i]); -- Steve > free(handle->buffers); > > tracecmd_free_hooks(handle->hooks); > @@ -4272,6 +4351,7 @@ tracecmd_buffer_instance_handle(struct tracecmd_input *handle, int indx) > return NULL; > > *new_handle = *handle; > + memset(&new_handle->top_buffer, 0, sizeof(new_handle->top_buffer)); > new_handle->cpu_data = NULL; > new_handle->nr_buffers = 0; > new_handle->buffers = NULL;