Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v24 03/10] trace-cmd: Add trace-cmd library APIs for ftrace clock name
Date: Wed, 21 Oct 2020 21:26:26 -0400
Message-ID: <20201021212626.1c3436c4@oasis.local.home> (raw)
In-Reply-To: <20201009140338.25260-4-tz.stoyanov@gmail.com>

On Fri,  9 Oct 2020 17:03:31 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> Added enum with ftrace clock IDs and APIs to convert ftrace name to ID
> and vice versa, as part of libtracecmd.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  include/trace-cmd/trace-cmd.h | 16 +++++++++++++
>  lib/trace-cmd/trace-util.c    | 45 +++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index f9c1f843..393a2e7b 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -415,6 +415,22 @@ int tracecmd_msg_recv_time_sync(struct tracecmd_msg_handle *msg_handle,
>  				unsigned int *sync_msg_id,
>  				unsigned int *payload_size, char **payload);
>  
> +enum tracecmd_clocks {
> +	TRACECMD_CLOCK_UNKNOWN	= 0,
> +	TRACECMD_CLOCK_LOCAL	= 1,
> +	TRACECMD_CLOCK_GLOBAL	= 1 << 1,
> +	TRACECMD_CLOCK_COUNTER	= 1 << 2,
> +	TRACECMD_CLOCK_UPTIME	= 1 << 3,
> +	TRACECMD_CLOCK_PERF	= 1 << 4,
> +	TRACECMD_CLOCK_MONO	= 1 << 5,
> +	TRACECMD_CLOCK_MONO_RAW	= 1 << 6,
> +	TRACECMD_CLOCK_BOOT	= 1 << 7,
> +	TRACECMD_CLOCK_X86_TSC	= 1 << 8

I'm curious to why you have this as a bitmask. We can only have on
clock at a time, right?

Also, if we make it a simple counter, we can create a string as well:

#define TRACECMD_CLOCKS \
 C(UNKNOWN,	unknown),	\
 C(LOCAL,	local),	\
 C(GLOBAL,	global),\
 C(COUNTER,	counter),\
 C(UPTIME,	uptime),\
 C(PERF,	perf),	\
 C(MONO,	mono),	\
 C(MONO_RAW,	mono_raw),\
 C(BOOT,	boot,	\
 C(X86_TSC,	x86-tsc)

#undef C
#define C(a, b) TRACECMD_CLOCK_##a

enum tracecmd_clocks { TRACECMD_CLOCKS };

#undef C

> +};
> +
> +enum tracecmd_clocks tracecmd_clock_str2id(const char *clock);
> +char *tracecmd_clock_id2str(enum tracecmd_clocks clock);
> +
>  /* --- Timestamp synchronization --- */
>  
>  enum{
> diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
> index 0ead96ea..e20362e3 100644
> --- a/lib/trace-cmd/trace-util.c
> +++ b/lib/trace-cmd/trace-util.c
> @@ -33,6 +33,51 @@ static bool debug;
>  
>  static FILE *logfp;
>  
> +const static struct {
> +	char *clock_str;
> +	enum tracecmd_clocks clock_id;
> +} trace_clocks[] = {
> +	{"local", TRACECMD_CLOCK_LOCAL},
> +	{"global", TRACECMD_CLOCK_GLOBAL},
> +	{"counter", TRACECMD_CLOCK_COUNTER},
> +	{"uptime", TRACECMD_CLOCK_UPTIME},
> +	{"perf", TRACECMD_CLOCK_PERF},
> +	{"mono", TRACECMD_CLOCK_MONO},
> +	{"mono_raw", TRACECMD_CLOCK_MONO_RAW},
> +	{"boot", TRACECMD_CLOCK_BOOT},
> +	{"x86-tsc", TRACECMD_CLOCK_X86_TSC},
> +	{NULL, -1}
> +};

He we would have:

#define C(a, b) #b
const char * trace_clocks[] = { TRACECMD_CLOCKS }

> +
> +/**
> + * tracecmd_clock_str2id - Convert ftrace clock name to clock ID
> + * @clock: Ftrace clock name
> + * Returns ID of the ftrace clock
> + */
> +enum tracecmd_clocks tracecmd_clock_str2id(const char *clock)
> +{
	int i;

btw, in normal C, it's not recommended to declare a counter in a loop.

	for (i = 0; i < ARRAY_SIZE(trace_clocks); i++) {
		if (strcmp(trace_clocks[i], clock) == 0)
			return i;
	return 0;
}


> +	for (int i = 0; trace_clocks[i].clock_str; i++) {
> +		if (!strncmp(clock, trace_clocks[i].clock_str,
> +		    strlen(trace_clocks[i].clock_str)))
> +			return trace_clocks[i].clock_id;
> +	}
> +	return TRACECMD_CLOCK_UNKNOWN;
> +}
> +
> +/**
> + * tracecmd_clock_id2str - Convert clock ID to ftare clock name
> + * @clock: Clock ID
> + * Returns name of a ftrace clock
> + */
> +char *tracecmd_clock_id2str(enum tracecmd_clocks clock)

Should probably have this return const char *.

Such that callers wont modify it.

> +{

And here we would have:

	if (clock < ARRAY_SIZE(trace_clocks))
		return trace_clocks[i];

	return NULL;

-- Steve

> +	for (int i = 0; trace_clocks[i].clock_str; i++) {
> +		if (trace_clocks[i].clock_id == clock)
> +			return trace_clocks[i].clock_str;
> +	}
> +	return NULL;
> +}
> +
>  /**
>   * tracecmd_set_debug - Set debug mode of the tracecmd library
>   * @set_debug: The new "debug" mode. If true, the tracecmd library is


  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-09 14:03 [PATCH v24 00/10] Timestamp synchronization of host - guest tracing session Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 01/10] trace-cmd: [POC] PTP-like algorithm for host - guest timestamp synchronization Tzvetomir Stoyanov (VMware)
2020-10-15 21:24   ` Steven Rostedt
2020-10-26  7:38     ` Tzvetomir Stoyanov
2020-10-09 14:03 ` [PATCH v24 02/10] trace-cmd: Debug scripts for " Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 03/10] trace-cmd: Add trace-cmd library APIs for ftrace clock name Tzvetomir Stoyanov (VMware)
2020-10-22  1:26   ` Steven Rostedt [this message]
2020-10-22  1:31     ` Steven Rostedt
2020-10-09 14:03 ` [PATCH v24 04/10] trace-cmd: Move VM related logic in a separate file Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 05/10] trace-cmd: Add clock parameter to timestamp synchronization plugins Tzvetomir Stoyanov (VMware)
2020-10-22  1:39   ` Steven Rostedt
2020-10-09 14:03 ` [PATCH v24 06/10] trace-cmd: Add role " Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 07/10] trace-cmd: Add host / guest role in timestamp synchronization context Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 08/10] trace-cmd: Add guest CPU count PID in tracecmd_time_sync struct Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 09/10] trace-cmd: Fixed bitmask logic tracecmd_tsync_proto_getall() Tzvetomir Stoyanov (VMware)
2020-10-09 14:03 ` [PATCH v24 10/10] trace-cmd [POC]: Add KVM timestamp synchronization plugin Tzvetomir Stoyanov (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=20201021212626.1c3436c4@oasis.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=tz.stoyanov@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