Re: [PATCH 2/3] kernel-shark-qt: Consolidate load_data_entries and load_data_records

["Yordan Karadzhov (VMware)" <y.karadz@gmail.com>]

On  3.07.2018 19:21, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> The two functions kshark_load_data_entries() and
> kshark_load_data_records() contain a lot of similar code. Adding helper
> functions can simplify it, and keep bugs from happening in one and not
> the other (bugs will happen in both! but they will be consistent).
> 
> Add some helper functions used by the two functions above.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>   kernel-shark-qt/src/libkshark.c | 232 +++++++++++++++++++-------------
>   1 file changed, 137 insertions(+), 95 deletions(-)
> 
> diff --git a/kernel-shark-qt/src/libkshark.c b/kernel-shark-qt/src/libkshark.c
> index e38ddebbbe41..680949077b7f 100644
> --- a/kernel-shark-qt/src/libkshark.c
> +++ b/kernel-shark-qt/src/libkshark.c
> @@ -500,6 +500,72 @@ static void kshark_set_entry_values(struct kshark_context *kshark_ctx,
>   	entry->pid = pevent_data_pid(kshark_ctx->pevent, record);
>   }
>   
> +struct rec_list {
> +	struct pevent_record	*rec;
> +	struct rec_list		*next;
> +};
> +
> +static void free_rec_list(struct rec_list **rec_list, int n_cpus)
> +{
> +	struct rec_list *temp_rec;
> +	int cpu;
> +
> +	for (cpu = 0; cpu < n_cpus; ++cpu) {
> +		while (rec_list[cpu]) {
> +			temp_rec = rec_list[cpu];
> +			rec_list[cpu] = temp_rec->next;
> +			free(temp_rec);
> +		}
> +	}
> +	free(rec_list);
> +}
> +
> +static size_t get_records(struct kshark_context *kshark_ctx,
> +			  struct rec_list ***rec_list)
> +{
> +	struct pevent_record *rec;
> +	struct rec_list **temp_next;
> +	struct rec_list **cpu_list;
> +	struct rec_list *temp_rec;
> +	size_t count, total = 0;
> +	int n_cpus;
> +	int cpu;
> +
> +	n_cpus = tracecmd_cpus(kshark_ctx->handle);
> +	cpu_list = calloc(n_cpus, sizeof(*cpu_list));
> +	if (!cpu_list)
> +		return -ENOMEM;
> +
> +	for (cpu = 0; cpu < n_cpus; ++cpu) {
> +		count = 0;
> +		cpu_list[cpu] = NULL;
> +		temp_next = &cpu_list[cpu];
> +
> +		rec = tracecmd_read_cpu_first(kshark_ctx->handle, cpu);
> +		while (rec) {
> +			*temp_next = temp_rec = malloc(sizeof(*temp_rec));
> +			if (!temp_rec)
> +				goto fail;
> +
> +			temp_rec->rec = rec;
> +			temp_rec->next = NULL;
> +			temp_next = &temp_rec->next;
> +
> +			++count;
> +			rec = tracecmd_read_data(kshark_ctx->handle, cpu);
> +		}
> +
> +		total += count;
> +	}
> +
> +	*rec_list = cpu_list;
> +	return total;
> +
> + fail:
> +	free_rec_list(cpu_list, n_cpus);
> +	return -ENOMEM;
> +}
> +
>   /**
>    * @brief Load the content of the trace data file into an array of
>    *	  kshark_entries. This function provides fast loading, however the

Actually , after applying the patch for trace-input.c and this patch on 
top kshark_load_data_entries() becomes slower than 
kshark_load_data_records(). Just make this clear in the description of 
the functions.

> @@ -521,9 +587,11 @@ ssize_t kshark_load_data_entries(struct kshark_context *kshark_ctx,
>   				struct kshark_entry ***data_rows)
>   {
>   	struct event_filter *adv_filter = kshark_ctx->advanced_event_filter;
> -	struct kshark_entry **cpu_list, **rows;
> -	struct kshark_entry *entry, **next;
>   	struct kshark_task_list *task;
> +	struct kshark_entry **rows;
> +	struct kshark_entry *entry;
> +	struct rec_list **rec_list;
> +	struct rec_list *temp_rec;
>   	struct pevent_record *rec;
>   	int cpu, n_cpus, next_cpu;
>   	size_t count, total = 0;
> @@ -533,24 +601,41 @@ ssize_t kshark_load_data_entries(struct kshark_context *kshark_ctx,
>   	if (*data_rows)
>   		free(*data_rows);
>   
> +	total = get_records(kshark_ctx, &rec_list);
> +	if (total < 0)
> +		goto fail;
> +
> +	rows = calloc(total, sizeof(struct kshark_entry *));
> +	if(!rows)
> +		goto fail;
> +
>   	n_cpus = tracecmd_cpus(kshark_ctx->handle);
> -	cpu_list = calloc(n_cpus, sizeof(struct kshark_entry *));
>   
> -	for (cpu = 0; cpu < n_cpus; ++cpu) {
> -		count = 0;
> -		cpu_list[cpu] = NULL;
> -		next = &cpu_list[cpu];
> +	for (count = 0; count < total; count++) {
> +		ts = 0;
> +		next_cpu = -1;
> +		for (cpu = 0; cpu < n_cpus; ++cpu) {
> +			if (!rec_list[cpu])
> +				continue;
>   
> -		rec = tracecmd_read_cpu_first(kshark_ctx->handle, cpu);
> -		while (rec) {
> -			*next = entry = malloc(sizeof(struct kshark_entry));
> +			if (!ts || rec_list[cpu]->rec->ts < ts) {
> +				ts = rec_list[cpu]->rec->ts;
> +				next_cpu = cpu;
> +			}
> +		}
> +
> +		if (next_cpu >= 0) {
> +			entry = malloc(sizeof(struct kshark_entry));
>   			if (!entry)
> -				goto fail;
> +				goto fail_free;
> +
> +			rec = rec_list[next_cpu]->rec;
> +			rows[count] = entry;
>   
>   			kshark_set_entry_values(kshark_ctx, rec, entry);
>   			task = kshark_add_task(kshark_ctx, entry->pid);
>   			if (!task)
> -				goto fail;
> +				goto fail_free;
>   
>   			/* Apply event filtering. */
>   			ret = FILTER_NONE;
> @@ -567,47 +652,26 @@ ssize_t kshark_load_data_entries(struct kshark_context *kshark_ctx,
>   				entry->visible &= ~kshark_ctx->filter_mask;
>   			}
>   
> -			entry->next = NULL;
> -			next = &entry->next;
> +			temp_rec = rec_list[next_cpu];
> +			rec_list[next_cpu] = rec_list[next_cpu]->next;
> +			free(temp_rec);
>   			free_record(rec);
> -
> -			++count;
> -			rec = tracecmd_read_data(kshark_ctx->handle, cpu);
> -		}
> -
> -		total += count;
> -	}
> -
> -	rows = calloc(total, sizeof(struct kshark_entry *));
> -	if (!rows)
> -		goto fail;
> -
> -	count = 0;
> -	while (count < total) {
> -		ts = 0;
> -		next_cpu = -1;
> -		for (cpu = 0; cpu < n_cpus; ++cpu) {
> -			if (!cpu_list[cpu])
> -				continue;
> -
> -			if (!ts || cpu_list[cpu]->ts < ts) {
> -				ts = cpu_list[cpu]->ts;
> -				next_cpu = cpu;
> -			}
> -		}
> -
> -		if (next_cpu >= 0) {
> -			rows[count] = cpu_list[next_cpu];
> -			cpu_list[next_cpu] = cpu_list[next_cpu]->next;
>   		}
> -		++count;
>   	}
>   
> -	free(cpu_list);
> +	free_rec_list(rec_list, n_cpus);
>   	*data_rows = rows;
>   	return total;
>   
> -fail:
> + fail_free:
> +	free_rec_list(rec_list, n_cpus);
> +	for (count = 0; count < total; count++) {
> +		if (!rows[count])
> +			break;
> +		free(rows[count]);
> +	}
> +	free(rows);
> + fail:
>   	fprintf(stderr, "Failed to allocate memory during data loading.\n");
>   	return -ENOMEM;
>   }
> @@ -625,82 +689,60 @@ fail:
>   ssize_t kshark_load_data_records(struct kshark_context *kshark_ctx,
>   				struct pevent_record ***data_rows)
>   {
> -	struct temp {
> -		struct pevent_record	*rec;
> -		struct temp		*next;
> -	} **cpu_list, **temp_next, *temp_rec;
> -
>   	struct kshark_task_list *task;
>   	struct pevent_record **rows;
> -	struct pevent_record *data;
> +	struct pevent_record *rec;
> +	struct rec_list **rec_list;
> +	struct rec_list *temp_rec;
>   	int cpu, n_cpus, next_cpu;
>   	size_t count, total = 0;
>   	uint64_t ts;
>   	int pid;
>   
> -	n_cpus = tracecmd_cpus(kshark_ctx->handle);
> -	cpu_list = calloc(n_cpus, sizeof(struct temp *));
> -
> -	for (cpu = 0; cpu < n_cpus; ++cpu) {
> -		count = 0;
> -		cpu_list[cpu] = NULL;
> -		temp_next = &cpu_list[cpu];
> -
> -		data = tracecmd_read_cpu_first(kshark_ctx->handle, cpu);
> -		while (data) {
> -			*temp_next = temp_rec = malloc(sizeof(*temp_rec));
> -			if (!temp_rec)
> -				goto fail;
> -
> -			pid = pevent_data_pid(kshark_ctx->pevent, data);
> -			task = kshark_add_task(kshark_ctx, pid);
> -			if (!task)
> -				goto fail;
> -
> -			temp_rec->rec = data;
> -			temp_rec->next = NULL;
> -			temp_next = &(temp_rec->next);
> -
> -			++count;
> -			data = tracecmd_read_data(kshark_ctx->handle, cpu);
> -		}
> -
> -		total += count;
> -	}
> +	total = get_records(kshark_ctx, &rec_list);
> +	if (total < 0)
> +		goto fail;
>   
>   	rows = calloc(total, sizeof(struct pevent_record *));
> -	if (!rows)
> +	if(!rows)
>   		goto fail;
>   
> -	count = 0;
> -	while (count < total) {
> +	n_cpus = tracecmd_cpus(kshark_ctx->handle);
> +
> +	for (count = 0; count < total; count++) {
>   		ts = 0;
>   		next_cpu = -1;
>   		for (cpu = 0; cpu < n_cpus; ++cpu) {
> -			if (!cpu_list[cpu])
> +			if (!rec_list[cpu])
>   				continue;
>   
> -			if (!ts || cpu_list[cpu]->rec->ts < ts) {
> -				ts = cpu_list[cpu]->rec->ts;
> +			if (!ts || rec_list[cpu]->rec->ts < ts) {
> +				ts = rec_list[cpu]->rec->ts;
>   				next_cpu = cpu;
>   			}
>   		}
>   
>   		if (next_cpu >= 0) {
> -			rows[count] = cpu_list[next_cpu]->rec;
> -			temp_rec = cpu_list[next_cpu];
> -			cpu_list[next_cpu] = cpu_list[next_cpu]->next;
> -			free (temp_rec);
> -		}
> +			rec = rec_list[next_cpu]->rec;
> +			rows[count] = rec;
>   
> -		++count;
> +			pid = pevent_data_pid(kshark_ctx->pevent, rec);
> +			task = kshark_add_task(kshark_ctx, pid);
> +			if (!task)
> +				goto fail;
> +
> +			temp_rec = rec_list[next_cpu];
> +			rec_list[next_cpu] = rec_list[next_cpu]->next;
> +			free(temp_rec);
> +			free_record(rec);

You have to free only the element of the list.
The user is responsible for freeing the record.

> +		}
>   	}
>   
> -	free(cpu_list);
> +	free_rec_list(rec_list, n_cpus);
>   	*data_rows = rows;
>   	return total;
>   
> -fail:
> + fail:
>   	fprintf(stderr, "Failed to allocate memory during data loading.\n");
>   	return -ENOMEM;
>   }
>