Re: [PATCH v2 07/20] kernel-shark: Add basic methods for Data streams

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 12 Oct 2020 16:35:10 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> +static struct kshark_data_stream *kshark_stream_alloc()
> +{
> +	struct kshark_data_stream *stream;
> +
> +	stream = calloc(1, sizeof(*stream));
> +	if (!stream)
> +		goto fail;
> +
> +	stream->show_task_filter = kshark_hash_id_alloc(KS_FILTER_HASH_NBITS);
> +	stream->hide_task_filter = kshark_hash_id_alloc(KS_FILTER_HASH_NBITS);
> +
> +	stream->show_event_filter = kshark_hash_id_alloc(KS_FILTER_HASH_NBITS);
> +	stream->hide_event_filter = kshark_hash_id_alloc(KS_FILTER_HASH_NBITS);
> +
> +	stream->show_cpu_filter = kshark_hash_id_alloc(KS_FILTER_HASH_NBITS);
> +	stream->hide_cpu_filter = kshark_hash_id_alloc(KS_FILTER_HASH_NBITS);
> +
> +	stream->tasks = kshark_hash_id_alloc(KS_TASK_HASH_NBITS);
> +
> +	if (!stream->show_task_filter ||
> +	    !stream->hide_task_filter ||
> +	    !stream->show_event_filter ||
> +	    !stream->hide_event_filter ||
> +	    !stream->tasks) {
> +		    goto fail;
> +	}
> +
> +	stream->format = KS_INVALIDE_DATA;
> +
> +	return stream;
> +
> + fail:
> +	fprintf(stderr, "Failed to allocate memory for data stream.\n");

I don't think we need the print. Not if this is a library routine. The
calloc failure should set the errno that the caller should be able to
figure out what happened.

> +	if (stream)
> +		kshark_stream_free(stream);
> +
> +	return NULL;
> +}
> +
> +/**
> + * @brief Add new Data stream.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + *
> + * @returns Zero on success or a negative errno code on failure.
> + */
> +int kshark_add_stream(struct kshark_context *kshark_ctx)
> +{
> +	struct kshark_data_stream *stream;
> +
> +	if (kshark_ctx->n_streams == KS_MAX_NUM_STREAMS)
> +		return -EMFILE;
> +
> +	stream = kshark_stream_alloc();

Need to check for success.

> +	stream->stream_id = kshark_ctx->n_streams;
> +
> +	if (pthread_mutex_init(&stream->input_mutex, NULL) != 0) {
> +		kshark_stream_free(stream);
> +		return -EAGAIN;
> +	}
> +
> +	kshark_ctx->stream[kshark_ctx->n_streams++] = stream;
> +
> +	return stream->stream_id;
> +}
> +
> +/**
> + * @brief Get the Data stream object having given Id.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + * @param sd: Data stream identifier.
> + *
> + * @returns Pointer to a Data stream object if the sream exists. Otherwise
> + *	    NULL.
> + */
> +struct kshark_data_stream *
> +kshark_get_data_stream(struct kshark_context *kshark_ctx, int sd)
> +{
> +	if (sd >= 0 && sd < KS_MAX_NUM_STREAMS)
> +		return kshark_ctx->stream[sd];
> +
> +	return NULL;
> +}
> +
> +/**
> + * @brief Get the Data stream object corresponding to a given entry
> + *
> + * @param entry: Input location for the KernelShark entry.
> + *
> + * @returns Pointer to a Data stream object on success. Otherwise NULL.
> + */
> +struct kshark_data_stream *
> +kshark_get_stream_from_entry(const struct kshark_entry *entry)
> +{
> +	struct kshark_context *kshark_ctx = NULL;
> +
> +	if (!kshark_instance(&kshark_ctx))
> +		return NULL;
> +
> +	return kshark_get_data_stream(kshark_ctx, entry->stream_id);
> +}
> +
> +/**
> + * @brief Get an array containing the Ids of all opened Trace data streams.
> + * 	  The User is responsible for freeing the array.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + */
> +int *kshark_all_streams(struct kshark_context *kshark_ctx)
> +{
> +	int *ids, i, count = 0;
> +
> +	ids = malloc(kshark_ctx->n_streams * (sizeof(*ids)));

I think calloc is more appropriate for the above.

	ids = calloc(kshark_ctx->n_streams, sizeof(*ids));

> +	if (!ids) {
> +		fprintf(stderr,
> +			"Failed to allocate memory for stream array.\n");

Probably don't need the print.

> +		return NULL;
> +	}
> +
> +	for (i = 0; i < KS_MAX_NUM_STREAMS; ++i)
> +		if (kshark_ctx->stream[i])
> +			ids[count++] = i;

Definitely need the calloc, as malloc doesn't initialize the array to
zero. Thus some ids[] will not be initialized.

> +
> +	return ids;
> +}
> +
>  /**
>   * @brief Close the trace data file and free the trace data handle.
>   *
> @@ -252,6 +399,56 @@ void kshark_free(struct kshark_context *kshark_ctx)
>  	free(kshark_ctx);
>  }
>  
> +/**
> + * @brief Get the name of the command/task from its Process Id.
> + *
> + * @param sd: Data stream identifier.
> + * @param pid: Process Id of the command/task.
> + */
> +char *kshark_comm_from_pid(int sd, int pid)

