On 29.05.19 г. 20:36 ч., Steven Rostedt wrote:
On Thu, 23 May 2019 18:18:08 +0300 Yordan Karadzhov <ykaradzhov@xxxxxxxxxx> wrote:The new function loads the content of the trace data file into a table / matrix, made of columns / arrays of data having various integer types. Later those arrays will be wrapped as NumPy arrays. Signed-off-by: Yordan Karadzhov <ykaradzhov@xxxxxxxxxx> --- kernel-shark/src/libkshark.c | 155 +++++++++++++++++++++++++++++++++++ kernel-shark/src/libkshark.h | 7 ++ 2 files changed, 162 insertions(+) diff --git a/kernel-shark/src/libkshark.c b/kernel-shark/src/libkshark.c index 175279c..ac634fd 100644 --- a/kernel-shark/src/libkshark.c +++ b/kernel-shark/src/libkshark.c @@ -957,6 +957,161 @@ ssize_t kshark_load_data_records(struct kshark_context *kshark_ctx, return -ENOMEM; }+static bool data_matrix_alloc(size_t n_rows, uint64_t **offset_array,+ uint8_t **cpu_array, + uint64_t **ts_array, + uint16_t **pid_array, + int **event_array) +{ + if (offset_array) + *offset_array = NULL; + + if (cpu_array) + *cpu_array = NULL; + + if (ts_array) + *ts_array = NULL; + + if (pid_array) + *pid_array = NULL; + + if (event_array) + *event_array = NULL; + + if (offset_array) {The way you can do this, is remove the above and have:+ *offset_array = calloc(n_rows, sizeof(**offset_array)); + if (!*offset_array)return false;+ goto free_all; + } + + if (cpu_array) { + *cpu_array = calloc(n_rows, sizeof(**cpu_array)); + if (!*cpu_array)goto free_offset;+ goto free_all; + } + + if (ts_array) { + *ts_array = calloc(n_rows, sizeof(**ts_array)); + if (!*ts_array)goto free_cpu;+ goto free_all; + } + + if (pid_array) { + *pid_array = calloc(n_rows, sizeof(**pid_array)); + if (!*pid_array)goto free_ts;+ goto free_all; + } + + if (event_array) { + *event_array = calloc(n_rows, sizeof(**event_array)); + if (!*event_array)goto free_pid;+ goto free_all; + } + + return true; +We can have a helper function: static inline void free_ptr(void *ptr) { if (ptr) free(*(void **)ptr); } free_pid: free_ptr(pid_array); free_ts: free_ptr(ts_array); free_cpu: free_ptr(cpu_array); free_offset: free_ptr(offset_array); Then have the print here.+ free_all: + fprintf(stderr, "Failed to allocate memory during data loading.\n");return false; This is the way it's usually done in the Linux kernel.
Great! Really elegant solution.
+ + if (offset_array) + free(*offset_array); + + if (cpu_array) + free(*cpu_array); + + if (ts_array) + free(*ts_array); + + if (pid_array) + free(*pid_array); + + if (event_array) + free(*event_array); + + return false; +} + +/** + * @brief Load the content of the trace data file into a table / matrix made + * of columns / arrays of data. The user is responsible for freeing the + * elements of the outputted array + * + * @param kshark_ctx: Input location for the session context pointer. + * @param offset_array: Output location for the array of record offsets. + * @param cpu_array: Output location for the array of CPU Ids. + * @param ts_array: Output location for the array of timestamps. + * @param pid_array: Output location for the array of Process Ids. + * @param event_array: Output location for the array of Event Ids. + * + * @returns The size of the outputted arrays in the case of success, or a + * negative error code on failure. + */ +size_t kshark_load_data_matrix(struct kshark_context *kshark_ctx, + uint64_t **offset_array, + uint8_t **cpu_array, + uint64_t **ts_array, + uint16_t **pid_array, + int **event_array) +{ + enum rec_type type = REC_ENTRY; + struct rec_list **rec_list; + size_t count, total = 0; + bool status; + int n_cpus; + + total = get_records(kshark_ctx, &rec_list, type); + if (total < 0) + goto fail; + + n_cpus = tracecmd_cpus(kshark_ctx->handle); + + status = data_matrix_alloc(total, offset_array, + cpu_array, + ts_array, + pid_array, + event_array);BTW, have you looked into how much memory this takes up in a large trace?
One record takes 24 bytes. So it will allocate total*24 bytes (or less if some of the input arguments is NULL).
+ if (!status) + goto fail_free; + + for (count = 0; count < total; count++) { + int next_cpu; + + next_cpu = pick_next_cpu(rec_list, n_cpus, type); + if (next_cpu >= 0) { + struct kshark_entry *e = &rec_list[next_cpu]->entry;Hmm, this looks like we are taking an address of a field and then freeing it down below. Looking at the definition of rec_list, this is currently OK. But this coding style is not robust because of the tight dependency to how rec_list is defined. If that ever changes it will be hard to find code like this to update it. A more robust way to do this is: struct rec_list *rec = rec_list[next_cpu]; struct kshark_entry *e = &rec->entry;+ + if (offset_array) + (*offset_array)[count] = e->offset; + + if (cpu_array) + (*cpu_array)[count] = e->cpu; + + if (ts_array) + (*ts_array)[count] = e->ts; + + if (pid_array) + (*pid_array)[count] = e->pid; + + if (event_array) + (*event_array)[count] = e->event_id; + + rec_list[next_cpu] = rec_list[next_cpu]->next;Then here: free(rec); That way there's not a dependency here with the data structure layout of rec_list and the freeing of the entry. -- Steve
Thanks a lot! Sending updated version. Y.
+ free(e); + } + } + + /* There should be no entries left in rec_list. */ + free_rec_list(rec_list, n_cpus, type); + return total; + + fail_free: + free_rec_list(rec_list, n_cpus, type); + + fail: + fprintf(stderr, "Failed to allocate memory during data loading.\n"); + return -ENOMEM; +} + static const char *kshark_get_latency(struct tep_handle *pe, struct tep_record *record) { diff --git a/kernel-shark/src/libkshark.h b/kernel-shark/src/libkshark.h index c218b61..92ade41 100644 --- a/kernel-shark/src/libkshark.h +++ b/kernel-shark/src/libkshark.h @@ -149,6 +149,13 @@ ssize_t kshark_load_data_entries(struct kshark_context *kshark_ctx, ssize_t kshark_load_data_records(struct kshark_context *kshark_ctx, struct tep_record ***data_rows);+size_t kshark_load_data_matrix(struct kshark_context *kshark_ctx,+ uint64_t **offset_array, + uint8_t **cpu_array, + uint64_t **ts_array, + uint16_t **pid_array, + int **event_array); + ssize_t kshark_get_task_pids(struct kshark_context *kshark_ctx, int **pids); void kshark_close(struct kshark_context *kshark_ctx);
|