Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: "Yordan Karadzhov (VMware)" <y.karadz@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v2 10/20] kernel-shark: Start using data streams
Date: Wed, 14 Oct 2020 14:56:27 -0400
Message-ID: <20201014145627.675e3c70@gandalf.local.home> (raw)
In-Reply-To: <20201012133523.469040-11-y.karadz@gmail.com>

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.

> +		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;

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:

	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);
>  }
>  
>  /**

  reply index

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-12 13:35 [PATCH v2 00/20] Start KernelShark v2 transformation Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 01/20] kernel-shark: Start introducing KernelShark 2.0 Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 02/20] kernel-shark: Use only signed types in kshark_entry Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 03/20] kernel-shark: Introduce libkshark-hash Yordan Karadzhov (VMware)
2020-10-12 14:05   ` Steven Rostedt
2020-10-12 14:05     ` Steven Rostedt
2020-10-12 14:18       ` Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 04/20] kernel-shark: Introduce Data streams Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 05/20] kernel-shark: Add stream_id to kshark_entry Yordan Karadzhov (VMware)
2020-10-13  0:05   ` Steven Rostedt
2020-10-29 10:08     ` Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 06/20] kernel-shark: Rename static methods in libkshark Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 07/20] kernel-shark: Add basic methods for Data streams Yordan Karadzhov (VMware)
2020-10-13  0:18   ` Steven Rostedt
2020-10-29 10:10     ` Yordan Karadzhov (VMware)
2020-10-29 14:04       ` Steven Rostedt
2020-10-29 14:49         ` Yordan Karadzhov (VMware)
2020-10-30  1:57           ` Steven Rostedt
2020-11-03 13:38             ` Yordan Karadzhov (VMware)
2020-11-04 15:41               ` Steven Rostedt
2020-11-05 14:35                 ` Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 08/20] kernel-shark: Housekeeping before implementing stream interface Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 09/20] kernel-shark: Add stream interface for trace-cmd data Yordan Karadzhov (VMware)
2020-10-13  0:44   ` Steven Rostedt
2020-10-29 11:16     ` Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 10/20] kernel-shark: Start using data streams Yordan Karadzhov (VMware)
2020-10-14 18:56   ` Steven Rostedt [this message]
2020-11-05 14:58     ` Yordan Karadzhov (VMware)
2020-11-05 18:17       ` Steven Rostedt
2020-11-06 14:31         ` Yordan Karadzhov (VMware)
2020-11-06 15:18           ` Steven Rostedt
2020-11-09 14:49             ` Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 11/20] kernel-shark: Remove dead code Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 12/20] kernel-shark: Redesign the plugin interface Yordan Karadzhov (VMware)
2020-10-14 21:09   ` Steven Rostedt
2020-10-12 13:35 ` [PATCH v2 13/20] kernel-shark: Complete the stream integration Yordan Karadzhov (VMware)
2020-10-14 23:52   ` Steven Rostedt
2020-10-12 13:35 ` [PATCH v2 14/20] kernel-shark: Provide merging of multiple data streams Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 15/20] kernel-shark: Integrate the stream definitions with data model Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 16/20] kernel-shark: Use only signed types for model defs Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 17/20] kernel-shark: Add ksmodel_get_bin() Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 18/20] kernel-shark: Protect ksmodel_set_in_range_bining() Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 19/20] kernel-shark: Add methods for time calibration Yordan Karadzhov (VMware)
2020-10-12 13:35 ` [PATCH v2 20/20] kernel-shark: Integrate streams with libkshark-configio Yordan Karadzhov (VMware)
2020-11-05 19:22   ` Steven Rostedt
2020-11-09 14:55     ` Yordan Karadzhov (VMware)
2020-11-09 15:28       ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201014145627.675e3c70@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=y.karadz@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git