[PATCH] oslat: Add command line option for bucket width

[PATCH] oslat: Add command line option for bucket width

[Crystal Wood]

New option -W/--bucket-width allows the user to specify how large of a
range of latencies is covered by a single bucket, including allowing the
creation of sub-microsecond buckets.

When the flag is not used, output should be unchanged.  However, if a
bucket width is specified that is not a multiple of one microsecond,
latencies will be output as fractional microseconds, at nanosecond
precision.  This includes JSON output.

When using this option, it is up to the user to determine what level
of precision is meaningful relative to measurement error, as is noted
in the documentation.

Signed-off-by: Crystal Wood <swood@redhat.com>
---
I'm hoping that this output format is acceptable, including JSON,
though I'm not familiar with how the latter is typically consumed.

Another option would be to simplify by not making the output precision
conditional (probably keeping the decimal for readability's sake, at
least in the non-JSON output) but I didn't want to risk compatibility
or user surprise issues.
---
 src/oslat/oslat.8 |   9 +++-
 src/oslat/oslat.c | 105 +++++++++++++++++++++++++++++++---------------
 2 files changed, 80 insertions(+), 34 deletions(-)

diff --git a/src/oslat/oslat.8 b/src/oslat/oslat.8
index 39b36df0db3f..eb96448bfff1 100644
--- a/src/oslat/oslat.8
+++ b/src/oslat/oslat.8
@@ -7,7 +7,7 @@ oslat \- OS Latency Detector
 .RI "[ \-shvz ] [ \-b " bucket-size " ] [ \-B " bias " ] [ \-c " cpu-list " ] \
 [ \-C " cpu-main-thread " ] [ \-f " rt-prio " ] [ \-\-json " filename " ] \
 [ \-m " workload-mem " ] [\-t " runtime " ] [ \-T " trace-threshold " ] \
-[ \-w " workload " ]"
+[ \-w " workload " ] [ \-W " bucket-width " ]"
 .SH DESCRIPTION
 .B oslat
 is an open source userspace polling mode stress program to detect OS level
@@ -57,6 +57,13 @@ NOTE: please make sure the CPU frequency on all testing cores
 are locked before using this parmater.  If you don't know how
 to lock the freq then please don't use this parameter.
 .TP
+.B \-W, \-\-bucket-width
+Interval between buckets in nanoseconds
+
+NOTE: Widths not a multiple of 1000 cause ns-precision output
+You are responsible for considering the impact of measurement
+overhead at the nanosecond scale.
+.TP
 .B \-h, \-\-help
 Show the help message.
 .TP
diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
index 55302f11986b..e8a642699cc7 100644
--- a/src/oslat/oslat.c
+++ b/src/oslat/oslat.c
@@ -192,6 +192,9 @@ struct global {
 	struct timeval        tv_start;
 	int                   rtprio;
 	int                   bucket_size;
+	int                   bucket_width;
+	int                   unit_per_us;
+	int                   precision;
 	int                   trace_threshold;
 	int                   runtime;
 	/* The core that we run the main thread.  Default is cpu0 */
@@ -325,45 +328,46 @@ static float cycles_to_sec(const struct thread *t, uint64_t cycles)
 
 static void insert_bucket(struct thread *t, stamp_t value)
 {
-	int index, us;
+	int index;
+	unsigned int lat;
 	uint64_t extra;
+	double us;
 
-	index = value / t->counter_mhz;
-	assert(index >= 0);
-	us = index + 1;
-	assert(us > 0);
-
+	lat = (value * g.unit_per_us + t->counter_mhz - 1) / t->counter_mhz;
+	us = (double)lat / g.unit_per_us;
 	if (!g.preheat && g.trace_threshold && us >= g.trace_threshold) {
-		char *line = "%s: Trace threshold (%d us) triggered on cpu %d with %u us!\n"
+		char *line = "%s: Trace threshold (%d us) triggered on cpu %d with %.*f us!\n"
 		    "Stopping the test.\n";
-		tracemark(line, g.app_name, g.trace_threshold, t->core_i, us);
-		err_quit(line, g.app_name, g.trace_threshold, t->core_i, us);
+		tracemark(line, g.app_name, g.trace_threshold, t->core_i,
+		          g.precision, us);
+		err_quit(line, g.app_name, g.trace_threshold, t->core_i,
+		         g.precision, us);
 	}
 
 	/* Update max latency */
-	if (us > t->maxlat)
-		t->maxlat = us;
+	if (lat > t->maxlat)
+		t->maxlat = lat;
 
-	if (us < t->minlat)
-		t->minlat = us;
+	if (lat < t->minlat)
+		t->minlat = lat;
 
 	if (g.bias) {
 		/* t->bias will be set after pre-heat if user enabled it */
-		us -= g.bias;
+		lat -= g.bias;
 		/*
 		 * Negative should hardly happen, but if it happens, we assume we're in
-		 * the smallest bucket, which is 1us.  Same to index.
+		 * the smallest bucket.
 		 */
-		if (us <= 0)
-			us = 1;
-		index -= g.bias;
-		if (index < 0)
-			index = 0;
+		if (lat <= 0)
+			lat = 1;
 	}
 
+	index = lat / g.bucket_width;
+	assert(index >= 0);
+
 	/* Too big the jitter; put into the last bucket */
 	if (index >= g.bucket_size) {
-		/* Keep the extra bit (in us) */
+		/* Keep the extra bit (in bucket width multiples) */
 		extra = index - g.bucket_size;
 		if (t->overflow_sum + extra < t->overflow_sum) {
 			/* The uint64_t even overflowed itself; bail out */
@@ -455,6 +459,19 @@ static void *thread_main(void *arg)
 		printf("%s\n", end);                    \
 	} while (0)
 
+#define putfieldp(label, val, end) do {                        \
+		printf("%12s:\t", label);                      \
+		for (i = 0; i < g.n_threads; ++i)              \
+			printf(" %.*f", g.precision,           \
+			       (double)(val) / g.unit_per_us); \
+		printf("%s\n", end);                           \
+	} while (0)
+
+static double bucket_to_lat(int bucket)
+{
+	return (g.bias + (bucket + 1) * (double)g.bucket_width) / g.unit_per_us;
+}
+
 void calculate(struct thread *t)
 {
 	int i, j;
@@ -465,11 +482,11 @@ void calculate(struct thread *t)
 		/* Calculate average */
 		sum = count = 0;
 		for (j = 0; j < g.bucket_size; j++) {
-			sum += 1.0 * t[i].buckets[j] * (g.bias+j+1);
+			sum += t[i].buckets[j] * bucket_to_lat(j);
 			count += t[i].buckets[j];
 		}
 		/* Add the extra amount of huge spikes in */
-		sum += t->overflow_sum;
+		sum += t->overflow_sum * g.bucket_width;
 		t[i].average = sum / count;
 	}
 }
@@ -501,16 +518,16 @@ static void write_summary(struct thread *t)
 			print_dotdotdot = 0;
 		}
 
