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 v26 10/15] trace-cmd: Add timestamp synchronization per vCPU
Date: Wed, 27 Jan 2021 21:09:40 -0500
Message-ID: <20210127210940.32cdb4f1@oasis.local.home> (raw)
In-Reply-To: <20210121074456.157658-11-tz.stoyanov@gmail.com>

On Thu, 21 Jan 2021 09:44:51 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

  
> -static void tsync_offset_load(struct tracecmd_input *handle, char *buf)
> +#define safe_read(R, C) \
> +	{ if ((C) > size) return -EFAULT; \
> +	 (R) = tep_read_number(tep, buf, (C)); \
> +	 buf += (C); \
> +	 size -= (C); }
> +static int tsync_offset_load(struct tep_handle	*tep,
> +			     struct timesync_offsets *ts_offsets, char *buf, int size)
>  {
> -	struct host_trace_info *host = &handle->host;
> -	long long *buf8 = (long long *)buf;
> +	int start_size = size;
>  	int i, j;
>  
> -	for (i = 0; i < host->ts_samples_count; i++) {
> -		host->ts_samples[i].time = tep_read_number(handle->pevent,
> -							   buf8 + i, 8);
> -		host->ts_samples[i].offset = tep_read_number(handle->pevent,
> -						buf8 + host->ts_samples_count + i, 8);
> -		host->ts_samples[i].scaling = tep_read_number(handle->pevent,
> -						buf8 + (2 * host->ts_samples_count) + i, 8);
> -	}
> -	qsort(host->ts_samples, host->ts_samples_count,
> +	for (i = 0; i < ts_offsets->ts_samples_count; i++)
> +		safe_read(ts_offsets->ts_samples[i].time, 8);
> +	for (i = 0; i < ts_offsets->ts_samples_count; i++)
> +		safe_read(ts_offsets->ts_samples[i].offset, 8);
> +	for (i = 0; i < ts_offsets->ts_samples_count; i++)
> +		safe_read(ts_offsets->ts_samples[i].scaling, 8);

I like to follow the Linux kernel requirements for macros. Which is any
complex macro like the "safe_read" above, needs to be done in a
do { } while (0) loop, as it is safer to use than just brackets. Also,
perhaps even making a macro out of the loop too.

#define safe_read(R, C) 					\
	do {							\
		if (size < (C))					\
			return -EFAULT;				\
		(R) = tep_read_number(tep, buf, (C));		\
		buf += (C);					\
		size -= (C);					\
	} while (0)

#define safe_read_loop(type) 						\
	do {								\
		int i;							\
		for (i = 0; i < ts_offsets->ts_samples_count; i++)	\
			safe_read(ts_offsets->ts_samples[i].type, 8);	\
	} while (0)

Then replace all the above with;

	safe_read_loop(time);
	safe_read_loop(offset);
	safe_read_loop(scaling);


I'll look at this more tomorrow.

-- Steve

> +	qsort(ts_offsets->ts_samples, ts_offsets->ts_samples_count,
>  	      sizeof(struct ts_offset_sample), tsync_offset_cmp);
>  	/* Filter possible samples with equal time */
> -	for (i = 0, j = 0; i < host->ts_samples_count; i++) {
> -		if (i == 0 || host->ts_samples[i].time != host->ts_samples[i-1].time)
> -			host->ts_samples[j++] = host->ts_samples[i];
> +	for (i = 0, j = 0; i < ts_offsets->ts_samples_count; i++) {
> +		if (i == 0 || ts_offsets->ts_samples[i].time != ts_offsets->ts_samples[i-1].time)
> +			ts_offsets->ts_samples[j++] = ts_offsets->ts_samples[i];
> +	}
> +	ts_offsets->ts_samples_count = j;
> +
> +	return start_size - size;
> +}
> +

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-21  7:44 [PATCH v26 00/15] Timestamp synchronization of host - guest tracing session Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 01/15] trace-cmd: Replace time sync protocol ID with string Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 02/15] trace-cmd: Add trace-cmd library APIs for ftrace clock name Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 03/15] trace-cmd: Move VM related logic in a separate file Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 04/15] trace-cmd: Add clock parameter to timestamp synchronization plugins Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 05/15] trace-cmd: Add role " Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 06/15] trace-cmd: Add host / guest role in timestamp synchronization context Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 07/15] trace-cmd: Add guest CPU count PID in tracecmd_time_sync struct Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 08/15] trace-cmd: Add scaling ratio for timestamp correction Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 09/15] trace-cmd: Add time sync protocol flags Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 10/15] trace-cmd: Add timestamp synchronization per vCPU Tzvetomir Stoyanov (VMware)
2021-01-28  2:09   ` Steven Rostedt [this message]
2021-01-21  7:44 ` [PATCH v26 11/15] trace-cmd: Define a macro for packed structures Tzvetomir Stoyanov (VMware)
2021-01-28 22:44   ` Steven Rostedt
2021-01-21  7:44 ` [PATCH v26 12/15] trace-cmd: Add dummy function to initialize timestamp sync logic Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 13/15] trace-cmd: [POC] PTP-like algorithm for host - guest timestamp synchronization Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 14/15] trace-cmd: Debug scripts for " Tzvetomir Stoyanov (VMware)
2021-01-21  7:44 ` [PATCH v26 15/15] 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=20210127210940.32cdb4f1@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