Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: "Yordan Karadzhov (VMware)" <y.karadz@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH 08/15] kernel-shark: Integrate the stream definitions with the C API
Date: Wed, 7 Oct 2020 16:29:25 -0400
Message-ID: <20201007162925.1b9d2b30@gandalf.local.home> (raw)
In-Reply-To: <20200929134123.178688-9-y.karadz@gmail.com>

On Tue, 29 Sep 2020 16:41:16 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:


> diff --git a/examples/datafilter.c b/examples/datafilter.c
> index bebc181..38afab8 100644
> --- a/examples/datafilter.c
> +++ b/examples/datafilter.c
> @@ -7,22 +7,22 @@
>  // C
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <string.h>
>  
>  // KernelShark
>  #include "libkshark.h"
> +#include "libkshark-tepdata.h"
>  
>  const char *default_file = "trace.dat";
>  
>  int main(int argc, char **argv)
>  {
> -	ssize_t i, n_rows, n_tasks, n_evts, count;
> +	size_t i, sd, n_rows, n_tasks, n_evts, count;
>  	struct kshark_context *kshark_ctx;
> +	struct kshark_data_stream *stream;
>  	struct kshark_entry **data = NULL;
> -	struct tep_event_filter *adv_filter;
> -	struct tep_event *event;
> +	int *pids, *evt_ids;
>  	char *entry_str;
> -	bool status;
> -	int *pids;
>  
>  	/* Create a new kshark session. */
>  	kshark_ctx = NULL;
> @@ -31,32 +31,30 @@ int main(int argc, char **argv)
>  
>  	/* Open a trace data file produced by trace-cmd. */
>  	if (argc > 1)
> -		status = kshark_open(kshark_ctx, argv[1]);
> +		sd = kshark_open(kshark_ctx, argv[1]);
>  	else
> -		status = kshark_open(kshark_ctx, default_file);
> +		sd = kshark_open(kshark_ctx, default_file);

OK, here's a perfect example of how I would start breaking this up.

I would implement the switch to using this integer, and add it to all the
current old APIs (don't worry about adding new ones with this change).


>  
> -	if (!status) {
> +	if (sd < 0) {
>  		kshark_free(kshark_ctx);
>  		return 1;
>  	}
>  
>  	/* Load the content of the file into an array of entries. */
> -	n_rows = kshark_load_data_entries(kshark_ctx, &data);
> -	if (n_rows < 1) {
> -		kshark_free(kshark_ctx);
> -		return 1;
> -	}



> diff --git a/src/libkshark.c b/src/libkshark.c
> index 92e2450..3a988df 100644
> --- a/src/libkshark.c
> +++ b/src/libkshark.c
> @@ -1,26 +1,28 @@
>  // SPDX-License-Identifier: LGPL-2.1
>  
>  /*
> - * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <y.karadz@gmail.com>
> + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>   */
>  
>   /**
>   *  @file    libkshark.c
> - *  @brief   API for processing of FTRACE (trace-cmd) data.
> + *  @brief   API for processing of traceing data.
>   */
>  
> +#ifndef _GNU_SOURCE
>  /** Use GNU C Library. */
> -#define _GNU_SOURCE 1
> +#define _GNU_SOURCE
> +#endif // _GNU_SOURCE
>  
>  // C
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <assert.h>
> +#include <string.h>
>  
>  // KernelShark
>  #include "libkshark.h"
> -
> -static __thread struct trace_seq seq;
> +#include "libkshark-tepdata.h"
>  
>  static struct kshark_context *kshark_context_handler = NULL;
>  
> @@ -32,18 +34,11 @@ static bool kshark_default_context(struct kshark_context **context)
>  	if (!kshark_ctx)
>  		return false;
>  
> -	kshark_ctx->event_handlers = NULL;
> -	kshark_ctx->collections = NULL;
> -	kshark_ctx->plugins = NULL;
> -
> -	kshark_ctx->show_task_filter = tracecmd_filter_id_hash_alloc();
> -	kshark_ctx->hide_task_filter = tracecmd_filter_id_hash_alloc();
> -
> -	kshark_ctx->show_event_filter = tracecmd_filter_id_hash_alloc();
> -	kshark_ctx->hide_event_filter = tracecmd_filter_id_hash_alloc();
> +	kshark_ctx->stream = calloc(KS_MAX_NUM_STREAMS,
> +				    sizeof(*kshark_ctx->stream));
>  
> -	kshark_ctx->show_cpu_filter = tracecmd_filter_id_hash_alloc();
> -	kshark_ctx->hide_cpu_filter = tracecmd_filter_id_hash_alloc();
> +	kshark_ctx->collections = NULL;
> +	kshark_ctx->inputs = NULL;
>  
>  	kshark_ctx->filter_mask = 0x0;
>  
> @@ -58,14 +53,6 @@ static bool kshark_default_context(struct kshark_context **context)
>  	return true;
>  }
>  
> -static bool init_thread_seq(void)
> -{
> -	if (!seq.buffer)
> -		trace_seq_init(&seq);
> -
> -	return seq.buffer != NULL;
> -}
> -
>  /**
>   * @brief Initialize a kshark session. This function must be called before
>   *	  calling any other kshark function. If the session has been
> @@ -102,1466 +89,809 @@ bool kshark_instance(struct kshark_context **kshark_ctx)
>  		}
>  	}
>  
> -	if (!init_thread_seq())
> -		return false;
> -
>  	return true;
>  }
>  
> -static void kshark_free_task_list(struct kshark_context *kshark_ctx)
> -{
> -	struct kshark_task_list *task;
> -	int i;
> -
> -	if (!kshark_ctx)
> -		return;
> -
> -	for (i = 0; i < KS_TASK_HASH_SIZE; ++i) {
> -		while (kshark_ctx->tasks[i]) {
> -			task = kshark_ctx->tasks[i];
> -			kshark_ctx->tasks[i] = task->next;
> -			free(task);
> -		}
> -	}
> -}
> -
>  /**
>   * @brief Open and prepare for reading a trace data file specified by "file".
> - *	  If the specified file does not exist, or contains no trace data,
> - *	  the function returns false.
>   *
>   * @param kshark_ctx: Input location for context pointer.
>   * @param file: The file to load.
>   *
> - * @returns True on success, or false on failure.
> + * @returns The Id number of the data stream associated with this file on success.
> + * 	    Otherwise a negative errno code.
>   */
> -bool kshark_open(struct kshark_context *kshark_ctx, const char *file)
> +int kshark_open(struct kshark_context *kshark_ctx, const char *file)
>  {
> -	struct tracecmd_input *handle;
> -
> -	kshark_free_task_list(kshark_ctx);
> -
> -	handle = tracecmd_open(file);
> -	if (!handle)
> -		return false;
> +	int sd, rt;
>  
> -	if (pthread_mutex_init(&kshark_ctx->input_mutex, NULL) != 0) {
> -		tracecmd_close(handle);
> -		return false;
> -	}
> +	sd = kshark_add_stream(kshark_ctx);
> +	if (sd < 0)
> +		return sd;

So what this can do is add the stream, but have the kshark_data_stream have
a tracecmd_handle in its field. And then you can still have:

	stream->handle = tracecmd_open(...);


This way, you can pass around the stream index in this step, but everything
still requiring to use the handle.

Then the next step would be to start abstracting each operation a little
more, until everything can use the stream directly. And then you remove the
handle.

This is what I mean by slowly moving to a new API. The in between steps may
not look like either API. But that's OK.

And it makes it much easier to review. Because honestly, this current
approach isn't much better than just squashing this all into one patch, and
hoping it works.

-- Steve


  parent reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-29 13:41 [PATCH 00/15] Start KernelShark v2 transformation Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 01/15] kernel-shark: split kernel-shark from trace-cmd repo Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 02/15] kernel-shark: Version 1.2.0 Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 03/15] kernel-shark: Start introducing KernelShark 2.0 Yordan Karadzhov (VMware)