-		snprintf(bucket_name, sizeof(bucket_name), "%03"PRIu64
-			 " (us)", g.bias+j+1);
+		snprintf(bucket_name, sizeof(bucket_name), "%03.*f (us)",
+			 g.precision, bucket_to_lat(j));
 		putfield(bucket_name, t[i].buckets[j], PRIu64,
 			 (j == g.bucket_size - 1) ? " (including overflows)" : "");
 	}
 
-	putfield("Minimum", t[i].minlat, PRIu64, " (us)");
+	putfieldp("Minimum", t[i].minlat, " (us)");
 	putfield("Average", t[i].average, ".3lf", " (us)");
-	putfield("Maximum", t[i].maxlat, PRIu64, " (us)");
-	putfield("Max-Min", t[i].maxlat - t[i].minlat, PRIu64, " (us)");
+	putfieldp("Maximum", t[i].maxlat, " (us)");
+	putfieldp("Max-Min", t[i].maxlat - t[i].minlat, " (us)");
 	putfield("Duration", cycles_to_sec(&(t[i]), t[i].runtime),
 		 ".3f", " (sec)");
 	printf("\n");
@@ -537,8 +554,8 @@ static void write_summary_json(FILE *f, void *data)
 			if (t[i].buckets[j] == 0)
 				continue;
 			fprintf(f, "%s", comma ? ",\n" : "\n");
-			fprintf(f, "        \"%" PRIu64 "\": %" PRIu64,
-				g.bias+j+1, t[i].buckets[j]);
+			fprintf(f, "        \"%.*f\": %" PRIu64,
+				g.precision, bucket_to_lat(j), t[i].buckets[j]);
 			comma = 1;
 		}
 		if (comma)
@@ -610,6 +627,10 @@ static void usage(int error)
 	       "-v, --version          Display the version of the software.\n"
 	       "-w, --workload         Specify a kind of workload, default is no workload\n"
 	       "                       (options: no, memmove)\n"
+	       "-W, --bucket-width     Interval between buckets in nanoseconds\n"
+	       "                       NOTE: Widths not a multiple of 1000 cause ns-precision output\n"
+	       "                       You are responsible for considering the impact of measurement\n"
+	       "                       overhead at the nanosecond scale.\n"
 	       "-z, --zero-omit        Don't display buckets in the output histogram if all zeros.\n"
 	       );
 	exit(error);
