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

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

On 29.10.20 г. 16:04 ч., Steven Rostedt wrote:
> On Thu, 29 Oct 2020 12:10:36 +0200
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>>>> +/**
>>>> + * @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?
> 
> 
> Do we need to make the interface "void *" and not just a non defined type
> "struct kshark_data_stream_interface *"?
> 
> Just have:
> 
> struct kshark_data_stream_interface;
> 
> And then reference it without defining it, and have the streams define it.
> If you need this to have inheritance and a bit of polymorphism you can do
> that too :-) That is, if you want this interface to have something common
> among all streams.
> 

Maybe I don't understand your idea very well, but I think what you 
suggest has very different behavior. What I want is the implementation 
of the interface to stay in the same header file (libkshark.h). In the 
future we can add more interfaces but this will be again in the same 
header (libkshark.h).

The readout plugins will include libkshark.h and will have to chose one 
of the available interfaces and implement its methods like this:

static int kshark_tep_stream_init(struct kshark_data_stream *stream,
				  struct tracecmd_input *input)
{
	struct kshark_data_stream_interface *interface;

	stream->interface = interface = calloc(1, sizeof(*interface));
	if (!interface)
		return -ENOMEM;

....

	interface->get_pid = tepdata_get_pid;
	interface->get_task = tepdata_get_task;
	interface->get_event_id = tepdata_get_event_id;

....

Note that the plugins will include libkshark.h but libkshark will never 
include headers from plugins.

Does it make any sense, or maybe I just don't understand your suggestion?

Thanks a lot!
Yordan

> struct kshark_data_stream_interface {
> 	int		type;
> 	int		common_data;
> };
> 
> then local to the file that implements it:
> 
> struct data_stream_interface {
> 	struct kshark_data_stream_interface	kinterface;
> 	int					unique_data;
> };
> 
> 
> {
> 	struct data_stream_interface	*interface;
> 
> 
> 	interface = (struct data_stream_interface *)stream->interface;
> 
> 	if (interface->kinterface.type != my_type)
> 		return (or error);
> 
> 	unique_data = interface->unique_data;
> 
> }
> 
> 
> This is even how the trace events work in the kernel. For example:
> 
> struct trace_entry {
> 	unsigned short		type;
> 	unsigned char		flags;
> 	unsigned char		preempt_count;
> 	int			pid;
> };
> 
> struct kprobe_trace_entry_head {
> 	struct trace_entry	ent;
> 	unsigned long		ip;
> };
> 
> struct kretprobe_trace_entry_head {
> 	struct trace_entry	ent;
> 	unsigned long		func;
> 	unsigned long		ret_ip;
> };
> 
> -- Steve
>