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 16:49:03 +0200
Message-ID: <b24331bb-a8e3-5347-62e5-ed9b630c0601@gmail.com> (raw)
In-Reply-To: <20201029100421.33720af1@gandalf.local.home>



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
> 

  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)
2020-10-29 14:04       ` Steven Rostedt
2020-10-29 14:49         ` Yordan Karadzhov (VMware) [this message]
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=b24331bb-a8e3-5347-62e5-ed9b630c0601@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