@@ -630,7 +651,7 @@ static int workload_select(char *name)
 }
 
 enum option_value {
-	OPT_BUCKETSIZE=1, OPT_CPU_LIST, OPT_CPU_MAIN_THREAD,
+	OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, OPT_CPU_MAIN_THREAD,
 	OPT_DURATION, OPT_JSON, OPT_RT_PRIO, OPT_HELP, OPT_TRACE_TH,
 	OPT_WORKLOAD, OPT_WORKLOAD_MEM, OPT_BIAS,
 	OPT_QUIET, OPT_SINGLE_PREHEAT, OPT_ZERO_OMIT,
@@ -644,6 +665,7 @@ static void parse_options(int argc, char *argv[])
 		int option_index = 0;
 		static struct option options[] = {
 			{ "bucket-size", required_argument,	NULL, OPT_BUCKETSIZE },
+			{ "bucket-width", required_argument,	NULL, OPT_BUCKETWIDTH },
 			{ "cpu-list",	required_argument,	NULL, OPT_CPU_LIST },
 			{ "cpu-main-thread", required_argument, NULL, OPT_CPU_MAIN_THREAD},
 			{ "duration",	required_argument,	NULL, OPT_DURATION },
@@ -660,7 +682,7 @@ static void parse_options(int argc, char *argv[])
 			{ "version",	no_argument,		NULL, OPT_VERSION },
 			{ NULL, 0, NULL, 0 },
 		};
-		int i, c = getopt_long(argc, argv, "b:Bc:C:D:f:hm:qsw:T:vz",
+		int i, c = getopt_long(argc, argv, "b:Bc:C:D:f:hm:qsw:W:T:vz",
 				       options, &option_index);
 		long ncores;
 
@@ -677,6 +699,20 @@ static void parse_options(int argc, char *argv[])
 				exit(1);
 			}
 			break;
+		case OPT_BUCKETWIDTH:
+		case 'W':
+			g.bucket_width = strtol(optarg, NULL, 10);
+			if (g.bucket_size <= 0) {
+				printf("Illegal bucket width: %s\n", optarg);
+				exit(1);
+			}
+			if (g.bucket_width % 1000) {
+				g.unit_per_us = 1000;
+				g.precision = 3;
+			} else {
+				g.bucket_width /= 1000;
+			}
+			break;
 		case OPT_BIAS:
 		case 'B':
 			g.enable_bias = 1;
@@ -811,7 +847,8 @@ static void record_bias(struct thread *t)
 			bias = t[i].minlat;
 	}
 	g.bias = bias;
-	printf("Global bias set to %" PRId64 " (us)\n", bias);
+	printf("Global bias set to %.*f (us)\n", g.precision,
+	       (double)bias / g.unit_per_us);
 }
 
 int main(int argc, char *argv[])
@@ -835,6 +872,8 @@ int main(int argc, char *argv[])
 	g.app_name = argv[0];
 	g.rtprio = 0;
 	g.bucket_size = BUCKET_SIZE;
+	g.bucket_width = 1;
+	g.unit_per_us = 1;
 	g.runtime = 1;
 	g.workload = &workload_list[WORKLOAD_DEFAULT];
 	g.workload_mem_size = WORKLOAD_MEM_SIZE;
-- 
2.38.1

Re: [PATCH] oslat: Add command line option for bucket width

[John Kacur]

On Thu, 8 Dec 2022, Crystal Wood wrote:

