Re: [PATCH v2 10/20] kernel-shark: Start using data streams

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

On 14.10.20 г. 21:56 ч., Steven Rostedt wrote:
> On Mon, 12 Oct 2020 16:35:13 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> Here we switch to using the trace data readout, provided by the Data
>> stream interface. The actual implementation of the new readout was
>> done in the previous commits.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>> --- a/src/libkshark.c
>> +++ b/src/libkshark.c
>> @@ -19,6 +19,7 @@
>>   
>>   // KernelShark
>>   #include "libkshark.h"
>> +#include "libkshark-tepdata.h"
>>   
>>   static __thread struct trace_seq seq;
>>   
>> @@ -32,6 +33,9 @@ static bool kshark_default_context(struct kshark_context **context)
>>   	if (!kshark_ctx)
>>   		return false;
>>   
>> +	kshark_ctx->stream = calloc(KS_MAX_NUM_STREAMS,
>> +				    sizeof(*kshark_ctx->stream));
>> +
>>   	kshark_ctx->event_handlers = NULL;
>>   	kshark_ctx->collections = NULL;
>>   	kshark_ctx->plugins = NULL;
>> @@ -108,62 +112,28 @@ bool kshark_instance(struct kshark_context **kshark_ctx)
>>   	return true;
>>   }
>>   
>> -static void kshark_free_task_list(struct kshark_context *kshark_ctx)
>> -{
>> -	struct kshark_task_list *task;
>> -	int i;
>> -
>> -	if (!kshark_ctx)
>> -		return;
>> -
>> -	for (i = 0; i < KS_TASK_HASH_SIZE; ++i) {
>> -		while (kshark_ctx->tasks[i]) {
>> -			task = kshark_ctx->tasks[i];
>> -			kshark_ctx->tasks[i] = task->next;
>> -			free(task);
>> -		}
>> -	}
>> -}
>> -
>>   /**
>>    * @brief Open and prepare for reading a trace data file specified by "file".
>> - *	  If the specified file does not exist, or contains no trace data,
>> - *	  the function returns false.
>>    *
>>    * @param kshark_ctx: Input location for context pointer.
>>    * @param file: The file to load.
>>    *
>> - * @returns True on success, or false on failure.
>> + * @returns The Id number of the data stream associated with this file on success.
>> + * 	    Otherwise a negative errno code.
>>    */
>> -bool kshark_open(struct kshark_context *kshark_ctx, const char *file)
>> +int kshark_open(struct kshark_context *kshark_ctx, const char *file)
>>   {
>> -	struct tracecmd_input *handle;
>> -
>> -	kshark_free_task_list(kshark_ctx);
>> -
>> -	handle = tracecmd_open(file);
>> -	if (!handle)
>> -		return false;
>> -
>> -	if (pthread_mutex_init(&kshark_ctx->input_mutex, NULL) != 0) {
>> -		tracecmd_close(handle);
>> -		return false;
>> -	}
>> -
>> -	kshark_ctx->handle = handle;
>> -	kshark_ctx->pevent = tracecmd_get_pevent(handle);
>> +	int sd, rt;
>>   
>> -	kshark_ctx->advanced_event_filter =
>> -		tep_filter_alloc(kshark_ctx->pevent);
>> +	sd = kshark_add_stream(kshark_ctx);
>> +	if (sd < 0)
>> +		return sd;
>>   
>> -	/*
>> -	 * Turn off function trace indent and turn on show parent
>> -	 * if possible.
>> -	 */
>> -	tep_plugin_add_option("ftrace:parent", "1");
>> -	tep_plugin_add_option("ftrace:indent", "0");
>> +	rt = kshark_stream_open(kshark_ctx->stream[sd], file);
>> +	if (rt < 0)
> 
> On failure, we should probably destroy the stream that was created.


Correct, will be fixed in v3.

> 
>> +		return rt;
>>   
>> -	return true;
>> +	return sd;
>>   }
>>   
>>   static void kshark_stream_free(struct kshark_data_stream *stream)
>> @@ -253,6 +223,56 @@ int kshark_add_stream(struct kshark_context *kshark_ctx)
>>   	return stream->stream_id;
>>   }
>>   
>> +static bool is_tep(const char *filename)
>> +{
>> +	/*
>> +	 * TODO: This is very naive. Implement more appropriate check. Ideally
>> +	 * it should be part of the trace-cmd library.
>> +	 */
>> +	char *ext = strrchr(filename, '.');
>> +	return ext && strcmp(ext, ".dat") == 0;
>> +}
>> +
>> +static void set_format(struct kshark_context *kshark_ctx,
>> +		       struct kshark_data_stream *stream,
>> +		       const char *filename)
>> +{
>> +	stream->format = KS_INVALIDE_DATA;
>> +
>> +	if (is_tep(filename)) {
>> +		stream->format = KS_TEP_DATA;
>> +		return;
>> +	}
>> +}
>> +
>> +/**
>> + * @brief Use an existing Trace data stream to open and prepare for reading
>> + *	  a trace data file specified by "file".
>> + *
>> + * @param stream: Input location for a Trace data stream pointer.
>> + * @param file: The file to load.
>> + *
>> + * @returns Zero on success or a negative error code in the case of an errno.
>> + */
>> +int kshark_stream_open(struct kshark_data_stream *stream, const char *file)
>> +{
>> +	struct kshark_context *kshark_ctx = NULL;
>> +
>> +	if (!stream || !kshark_instance(&kshark_ctx))
>> +		return -EFAULT;
>> +
>> +	stream->file = strdup(file);
>> +	set_format(kshark_ctx, stream, file);
>> +
>> +	switch (stream->format) {
>> +	case KS_TEP_DATA:
>> +		return kshark_tep_init_input(stream, file);
>> +
>> +	default:
>> +		return -ENODATA;
>> +	}
>> +}
>> +
>>   /**
>>    * @brief Get the Data stream object having given Id.
>>    *
>> @@ -313,45 +333,76 @@ int *kshark_all_streams(struct kshark_context *kshark_ctx)
>>   	return ids;
>>   }
>>   
>> +static void kshark_stream_close(struct kshark_data_stream *stream)
>> +{
>> +	struct kshark_context *kshark_ctx = NULL;
>> +
>> +	if (!stream || !kshark_instance(&kshark_ctx))
>> +		return;
>> +
>> +	/*
>> +	 * All filters are file specific. Make sure that all Process Ids and
>> +	 * Event Ids from this file are not going to be used with another file.
>> +	 */
>> +	kshark_hash_id_clear(stream->show_task_filter);
>> +	kshark_hash_id_clear(stream->hide_task_filter);
>> +	kshark_hash_id_clear(stream->show_event_filter);
>> +	kshark_hash_id_clear(stream->hide_event_filter);
>> +	kshark_hash_id_clear(stream->show_cpu_filter);
>> +	kshark_hash_id_clear(stream->hide_cpu_filter);
>> +
>> +	switch (stream->format) {
>> +	case KS_TEP_DATA:
>> +		kshark_tep_close_interface(stream);
>> +		break;
>> +
>> +	default:
>> +		break;
>> +	}
>> +
>> +	pthread_mutex_destroy(&stream->input_mutex);
>> +}
>> +
>>   /**
>>    * @brief Close the trace data file and free the trace data handle.
>>    *
>>    * @param kshark_ctx: Input location for the session context pointer.
>> + * @param sd: Data stream identifier.
>>    */
>> -void kshark_close(struct kshark_context *kshark_ctx)
>> +void kshark_close(struct kshark_context *kshark_ctx, int sd)
>>   {
>> -	if (!kshark_ctx || !kshark_ctx->handle)
>> +	struct kshark_data_stream *stream;
>> +
>> +	stream = kshark_get_data_stream(kshark_ctx, sd);
>> +	if (!stream)
>>   		return;
>>   
>> -	/*
>> -	 * All filters are file specific. Make sure that the Pids and Event Ids
>> -	 * from this file are not going to be used with another file.
>> -	 */
>> -	tracecmd_filter_id_clear(kshark_ctx->show_task_filter);
>> -	tracecmd_filter_id_clear(kshark_ctx->hide_task_filter);
>> -	tracecmd_filter_id_clear(kshark_ctx->show_event_filter);
>> -	tracecmd_filter_id_clear(kshark_ctx->hide_event_filter);
>> -	tracecmd_filter_id_clear(kshark_ctx->show_cpu_filter);
>> -	tracecmd_filter_id_clear(kshark_ctx->hide_cpu_filter);
>> -
>> -	if (kshark_ctx->advanced_event_filter) {
>> -		tep_filter_reset(kshark_ctx->advanced_event_filter);
>> -		tep_filter_free(kshark_ctx->advanced_event_filter);
>> -		kshark_ctx->advanced_event_filter = NULL;
>> -	}
>> +	kshark_stream_close(stream);
>> +	kshark_stream_free(stream);
>> +	kshark_ctx->stream[sd] = NULL;
>> +	kshark_ctx->n_streams--;
> 
> So, if you have multiple streams, and you close one that's not the last,
> and then add a new one, this will cause the new one to be overwritten.
> 
> As add_stream has:
> 
> 	kshark_ctx->stream[kshark_ctx->n_streams++] = stream;
> 

I see the problem. This is definitely wrong.

What if in addition to "n_streams" I add another counter called 
"last_stream_added" and initialize this counter to -1?

Then I can add streams like this:

	kshark_ctx->stream[++kshark_ctx->last_stream_added] = stream;
	++kshark_ctx->n_streams;

> You may want to do instead:
> 
> 	kshark_ctx->stream[sd] = NULL;
> 	while (!kshark->streams[kshark_ctx->n_streams - 1])
> 		kshark_ctx->n_streams--;
> 
> You can also add a free store, where you store an index in the stream[sd]
> field of the next free item. Then for adding you have:
> 
> 	if (kshark_ctx->free >= 0) {
> 		sd = kshark_ctx->free;
> 		kshark_ctx->free = (int)kshark_ctx->stream[kshark_ctx->free];
> 	} else {
> 		sd = kshark_ctx->n_streams++;
> 	}
> 
> and on freeing:
> 
> 	kshark_ctx->streams[sd] = (void *)kshark_ctx->free;
> 	kshark_ctx->free = sd;
> 
> Just need to initialize kshark_ctx->free to -1.
> 
> And never decrement n_streams. If you do any loops over the stream, you
> could verify that it is a real stream by:
> 

I really need n_streams to show the true number of active streams 
because the widgets are using this a lot.

The way I loop over the active streams is the following:

	int *stream_ids = kshark_all_streams(kshark_ctx);
	for (i = 0; i < kshark_ctx->n_streams; ++i) {
		stream = kshark_ctx->stream[stream_ids[i]];

		....
	}

	free(stream_ids);

and with the addition of "last_stream_added" kshark_all_streams() will 
look like this:

int *kshark_all_streams(struct kshark_context *kshark_ctx)
{
	int *ids, i, count = 0;

	ids = calloc(kshark_ctx->n_streams, (sizeof(*ids)));
	if (!ids)
		return NULL;

	for (i = 0; i <= kshark_ctx->last_stream_added; ++i)
		if (kshark_ctx->stream[i])
			ids[count++] = i;

	return ids;
}

What do you think?

Thanks a lot!
Yordan

> 	if ((unsigned long)kshark_ctx->streams[sd] > kshark_ctx->n_streams)
> 		/* pointer to a stream */
> 	else
> 		/* a free item. */
> 
> Places like kshark_close() would need the above (if doing a free store).
> 
> -- Steve
> 
> 
>> +}
>> +
>> +/**
>> + * @brief Close all currently open trace data file and free the trace data handle.
>> + *
>> + * @param kshark_ctx: Input location for the session context pointer.
>> + */
>> +void kshark_close_all(struct kshark_context *kshark_ctx)
>> +{
>> +	int i, *stream_ids, n_streams;
>> +
>> +	stream_ids = kshark_all_streams(kshark_ctx);
>>   
>>   	/*
>> -	 * All data collections are file specific. Make sure that collections
>> -	 * from this file are not going to be used with another file.
>> +	 * Get a copy of shark_ctx->n_streams befor you start closing. Be aware
>> +	 * that kshark_close() will decrement shark_ctx->n_streams.
>>   	 */
>> -	kshark_free_collection_list(kshark_ctx->collections);
>> -	kshark_ctx->collections = NULL;
>> -
>> -	tracecmd_close(kshark_ctx->handle);
>> -	kshark_ctx->handle = NULL;
>> -	kshark_ctx->pevent = NULL;
>> +	n_streams = kshark_ctx->n_streams;
>> +	for (i = 0; i < n_streams; ++i)
>> +		kshark_close(kshark_ctx, stream_ids[i]);
>>   
>> -	pthread_mutex_destroy(&kshark_ctx->input_mutex);
>> +	free(stream_ids);
>>   }
>>   
>>   /**