Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Yordan Karadzhov (VMware)" <y.karadz@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v2 07/20] kernel-shark: Add basic methods for Data streams
Date: Thu, 29 Oct 2020 12:10:36 +0200
Message-ID: <d67dd724-abe1-3696-29f7-75358f98df87@gmail.com> (raw)
In-Reply-To: <20201012201847.1218c974@oasis.local.home>



On 13.10.20 г. 3:18 ч., Steven Rostedt wrote:
> 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;
>> +
>> +
>> +	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.
> 

Agree. Will be fixed in v3.

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

OK. Fix in v3.

> 
>> +	stream->stream_id = kshark_ctx->n_streams;
>> +
>> +	if (pthread_mutex_init(&stream->input_mutex, NULL) != 0) {
>> +		kshark_stream_free(stream);
>> +		return -EAGAIN;

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

Fix in v3.

>> +	if (!ids) {
>> +		fprintf(stderr,
>> +			"Failed to allocate memory for stream array.\n");
> 
> Probably don't need the print.

Fix in v3.

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

This is just a helper function that wraps the corresponding method of 
the interface. In the future we can add a similar helper/wrapper 
function with different name if we have streams that contain no processes.

However, you are actually making a very good point. It may be even 
better if we abstract the interface itself, instead of trying to have 
more abstract method names. We can keep the existing interface of 
methods unchanged, but in the definition of "struct kshark_data_stream" 
make the interface void*

struct kshark_data_stream {
	/** Data stream identifier. */
	uint8_t			stream_id;

....

	/** List of Plugin's Draw handlers. */
	struct kshark_draw_handler		*draw_handlers;

	/**
	 * Abstract interface of methods used to operate over the data
	 * from a given stream. An implementation must be provided.
	 */
	void	*interface;
};

and then the wrapping functions may look like this

char *kshark_comm_from_pid(int sd, int pid)
{
	struct kshark_data_stream_interface *interface;
	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;

	interface = stream->interface;
	if (!interface || !interface->get_task)
		return NULL;

	e.visible = KS_PLUGIN_UNTOUCHED_MASK;
	e.pid = pid;

	return interface->get_task(stream, &e);
}

And in the future we can add more interface implementations and more 
helper functions.

What do you think?

Thanks!
Yordan



>> +{
>> +	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.
> 

  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) [this message]
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
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=d67dd724-abe1-3696-29f7-75358f98df87@gmail.com \
    --to=y.karadz@gmail.com \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    /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