> New option -W/--bucket-width allows the user to specify how large of a
> range of latencies is covered by a single bucket, including allowing the
> creation of sub-microsecond buckets.
> 
> When the flag is not used, output should be unchanged.  However, if a
> bucket width is specified that is not a multiple of one microsecond,
> latencies will be output as fractional microseconds, at nanosecond
> precision.  This includes JSON output.
> 
> When using this option, it is up to the user to determine what level
> of precision is meaningful relative to measurement error, as is noted
> in the documentation.
> 
> Signed-off-by: Crystal Wood <swood@redhat.com>
> ---
> I'm hoping that this output format is acceptable, including JSON,
> though I'm not familiar with how the latter is typically consumed.
> 
> Another option would be to simplify by not making the output precision
> conditional (probably keeping the decimal for readability's sake, at
> least in the non-JSON output) but I didn't want to risk compatibility
> or user surprise issues.
> ---
>  src/oslat/oslat.8 |   9 +++-
>  src/oslat/oslat.c | 105 +++++++++++++++++++++++++++++++---------------
>  2 files changed, 80 insertions(+), 34 deletions(-)
> 
> diff --git a/src/oslat/oslat.8 b/src/oslat/oslat.8
> index 39b36df0db3f..eb96448bfff1 100644
> --- a/src/oslat/oslat.8
> +++ b/src/oslat/oslat.8
> @@ -7,7 +7,7 @@ oslat \- OS Latency Detector
>  .RI "[ \-shvz ] [ \-b " bucket-size " ] [ \-B " bias " ] [ \-c " cpu-list " ] \
>  [ \-C " cpu-main-thread " ] [ \-f " rt-prio " ] [ \-\-json " filename " ] \
>  [ \-m " workload-mem " ] [\-t " runtime " ] [ \-T " trace-threshold " ] \
> -[ \-w " workload " ]"
> +[ \-w " workload " ] [ \-W " bucket-width " ]"
>  .SH DESCRIPTION
>  .B oslat
>  is an open source userspace polling mode stress program to detect OS level
> @@ -57,6 +57,13 @@ NOTE: please make sure the CPU frequency on all testing cores
>  are locked before using this parmater.  If you don't know how
>  to lock the freq then please don't use this parameter.
>  .TP
> +.B \-W, \-\-bucket-width
> +Interval between buckets in nanoseconds
> +
> +NOTE: Widths not a multiple of 1000 cause ns-precision output
> +You are responsible for considering the impact of measurement
> +overhead at the nanosecond scale.
> +.TP
>  .B \-h, \-\-help
>  Show the help message.
>  .TP
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index 55302f11986b..e8a642699cc7 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -192,6 +192,9 @@ struct global {
>  	struct timeval        tv_start;
>  	int                   rtprio;
>  	int                   bucket_size;
> +	int                   bucket_width;
> +	int                   unit_per_us;
> +	int                   precision;
>  	int                   trace_threshold;
>  	int                   runtime;
>  	/* The core that we run the main thread.  Default is cpu0 */
> @@ -325,45 +328,46 @@ static float cycles_to_sec(const struct thread *t, uint64_t cycles)
>  
>  static void insert_bucket(struct thread *t, stamp_t value)
>  {
> -	int index, us;
> +	int index;
> +	unsigned int lat;
>  	uint64_t extra;
> +	double us;
>  
> -	index = value / t->counter_mhz;
> -	assert(index >= 0);
> -	us = index + 1;
> -	assert(us > 0);
> -
> +	lat = (value * g.unit_per_us + t->counter_mhz - 1) / t->counter_mhz;
> +	us = (double)lat / g.unit_per_us;
>  	if (!g.preheat && g.trace_threshold && us >= g.trace_threshold) {
> -		char *line = "%s: Trace threshold (%d us) triggered on cpu %d with %u us!\n"
> +		char *line = "%s: Trace threshold (%d us) triggered on cpu %d with %.*f us!\n"
>  		    "Stopping the test.\n";
> -		tracemark(line, g.app_name, g.trace_threshold, t->core_i, us);
> -		err_quit(line, g.app_name, g.trace_threshold, t->core_i, us);
> +		tracemark(line, g.app_name, g.trace_threshold, t->core_i,
> +		          g.precision, us);
> +		err_quit(line, g.app_name, g.trace_threshold, t->core_i,
> +		         g.precision, us);
>  	}
>  
>  	/* Update max latency */
> -	if (us > t->maxlat)
> -		t->maxlat = us;
> +	if (lat > t->maxlat)
> +		t->maxlat = lat;
>  
> -	if (us < t->minlat)
> -		t->minlat = us;
> +	if (lat < t->minlat)
> +		t->minlat = lat;
>  
>  	if (g.bias) {
>  		/* t->bias will be set after pre-heat if user enabled it */
> -		us -= g.bias;
> +		lat -= g.bias;
>  		/*
>  		 * Negative should hardly happen, but if it happens, we assume we're in
> -		 * the smallest bucket, which is 1us.  Same to index.
> +		 * the smallest bucket.
>  		 */
> -		if (us <= 0)
> -			us = 1;
> -		index -= g.bias;
> -		if (index < 0)
> -			index = 0;
> +		if (lat <= 0)
> +			lat = 1;
>  	}
>  
> +	index = lat / g.bucket_width;
> +	assert(index >= 0);
> +
>  	/* Too big the jitter; put into the last bucket */
>  	if (index >= g.bucket_size) {
> -		/* Keep the extra bit (in us) */
> +		/* Keep the extra bit (in bucket width multiples) */
>  		extra = index - g.bucket_size;
>  		if (t->overflow_sum + extra < t->overflow_sum) {
>  			/* The uint64_t even overflowed itself; bail out */
> @@ -455,6 +459,19 @@ static void *thread_main(void *arg)
>  		printf("%s\n", end);                    \
>  	} while (0)
>  
> +#define putfieldp(label, val, end) do {                        \
> +		printf("%12s:\t", label);                      \
> +		for (i = 0; i < g.n_threads; ++i)              \
> +			printf(" %.*f", g.precision,           \
> +			       (double)(val) / g.unit_per_us); \
> +		printf("%s\n", end);                           \
> +	} while (0)
> +
> +static double bucket_to_lat(int bucket)
> +{
> +	return (g.bias + (bucket + 1) * (double)g.bucket_width) / g.unit_per_us;
> +}
> +
>  void calculate(struct thread *t)
>  {
>  	int i, j;
> @@ -465,11 +482,11 @@ void calculate(struct thread *t)
>  		/* Calculate average */
>  		sum = count = 0;
>  		for (j = 0; j < g.bucket_size; j++) {
> -			sum += 1.0 * t[i].buckets[j] * (g.bias+j+1);
> +			sum += t[i].buckets[j] * bucket_to_lat(j);
>  			count += t[i].buckets[j];
>  		}
>  		/* Add the extra amount of huge spikes in */
> -		sum += t->overflow_sum;
> +		sum += t->overflow_sum * g.bucket_width;
>  		t[i].average = sum / count;
>  	}
>  }
> @@ -501,16 +518,16 @@ static void write_summary(struct thread *t)
>  			print_dotdotdot = 0;
>  		}
>  
> -		snprintf(bucket_name, sizeof(bucket_name), "%03"PRIu64
> -			 " (us)", g.bias+j+1);
> +		snprintf(bucket_name, sizeof(bucket_name), "%03.*f (us)",
> +			 g.precision, bucket_to_lat(j));
>  		putfield(bucket_name, t[i].buckets[j], PRIu64,
>  			 (j == g.bucket_size - 1) ? " (including overflows)" : "");
>  	}
>  
> -	putfield("Minimum", t[i].minlat, PRIu64, " (us)");
> +	putfieldp("Minimum", t[i].minlat, " (us)");
>  	putfield("Average", t[i].average, ".3lf", " (us)");
> -	putfield("Maximum", t[i].maxlat, PRIu64, " (us)");
> -	putfield("Max-Min", t[i].maxlat - t[i].minlat, PRIu64, " (us)");
> +	putfieldp("Maximum", t[i].maxlat, " (us)");
> +	putfieldp("Max-Min", t[i].maxlat - t[i].minlat, " (us)");
>  	putfield("Duration", cycles_to_sec(&(t[i]), t[i].runtime),
>  		 ".3f", " (sec)");
>  	printf("\n");
> @@ -537,8 +554,8 @@ static void write_summary_json(FILE *f, void *data)
>  			if (t[i].buckets[j] == 0)
>  				continue;
>  			fprintf(f, "%s", comma ? ",\n" : "\n");
> -			fprintf(f, "        \"%" PRIu64 "\": %" PRIu64,
> -				g.bias+j+1, t[i].buckets[j]);
> +			fprintf(f, "        \"%.*f\": %" PRIu64,
> +				g.precision, bucket_to_lat(j), t[i].buckets[j]);
>  			comma = 1;
>  		}
>  		if (comma)
> @@ -610,6 +627,10 @@ static void usage(int error)
>  	       "-v, --version          Display the version of the software.\n"
>  	       "-w, --workload         Specify a kind of workload, default is no workload\n"
>  	       "                       (options: no, memmove)\n"
> +	       "-W, --bucket-width     Interval between buckets in nanoseconds\n"
> +	       "                       NOTE: Widths not a multiple of 1000 cause ns-precision output\n"
> +	       "                       You are responsible for considering the impact of measurement\n"
> +	       "                       overhead at the nanosecond scale.\n"
>  	       "-z, --zero-omit        Don't display buckets in the output histogram if all zeros.\n"
>  	       );
>  	exit(error);
> @@ -630,7 +651,7 @@ static int workload_select(char *name)
>  }
>  
>  enum option_value {
> -	OPT_BUCKETSIZE=1, OPT_CPU_LIST, OPT_CPU_MAIN_THREAD,
> +	OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, OPT_CPU_MAIN_THREAD,
>  	OPT_DURATION, OPT_JSON, OPT_RT_PRIO, OPT_HELP, OPT_TRACE_TH,
>  	OPT_WORKLOAD, OPT_WORKLOAD_MEM, OPT_BIAS,
>  	OPT_QUIET, OPT_SINGLE_PREHEAT, OPT_ZERO_OMIT,
> @@ -644,6 +665,7 @@ static void parse_options(int argc, char *argv[])
>  		int option_index = 0;
>  		static struct option options[] = {
>  			{ "bucket-size", required_argument,	NULL, OPT_BUCKETSIZE },
> +			{ "bucket-width", required_argument,	NULL, OPT_BUCKETWIDTH },
>  			{ "cpu-list",	required_argument,	NULL, OPT_CPU_LIST },
>  			{ "cpu-main-thread", required_argument, NULL, OPT_CPU_MAIN_THREAD},
>  			{ "duration",	required_argument,	NULL, OPT_DURATION },
> @@ -660,7 +682,7 @@ static void parse_options(int argc, char *argv[])
>  			{ "version",	no_argument,		NULL, OPT_VERSION },
>  			{ NULL, 0, NULL, 0 },
>  		};
> -		int i, c = getopt_long(argc, argv, "b:Bc:C:D:f:hm:qsw:T:vz",
> +		int i, c = getopt_long(argc, argv, "b:Bc:C:D:f:hm:qsw:W:T:vz",
>  				       options, &option_index);
>  		long ncores;
>  
> @@ -677,6 +699,20 @@ static void parse_options(int argc, char *argv[])
>  				exit(1);
>  			}
>  			break;
> +		case OPT_BUCKETWIDTH:
> +		case 'W':
> +			g.bucket_width = strtol(optarg, NULL, 10);
> +			if (g.bucket_size <= 0) {

I think this should be g.bucket_width

> +				printf("Illegal bucket width: %s\n", optarg);
> +				exit(1);
> +			}
> +			if (g.bucket_width % 1000) {
> +				g.unit_per_us = 1000;
> +				g.precision = 3;
> +			} else {
> +				g.bucket_width /= 1000;
> +			}
> +			break;
>  		case OPT_BIAS:
>  		case 'B':
>  			g.enable_bias = 1;
> @@ -811,7 +847,8 @@ static void record_bias(struct thread *t)
>  			bias = t[i].minlat;
>  	}
>  	g.bias = bias;
> -	printf("Global bias set to %" PRId64 " (us)\n", bias);
> +	printf("Global bias set to %.*f (us)\n", g.precision,
> +	       (double)bias / g.unit_per_us);
>  }
>  
>  int main(int argc, char *argv[])
> @@ -835,6 +872,8 @@ int main(int argc, char *argv[])
>  	g.app_name = argv[0];
>  	g.rtprio = 0;
>  	g.bucket_size = BUCKET_SIZE;
> +	g.bucket_width = 1;
> +	g.unit_per_us = 1;
>  	g.runtime = 1;
>  	g.workload = &workload_list[WORKLOAD_DEFAULT];
>  	g.workload_mem_size = WORKLOAD_MEM_SIZE;
> -- 
> 2.38.1
> 
> 

