Re: [PATCH v3 04/20] kernel-shark: Introduce Data streams

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 12 Nov 2020 16:23:42 +0200
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> With the help of Data stream, KernelShark will be able to load and
> merge multiple trace files (streams). Each stream can have different
> plugins or filters, registered for it, which means that the raw trace
> data of the streams can have different formats, and will allow for a
> great degree of customization of the provided data visualization. In
> this patch we only provide the basic definitions. The actual integration
> of the Data streams into the C API of KernelShark will happen in the
> following patches.
> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  src/libkshark.h | 216 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 216 insertions(+)
> 
> diff --git a/src/libkshark.h b/src/libkshark.h
> index cc20077f..c8fdd266 100644
> --- a/src/libkshark.h
> +++ b/src/libkshark.h
> @@ -121,6 +121,222 @@ void kshark_hash_id_free(struct kshark_hash_id *hash);
>  
>  int *kshark_hash_ids(struct kshark_hash_id *hash);
>  
> +struct kshark_data_stream;
> +
> +/** A function type to be used to initialize the interface of the data stream. */
> +typedef int (*interface_init_func) (struct kshark_data_stream *,
> +				    const char *);
> +
> +/** A function type to be used to initialize the interface of the data stream. */
> +typedef int (*interface_close_func) (struct kshark_data_stream *,
> +				     const char *);

These two typedefs should probably be introduced when a patch that uses them
is added. Otherwise they are basically meaningless for the reviewer.

Also, it the above looks like a cut-and-paste error as the close has the
same description as the init.

I don't even see the above used in later patches :-/


> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef char *(*stream_get_str_func) (struct kshark_data_stream *,
> +				      const struct kshark_entry *);
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef const int (*stream_get_int_func) (struct kshark_data_stream *,
> +					  const struct kshark_entry *);
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef int (*stream_find_id_func) (struct kshark_data_stream *,
> +				    const char *);
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef int *(*stream_get_ids_func) (struct kshark_data_stream *);
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef int (*stream_get_names_func) (struct kshark_data_stream *,
> +				      const struct kshark_entry *,
> +				      char ***);
> +
> +/** Event field format identifier. */
> +typedef enum kshark_event_field_format {
> +	/** A field of unknown type. */
> +	KS_INVALIDE_FIELD,

	KS_INVALID_FIELD ?

> +
> +	/** Integer number */
> +	KS_INTEGER_FIELD,
> +
> +	/** Floating-point number */
> +	KS_FLOAT_FIELD
> +} kshark_event_field_format;
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef kshark_event_field_format
> +(*stream_event_field_type) (struct kshark_data_stream *,
> +			    const struct kshark_entry *,
> +			    const char *);
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef const int (*stream_read_event_field) (struct kshark_data_stream *,
> +					      const struct kshark_entry *,
> +					      const char *,
> +					      int64_t *);
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef const int (*stream_read_record_field) (struct kshark_data_stream *,
> +					       void *,
> +					       const char *,
> +					       int64_t *);
> +
> +struct kshark_context;
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef ssize_t (*load_entries_func) (struct kshark_data_stream *,
> +				      struct kshark_context *,
> +				      struct kshark_entry ***);
> +
> +/** A function type to be used by the method interface of the data stream. */
> +typedef ssize_t (*load_matrix_func) (struct kshark_data_stream *,
> +				     struct kshark_context *,
> +				     int16_t **event_array,
> +				     int16_t **cpu_array,
> +				     int32_t **pid_array,
> +				     int64_t **offset_array,
> +				     int64_t **ts_array);
> +
> +/** Data format identifier. */
> +typedef enum kshark_data_format {
> +	/** A data of unknown type. */
> +	KS_INVALIDE_DATA,

	KS_INVALID_DATA ?

> +
> +	/** Ftrace data. */
> +	KS_TEP_DATA,
> +
> +	/** VMware SchedTrace data. */
> +	KS_VMW_ST_DATA,
> +} kshark_data_format;

I wonder if this should be a string value that gets registered? Otherwise
we will have to be the registry of every new stream format added.

This is the approach that Tzvetomir took for protocol ids, because that way
we don't need to keep track of them:

 https://lore.kernel.org/r/20201029111816.247241-2-tz.stoyanov@gmail.com

-- Steve

> +
> +/** Data interface identifier. */
> +typedef enum kshark_data_interface_id {
> +	/** An interface with unknown type. */
> +	KS_INVALIDE_INTERFACE,
> +
> +	/** Generic interface suitable for Ftrace data. */
> +	KS_GENERIC_DATA_INTERFACE,
> +} kshark_data_interface_id;
> +