2020-10-07 20:08   ` Steven Rostedt
2020-09-29 13:41 ` [PATCH 04/15] kernel-shark: Use only signed types in kshark_entry Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 05/15] kernel-shark: Introduce libkshark-hash Yordan Karadzhov (VMware)
2020-10-06 21:02   ` Steven Rostedt
2020-09-29 13:41 ` [PATCH 06/15] kernel-shark: Introduce Data streams Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 07/15] kernel-shark: Add stream_id to kshark_entry Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 08/15] kernel-shark: Integrate the stream definitions with the C API Yordan Karadzhov (VMware)
2020-10-07 20:12   ` Steven Rostedt
2020-10-08  7:17     ` Yordan Karadzhov (VMware)
2020-10-07 20:29   ` Steven Rostedt [this message]
2020-09-29 13:41 ` [PATCH 09/15] kernel-shark: Provide merging of multiple data streams Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 10/15] kernel-shark: Integrate the stream definitions with data model Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 11/15] kernel-shark: Use only signed types for model defs Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 12/15] kernel-shark: Add ksmodel_get_bin() Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 13/15] kernel-shark: Protect ksmodel_set_in_range_bining() Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 14/15] kernel-shark: Add methods for time calibration Yordan Karadzhov (VMware)
2020-09-29 13:41 ` [PATCH 15/15] kernel-shark: Integrate streams with libkshark-configio Yordan Karadzhov (VMware)

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=20201007162925.1b9d2b30@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=y.karadz@gmail.com \
    /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