A quick first look through and run, see the one comment above
near "case 'W'"

and then

checkpatch reports some minor easily fixed problems

../linux/scripts/checkpatch.pl oslat.patch 
ERROR: code indent should use tabs where possible
#100: FILE: src/oslat/oslat.c:342:
+^I^I          g.precision, us);$

ERROR: code indent should use tabs where possible
#102: FILE: src/oslat/oslat.c:344:
+^I^I         g.precision, us);$

ERROR: spaces required around that '=' (ctx:VxV)
#227: FILE: src/oslat/oslat.c:654:
+	OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, 
OPT_CPU_MAIN_THREAD,
 	              ^

I'll have a closer look on Monday, but thanks for your patch!

John

Re: [PATCH] oslat: Add command line option for bucket width

[Crystal Wood]

On Fri, 2022-12-09 at 20:03 -0500, John Kacur wrote:
> 
> 
> On Thu, 8 Dec 2022, Crystal Wood wrote:
> 
> 
> >  
> > @@ -677,6 +699,20 @@ static void parse_options(int argc, char *argv[])
> >                                 exit(1);
> >                         }
> >                         break;
> > +               case OPT_BUCKETWIDTH:
> > +               case 'W':
> > +                       g.bucket_width = strtol(optarg, NULL, 10);
> > +                       if (g.bucket_size <= 0) {
> 
> I think this should be g.bucket_width

Oops

> 
> A quick first look through and run, see the one comment above
> near "case 'W'"
> 
> and then
> 
> checkpatch reports some minor easily fixed problems
> 
> ../linux/scripts/checkpatch.pl oslat.patch 
> ERROR: code indent should use tabs where possible
> #100: FILE: src/oslat/oslat.c:342:
> +^I^I          g.precision, us);$
> 
> ERROR: code indent should use tabs where possible
> #102: FILE: src/oslat/oslat.c:344:
> +^I^I         g.precision, us);$