I wonder if we should abstract this further, and call it
"kshark_name_from_id()", as comm and pid are specific to processes, and
we may have a stream that will represent something other than processes.

> +{
> +	struct kshark_context *kshark_ctx = NULL;
> +	struct kshark_data_stream *stream;
> +	struct kshark_entry e;
> +
> +	if (!kshark_instance(&kshark_ctx))
> +		return NULL;
> +
> +	stream = kshark_get_data_stream(kshark_ctx, sd);
> +	if (!stream)
> +		return NULL;
> +
> +	e.visible = KS_PLUGIN_UNTOUCHED_MASK;
> +	e.pid = pid;
> +
> +	return stream->interface.get_task(stream, &e);
> +}
> +
> +/**
> + * @brief Get the name of the event from its Id.
> + *
> + * @param sd: Data stream identifier.
> + * @param event_id: The unique Id of the event type.
> + */
> +char *kshark_event_from_id(int sd, int event_id)
> +{
> +	struct kshark_context *kshark_ctx = NULL;
> +	struct kshark_data_stream *stream;
> +	struct kshark_entry e;
> +
> +	if (!kshark_instance(&kshark_ctx))
> +		return NULL;
> +
> +	stream = kshark_get_data_stream(kshark_ctx, sd);
> +	if (!stream)
> +		return NULL;
> +
> +	e.visible = KS_PLUGIN_UNTOUCHED_MASK;
> +	e.event_id = event_id;
> +
> +	return stream->interface.get_event_name(stream, &e);
> +}
> +
>  static struct kshark_task_list *
>  kshark_find_task(struct kshark_context *kshark_ctx, uint32_t key, int pid)
>  {
> diff --git a/src/libkshark.h b/src/libkshark.h
> index fe0ba7f2..e299d067 100644
> --- a/src/libkshark.h
> +++ b/src/libkshark.h
> @@ -340,6 +340,12 @@ struct kshark_task_list {
>  
>  /** Structure representing a kshark session. */
>  struct kshark_context {
> +	/** Array of data stream descriptors. */
> +	struct kshark_data_stream	**stream;
> +
> +	/** The number of data streams. */
> +	int				n_streams;
> +
>  	/** Input handle for the trace data file. */
>  	struct tracecmd_input	*handle;
>  
> @@ -397,6 +403,16 @@ bool kshark_instance(struct kshark_context **kshark_ctx);
>  
>  bool kshark_open(struct kshark_context *kshark_ctx, const char *file);
>  
> +int kshark_add_stream(struct kshark_context *kshark_ctx);
> +
> +struct kshark_data_stream *
> +kshark_get_data_stream(struct kshark_context *kshark_ctx, int sd);
> +
> +struct kshark_data_stream *
> +kshark_get_stream_from_entry(const struct kshark_entry *entry);
> +
> +int *kshark_all_streams(struct kshark_context *kshark_ctx);
> +
>  ssize_t kshark_load_data_entries(struct kshark_context *kshark_ctx,
>  				 struct kshark_entry ***data_rows);
>  
> @@ -416,6 +432,10 @@ void kshark_close(struct kshark_context *kshark_ctx);
>  
>  void kshark_free(struct kshark_context *kshark_ctx);
>  
> +char *kshark_comm_from_pid(int sd, int pid);
> +
> +char *kshark_event_from_id(int sd, int event_id);
> +
>  int kshark_get_pid_easy(struct kshark_entry *entry);
>  
>  const char *kshark_get_task_easy(struct kshark_entry *entry);
> @@ -432,6 +452,157 @@ void kshark_convert_nano(uint64_t time, uint64_t *sec, uint64_t *usec);
>  
>  char* kshark_dump_entry(const struct kshark_entry *entry);
>  
> +static inline int kshark_get_pid(const struct kshark_entry *entry)
> +{
> +	struct kshark_data_stream *stream =
> +		kshark_get_stream_from_entry(entry);
> +
> +	if (!stream)
> +		return -1;
> +
> +	return stream->interface.get_pid(stream, entry);
> +}
> +
> +static inline int kshark_get_event_id(const struct kshark_entry *entry)
> +{
> +	struct kshark_data_stream *stream =
> +		kshark_get_stream_from_entry(entry);
> +
> +	if (!stream)
> +		return -1;
> +
> +	return stream->interface.get_event_id(stream, entry);
> +}
> +
> +static inline int *kshark_get_all_event_ids(struct kshark_data_stream *stream)
> +{
> +	return stream->interface.get_all_event_ids(stream);
> +}
> +
> +static inline char *kshark_get_event_name(const struct kshark_entry *entry)
> +{
> +	struct kshark_data_stream *stream =
> +		kshark_get_stream_from_entry(entry);
> +
> +	if (!stream)
> +		return NULL;
> +
> +	return stream->interface.get_event_name(stream, entry);
> +}
> +
> +static inline char *kshark_get_task(const struct kshark_entry *entry)
> +{
> +	struct kshark_data_stream *stream =
> +		kshark_get_stream_from_entry(entry);
> +
> +	if (!stream)
> +		return NULL;
> +
> +	return stream->interface.get_task(stream, entry);
> +}
> +
> +static inline char *kshark_get_latency(const struct kshark_entry *entry)
> +{
> +	struct kshark_data_stream *stream =
> +		kshark_get_stream_from_entry(entry);
> +
> +	if (!stream)
> +		return NULL;
> +
> +	return stream->interface.get_latency(stream, entry);
> +}
> +
> +static inline char *kshark_get_info(const struct kshark_entry *entry)
> +{
> +	struct kshark_data_stream *stream =
> +		kshark_get_stream_from_entry(entry);
> +
> +	if (!stream)
> +		return NULL;
> +
> +	return stream->interface.get_info(stream, entry);
> +}
> +
> +static inline int kshark_read_event_field(const struct kshark_entry *entry,
> +					  const char* field, int64_t *val)
> +{
> +	struct kshark_data_stream *stream =
> +		kshark_get_stream_from_entry(entry);
> +
> +	if (!stream)
> +		return -1;
> +
> +	return stream->interface.read_event_field_int64(stream, entry,
> +							field, val);
> +}
> +
> +/**
> + * @brief Load the content of the trace data file asociated with a given
> + *	  Data stream identifie into an array of kshark_entries.
> + *	  If one or more filters are set, the "visible" fields of each entry
> + *	  is updated according to the criteria provided by the filters. The
> + *	  field "filter_mask" of the session's context is used to control the
> + *	  level of visibility/invisibility of the filtered entries.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + * @param sd: Data stream identifier.
> + * @param data_rows: Output location for the trace data. The user is
> + *		     responsible for freeing the elements of the outputted
> + *		     array.
> + *
> + * @returns The size of the outputted data in the case of success, or a
> + *	    negative error code on failure.
> + */
> +static inline ssize_t kshark_load_entries(struct kshark_context *kshark_ctx,
> +					  int sd,
> +					  struct kshark_entry ***data_rows)
> +{
> +	struct kshark_data_stream *stream;
> +
> +	stream = kshark_get_data_stream(kshark_ctx, sd);
> +	if (!stream)
> +		return -EBADF;
> +
> +	return stream->interface.load_entries(stream, kshark_ctx, data_rows);
> +}
> +
> +/**
> + * @brief Load the content of the trace data file asociated with a given
> + *	  Data stream identifie into a data matrix. The user is responsible

identifie?

> + *	  for freeing the outputted data.
> + *
> + * @param kshark_ctx: Input location for context pointer.
> + * @param sd: Data stream identifier.
> + * @param cpu_array: Output location for the CPU column (array) of the matrix.
> + * @param event_array: Output location for the Event Id column (array) of the
> + *		       matrix.
> + * @param _array: Output location for the  column (array) of the matrix.
> + * @param offset_array: Output location for the offset column (array) of the
> + *			matrix.
> + * @param ts_array: Output location for the time stamp column (array) of the
> + *		    matrix.

Hmm, how is this matrix composed? How do each realate, via the ts_array?

-- Steve


> + */
> +static inline ssize_t kshark_load_matrix(struct kshark_context *kshark_ctx,
> +					 int sd,
> +					 int16_t **cpu_array,
> +					 int32_t **pid_array,
> +					 int32_t **event_array,
> +					 int64_t **offset_array,
> +					 int64_t **ts_array)
> +{
> +	struct kshark_data_stream *stream;
> +
> +	stream = kshark_get_data_stream(kshark_ctx, sd);
> +	if (!stream)
> +		return -EBADF;
> +
> +	return stream->interface.load_matrix(stream, kshark_ctx, cpu_array,
> +								 pid_array,
> +								 event_array,
> +								 offset_array,
> +								 ts_array);
> +}
> +
>  /**
>   * Custom entry info function type. To be user for dumping info for custom
>   * KernelShark entryes.