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

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

On 12.11.20 г. 22:50 ч., Steven Rostedt wrote:
> 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 :-/
> 

You are right. This is a dead code left over from previous versions of 
the way input plugins have been loaded.

> 
>> +
>> +/** 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 ?
> 

My idea is this identifier to be used for event fields having type that 
is not supported by the interface. For example strings or arrays.

>> +
>> +	/** 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 ?

this identifier will be used in the case you are trying to open an 
arbitrary file that contains no tracing data (.jpeg for example).
> 
>> +
>> +	/** 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
> 

I wonder how we can avoid name collisions, especially in the case when 
some of the data readout plugins will be proprietary.
Also I don't think we can expect more than a dozen of distinct data 
formats to be supported.

Thanks!
Yordan


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