I was matching the existing style in the file that tended to use spaces for
alignment.

> ERROR: spaces required around that '=' (ctx:VxV)
> #227: FILE: src/oslat/oslat.c:654:
> +       OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, 
> OPT_CPU_MAIN_THREAD,
>                       ^

I only added OPT_BUCKETWIDTH to the list; I didn't touch the =1 part.

-Scott

Re: [PATCH] oslat: Add command line option for bucket width

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]



On Fri, 9 Dec 2022, Crystal Wood wrote:

> On Fri, 2022-12-09 at 20:03 -0500, John Kacur wrote:
> > 
> > 
> > On Thu, 8 Dec 2022, Crystal Wood wrote:
> > 
> > 
> > >  
> > > @@ -677,6 +699,20 @@ static void parse_options(int argc, char *argv[])
> > >                                 exit(1);
> > >                         }
> > >                         break;
> > > +               case OPT_BUCKETWIDTH:
> > > +               case 'W':
> > > +                       g.bucket_width = strtol(optarg, NULL, 10);
> > > +                       if (g.bucket_size <= 0) {
> > 
> > I think this should be g.bucket_width
> 
> Oops
> 
> > 
> > A quick first look through and run, see the one comment above
> > near "case 'W'"
> > 
> > and then
> > 
> > checkpatch reports some minor easily fixed problems
> > 
> > ../linux/scripts/checkpatch.pl oslat.patch 
> > ERROR: code indent should use tabs where possible
> > #100: FILE: src/oslat/oslat.c:342:
> > +^I^I          g.precision, us);$
> > 
> > ERROR: code indent should use tabs where possible
> > #102: FILE: src/oslat/oslat.c:344:
> > +^I^I         g.precision, us);$
> 
> I was matching the existing style in the file that tended to use spaces for
> alignment.

Hmmn, when I run checkpatch on the file I don't get any other warnings 
about spaces, or when I look in vim I see tabs, maybe check your editor?
Please fix the above when you submit a version two

> 
> > ERROR: spaces required around that '=' (ctx:VxV)
> > #227: FILE: src/oslat/oslat.c:654:
> > +       OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, 
> > OPT_CPU_MAIN_THREAD,
> >                       ^
> 
> I only added OPT_BUCKETWIDTH to the list; I didn't touch the =1 part.

Yup, you're right, we're not as strict about the formatting in rt-tests as
the kernel is, and sure it will catch stuff that isn't strictly your fault 
but I do normally run checkpatch on the patches. If you want to throw in 
the spaces around the '=' in your patch I'd appreciate it.

Now to the more important stuff, I like your patch, but I have a few 
questions.

What if the user specifies a bucket-width greater than 1000?
Is it meaningful to to have a bucket-width of 2000 for example?

If we have a bucket width of 1500, then we are going to have 
the same precision as if we specified 500?

By the way I am not aware of any tools that are consuming
json output, and if there are, I'm sure they can adapt to
any changes such as decimal, so I wouldn't worry about that.

John

Re: [PATCH] oslat: Add command line option for bucket width

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]



On Fri, 9 Dec 2022, Crystal Wood wrote:

> On Fri, 2022-12-09 at 20:03 -0500, John Kacur wrote:
> > 
> > 
> > On Thu, 8 Dec 2022, Crystal Wood wrote:
> > 
> > 
> > >  
> > > @@ -677,6 +699,20 @@ static void parse_options(int argc, char *argv[])
> > >                                 exit(1);
> > >                         }
> > >                         break;
> > > +               case OPT_BUCKETWIDTH:
> > > +               case 'W':
> > > +                       g.bucket_width = strtol(optarg, NULL, 10);
> > > +                       if (g.bucket_size <= 0) {
> > 
> > I think this should be g.bucket_width
> 
> Oops
> 
> > 
> > A quick first look through and run, see the one comment above
> > near "case 'W'"
> > 
> > and then
> > 
> > checkpatch reports some minor easily fixed problems
> > 
> > ../linux/scripts/checkpatch.pl oslat.patch 
> > ERROR: code indent should use tabs where possible
> > #100: FILE: src/oslat/oslat.c:342:
> > +^I^I          g.precision, us);$
> > 
> > ERROR: code indent should use tabs where possible
> > #102: FILE: src/oslat/oslat.c:344:
> > +^I^I         g.precision, us);$
> 
> I was matching the existing style in the file that tended to use spaces for
> alignment.
> 
> > ERROR: spaces required around that '=' (ctx:VxV)
> > #227: FILE: src/oslat/oslat.c:654:
> > +       OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, 
> > OPT_CPU_MAIN_THREAD,
> >                       ^
> 
> I only added OPT_BUCKETWIDTH to the list; I didn't touch the =1 part.
> 
> -Scott
> 
> 

One more thing that we just saw, if you run oslat without any options, 
just the defaults you get 32 buckets with the highest one being 32us

But if you run
oslat -W 500

You still get 32 buckets but since the width is half, then largest bucket
is 32us

This increases the resolution of the buckets, but it puts all the overflow
in the 16us buckets, wondering if we should double the number of buckets
so that the largest one is still 32us ?

I realize you could do
oslat -b 64  -W 500
to achieve that, but perhaps the default is not good like this.

John

Re: [PATCH] oslat: Add command line option for bucket width

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]



On Tue, 13 Dec 2022, John Kacur wrote:

> 
> 
> On Fri, 9 Dec 2022, Crystal Wood wrote:
> 
> > On Fri, 2022-12-09 at 20:03 -0500, John Kacur wrote:
> > > 
> > > 
> > > On Thu, 8 Dec 2022, Crystal Wood wrote:
> > > 
> > > 
> > > >  
> > > > @@ -677,6 +699,20 @@ static void parse_options(int argc, char *argv[])
> > > >                                 exit(1);
> > > >                         }
> > > >                         break;
> > > > +               case OPT_BUCKETWIDTH:
> > > > +               case 'W':
> > > > +                       g.bucket_width = strtol(optarg, NULL, 10);
> > > > +                       if (g.bucket_size <= 0) {
> > > 
> > > I think this should be g.bucket_width
> > 
> > Oops
> > 
> > > 
> > > A quick first look through and run, see the one comment above
> > > near "case 'W'"
> > > 
> > > and then
> > > 
> > > checkpatch reports some minor easily fixed problems
> > > 
> > > ../linux/scripts/checkpatch.pl oslat.patch 
> > > ERROR: code indent should use tabs where possible
> > > #100: FILE: src/oslat/oslat.c:342:
> > > +^I^I          g.precision, us);$
> > > 
> > > ERROR: code indent should use tabs where possible
> > > #102: FILE: src/oslat/oslat.c:344:
> > > +^I^I         g.precision, us);$
> > 
> > I was matching the existing style in the file that tended to use spaces for
> > alignment.
> > 
> > > ERROR: spaces required around that '=' (ctx:VxV)
> > > #227: FILE: src/oslat/oslat.c:654:
> > > +       OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, 
> > > OPT_CPU_MAIN_THREAD,
> > >                       ^
> > 
> > I only added OPT_BUCKETWIDTH to the list; I didn't touch the =1 part.
> > 
> > -Scott
> > 
> > 
> 
> One more thing that we just saw, if you run oslat without any options, 
> just the defaults you get 32 buckets with the highest one being 32us
> 
> But if you run
> oslat -W 500
> 
> You still get 32 buckets but since the width is half, then largest bucket
> is 32us

I mean 16us of course

> 
> This increases the resolution of the buckets, but it puts all the overflow
> in the 16us buckets, wondering if we should double the number of buckets
> so that the largest one is still 32us ?
> 
> I realize you could do
> oslat -b 64  -W 500
> to achieve that, but perhaps the default is not good like this.
> 
> John

Re: [PATCH] oslat: Add command line option for bucket width

[Crystal Wood]

On Tue, 2022-12-13 at 14:31 -0500, John Kacur wrote:
> 
> 
> On Fri, 9 Dec 2022, Crystal Wood wrote:
> 
> > On Fri, 2022-12-09 at 20:03 -0500, John Kacur wrote:
> > 
> > > A quick first look through and run, see the one comment above
> > > near "case 'W'"
> > > 
> > > and then
> > > 
> > > checkpatch reports some minor easily fixed problems
> > > 
> > > ../linux/scripts/checkpatch.pl oslat.patch 
> > > ERROR: code indent should use tabs where possible
> > > #100: FILE: src/oslat/oslat.c:342:
> > > +^I^I          g.precision, us);$
> > > 
> > > ERROR: code indent should use tabs where possible
> > > #102: FILE: src/oslat/oslat.c:344:
> > > +^I^I         g.precision, us);$
> > 
> > I was matching the existing style in the file that tended to use spaces
> > for
> > alignment.
> 
> Hmmn, when I run checkpatch on the file I don't get any other warnings 
> about spaces, or when I look in vim I see tabs, maybe check your editor?
> Please fix the above when you submit a version two

I see it in a lot of the structure definitions and macros; looks like
checkpatch only cares if it's part of the whitespace at the beginning of a
line (which is not necessarily the same as indentation, but whatever).  I'll
change it since you asked (it wasn't clear from the docs that checkpatch.pl
is the applicable style guide), but barring specific style guides or
pushback I tend to default to being kind to people using different tab
widths, having been one in a past life before the kernel beat it out of me.
:-)

> > > ERROR: spaces required around that '=' (ctx:VxV)
> > > #227: FILE: src/oslat/oslat.c:654:
> > > +       OPT_BUCKETSIZE=1, OPT_BUCKETWIDTH, OPT_CPU_LIST, 
> > > OPT_CPU_MAIN_THREAD,
> > >                       ^
> > 
> > I only added OPT_BUCKETWIDTH to the list; I didn't touch the =1 part.
> 
> Yup, you're right, we're not as strict about the formatting in rt-tests as
> the kernel is, and sure it will catch stuff that isn't strictly your fault
> but I do normally run checkpatch on the patches. If you want to throw in 
> the spaces around the '=' in your patch I'd appreciate it.

Sure.  As an aside, I do think that's a rule that is usually helpful but can
harm readability in some circumstances when applied strictly (e.g. "2*x + 1"
looks better to me than "2 * x + 1" which doesn't emphasize the order of
operations and doesn't provide the eye with as much structure).

> Now to the more important stuff, I like your patch, but I have a few 
> questions.
> 
> What if the user specifies a bucket-width greater than 1000?
> Is it meaningful to to have a bucket-width of 2000 for example?

Yes, that works and could be useful if you're trying to focus on larger
latency sources.

> If we have a bucket width of 1500, then we are going to have 
> the same precision as if we specified 500?

Anything that isn't a multiple of 1000 will give you nanosecond precision.

-Crystal

Re: [PATCH] oslat: Add command line option for bucket width

[Crystal Wood]

On Tue, 2022-12-13 at 15:41 -0500, John Kacur wrote:


> One more thing that we just saw, if you run oslat without any options, 
> just the defaults you get 32 buckets with the highest one being 32us
> 
> But if you run
> oslat -W 500
> 
> You still get 32 buckets but since the width is half, then largest bucket
> is 32us
> 
> This increases the resolution of the buckets, but it puts all the overflow
> in the 16us buckets, wondering if we should double the number of buckets
> so that the largest one is still 32us ?
> 
> I realize you could do
> oslat -b 64  -W 500
> to achieve that, but perhaps the default is not good like this.

Sounds reasonable.

-Scott