[PATCH 1/2] stat: remove duplicated code in show_mixed_ddir_status()

[PATCH 1/2] stat: remove duplicated code in show_mixed_ddir_status()

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

When using unified_rw_reporting=mixed, show_ddir_status() is called,
and is solely responsible for printing the mixed stats.

When using unified_rw_reporting=both, show_ddir_status() is called
and prints the regular output, after that, show_mixed_ddir_status()
is called to print the mixed stats.

The way that show_mixed_ddir_status_terse() and
add_mixed_ddir_status_json() is implemented, is to alloc a new local ts
that will hold the mixed result, and then simply call the regular non-mixed
print function show_ddir_status_terse()/add_ddir_status_json() with this
local ts.

show_mixed_ddir_status() also allocates a new local ts, but fails to
initialize the lat percentiles and the percentile_list in the new local ts.
Therefore, show_mixed_ddir_status() has duplicated all the code from
show_ddir_status(), except that it uses the lat percentiles and the
percentile_list from the original ts.

Simplify show_mixed_ddir_status(), to behave in the same way as
show_mixed_ddir_status_terse() and add_mixed_ddir_status_json().

In other words, initialize the lat percentiles and the percentile_list in
the new local ts, and replace all the duplicated code with a simple call to
the regular non-mixed print function (show_ddir_status()).

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 stat.c | 187 +++++++++------------------------------------------------
 1 file changed, 28 insertions(+), 159 deletions(-)

diff --git a/stat.c b/stat.c
index 36742a25..2b3687bb 100644
--- a/stat.c
+++ b/stat.c
@@ -474,163 +474,6 @@ static double convert_agg_kbytes_percent(struct group_run_stats *rs, int ddir, i
 	return p_of_agg;
 }
 
-static void show_mixed_ddir_status(struct group_run_stats *rs,
-				   struct thread_stat *ts,
-				   struct buf_output *out)
-{
-	unsigned long runt;
-	unsigned long long min, max, bw, iops;
-	double mean, dev;
-	char *io_p, *bw_p, *bw_p_alt, *iops_p, *post_st = NULL;
-	struct thread_stat *ts_lcl;
-	int i2p;
-	int ddir = 0;
-
-	/*
-	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
-	 * Trims (ddir = 2) */
-	ts_lcl = malloc(sizeof(struct thread_stat));
-	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
-	/* calculate mixed stats  */
-	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
-	init_thread_stat_min_vals(ts_lcl);
-
-	sum_thread_stats(ts_lcl, ts);
-
-	assert(ddir_rw(ddir));
-
-	if (!ts_lcl->runtime[ddir]) {
-		free(ts_lcl);
-		return;
-	}
-
-	i2p = is_power_of_2(rs->kb_base);
-	runt = ts_lcl->runtime[ddir];
-
-	bw = (1000 * ts_lcl->io_bytes[ddir]) / runt;
-	io_p = num2str(ts_lcl->io_bytes[ddir], ts->sig_figs, 1, i2p, N2S_BYTE);
-	bw_p = num2str(bw, ts->sig_figs, 1, i2p, ts->unit_base);
-	bw_p_alt = num2str(bw, ts->sig_figs, 1, !i2p, ts->unit_base);
-
-	iops = (1000 * ts_lcl->total_io_u[ddir]) / runt;
-	iops_p = num2str(iops, ts->sig_figs, 1, 0, N2S_NONE);
-
-	log_buf(out, "  mixed: IOPS=%s, BW=%s (%s)(%s/%llumsec)%s\n",
-			iops_p, bw_p, bw_p_alt, io_p,
-			(unsigned long long) ts_lcl->runtime[ddir],
-			post_st ? : "");
-
-	free(post_st);
-	free(io_p);
-	free(bw_p);
-	free(bw_p_alt);
-	free(iops_p);
-
-	if (calc_lat(&ts_lcl->slat_stat[ddir], &min, &max, &mean, &dev))
-		display_lat("slat", min, max, mean, dev, out);
-	if (calc_lat(&ts_lcl->clat_stat[ddir], &min, &max, &mean, &dev))
-		display_lat("clat", min, max, mean, dev, out);
-	if (calc_lat(&ts_lcl->lat_stat[ddir], &min, &max, &mean, &dev))
-		display_lat(" lat", min, max, mean, dev, out);
-	if (calc_lat(&ts_lcl->clat_high_prio_stat[ddir], &min, &max, &mean, &dev)) {
-		display_lat(ts_lcl->lat_percentiles ? "high prio_lat" : "high prio_clat",
-				min, max, mean, dev, out);
-		if (calc_lat(&ts_lcl->clat_low_prio_stat[ddir], &min, &max, &mean, &dev))
-			display_lat(ts_lcl->lat_percentiles ? "low prio_lat" : "low prio_clat",
-					min, max, mean, dev, out);
-	}
-
-	if (ts->slat_percentiles && ts_lcl->slat_stat[ddir].samples > 0)
-		show_clat_percentiles(ts_lcl->io_u_plat[FIO_SLAT][ddir],
-				ts_lcl->slat_stat[ddir].samples,
-				ts->percentile_list,
-				ts->percentile_precision, "slat", out);
-	if (ts->clat_percentiles && ts_lcl->clat_stat[ddir].samples > 0)
-		show_clat_percentiles(ts_lcl->io_u_plat[FIO_CLAT][ddir],
-				ts_lcl->clat_stat[ddir].samples,
-				ts->percentile_list,
-				ts->percentile_precision, "clat", out);
-	if (ts->lat_percentiles && ts_lcl->lat_stat[ddir].samples > 0)
-		show_clat_percentiles(ts_lcl->io_u_plat[FIO_LAT][ddir],
-				ts_lcl->lat_stat[ddir].samples,
-				ts->percentile_list,
-				ts->percentile_precision, "lat", out);
-
-	if (ts->clat_percentiles || ts->lat_percentiles) {
-		const char *name = ts->lat_percentiles ? "lat" : "clat";
-		char prio_name[32];
-		uint64_t samples;
-
-		if (ts->lat_percentiles)
-			samples = ts_lcl->lat_stat[ddir].samples;
-		else
-			samples = ts_lcl->clat_stat[ddir].samples;
-
-		/* Only print if high and low priority stats were collected */
-		if (ts_lcl->clat_high_prio_stat[ddir].samples > 0 &&
-				ts_lcl->clat_low_prio_stat[ddir].samples > 0) {
-			sprintf(prio_name, "high prio (%.2f%%) %s",
-					100. * (double) ts_lcl->clat_high_prio_stat[ddir].samples / (double) samples,
-					name);
-			show_clat_percentiles(ts_lcl->io_u_plat_high_prio[ddir],
-					ts_lcl->clat_high_prio_stat[ddir].samples,
-					ts->percentile_list,
-					ts->percentile_precision, prio_name, out);
-
-			sprintf(prio_name, "low prio (%.2f%%) %s",
-					100. * (double) ts_lcl->clat_low_prio_stat[ddir].samples / (double) samples,
-					name);
-			show_clat_percentiles(ts_lcl->io_u_plat_low_prio[ddir],
-					ts_lcl->clat_low_prio_stat[ddir].samples,
-					ts->percentile_list,
-					ts->percentile_precision, prio_name, out);
-		}
-	}
-
-	if (calc_lat(&ts_lcl->bw_stat[ddir], &min, &max, &mean, &dev)) {
-		double p_of_agg = 100.0, fkb_base = (double)rs->kb_base;
-		const char *bw_str;
-
-		if ((rs->unit_base == 1) && i2p)
-			bw_str = "Kibit";
-		else if (rs->unit_base == 1)
-			bw_str = "kbit";
-		else if (i2p)
-			bw_str = "KiB";
-		else
-			bw_str = "kB";
-
-		p_of_agg = convert_agg_kbytes_percent(rs, ddir, mean);
-
-		if (rs->unit_base == 1) {
-			min *= 8.0;
-			max *= 8.0;
-			mean *= 8.0;
-			dev *= 8.0;
-		}
-
-		if (mean > fkb_base * fkb_base) {
-			min /= fkb_base;
-			max /= fkb_base;
-			mean /= fkb_base;
-			dev /= fkb_base;
-			bw_str = (rs->unit_base == 1 ? "Mibit" : "MiB");
-		}
-
-		log_buf(out, "   bw (%5s/s): min=%5llu, max=%5llu, per=%3.2f%%, "
-			"avg=%5.02f, stdev=%5.02f, samples=%" PRIu64 "\n",
-			bw_str, min, max, p_of_agg, mean, dev,
-			(&ts_lcl->bw_stat[ddir])->samples);
-	}
-	if (calc_lat(&ts_lcl->iops_stat[ddir], &min, &max, &mean, &dev)) {
-		log_buf(out, "   iops        : min=%5llu, max=%5llu, "
-			"avg=%5.02f, stdev=%5.02f, samples=%" PRIu64 "\n",
-			min, max, mean, dev, (&ts_lcl->iops_stat[ddir])->samples);
-	}
-
-	free(ts_lcl);
-}
-
 static void show_ddir_status(struct group_run_stats *rs, struct thread_stat *ts,
 			     int ddir, struct buf_output *out)
 {
@@ -797,6 +640,33 @@ static void show_ddir_status(struct group_run_stats *rs, struct thread_stat *ts,
 	}
 }
 
+static void show_mixed_ddir_status(struct group_run_stats *rs,
+				   struct thread_stat *ts,
+				   struct buf_output *out)
+{
+	struct thread_stat *ts_lcl;
+
+	/*
+	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
+	 * Trims (ddir = 2)
+	 */
+	ts_lcl = malloc(sizeof(struct thread_stat));
+	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
+	/* calculate mixed stats  */
+	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
+	init_thread_stat_min_vals(ts_lcl);
+	ts_lcl->lat_percentiles = ts->lat_percentiles;
+	ts_lcl->clat_percentiles = ts->clat_percentiles;
+	ts_lcl->slat_percentiles = ts->slat_percentiles;
+	ts_lcl->percentile_precision = ts->percentile_precision;
+	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
+
+	sum_thread_stats(ts_lcl, ts);
+
+	show_ddir_status(rs, ts_lcl, DDIR_READ, out);
+	free(ts_lcl);
+}
+
 static bool show_lat(double *io_u_lat, int nr, const char **ranges,
 		     const char *msg, struct buf_output *out)
 {
@@ -1478,10 +1348,9 @@ static void show_mixed_ddir_status_terse(struct thread_stat *ts,
 	ts_lcl->slat_percentiles = ts->slat_percentiles;
 	ts_lcl->percentile_precision = ts->percentile_precision;
 	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
-	
+
 	sum_thread_stats(ts_lcl, ts);
 
-	/* add the aggregated stats to json parent */
 	show_ddir_status_terse(ts_lcl, rs, DDIR_READ, ver, out);
 	free(ts_lcl);
 }
-- 
2.34.1

[PATCH 2/2] stat: move unified=both mixed allocation and calculation to new helper

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

When using unified_rw_reporting=both, we need to print both the
per ddir stats, as well as the mixed stats.

In order to print both, the regular printing functions are responsible
for printing the per ddir stats from the unmodified struct thread_stat,
and show_mixed_ddir_status(), show_mixed_ddir_status_terse()
or add_mixed_ddir_status_json() is responsible for calculating and
printing the mixed stats.

In order to keep the original struct thread_stat intact, these three
functions have to allocate a new local thread_stat, where the mixed ddir
result can be stored before printing.

Move the allocation and calculation of this new struct thread_stat to a
new helper function, so that the code is easier to follow.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 stat.c | 81 ++++++++++++++++++++--------------------------------------
 1 file changed, 27 insertions(+), 54 deletions(-)

diff --git a/stat.c b/stat.c
index 2b3687bb..ff2393b0 100644
--- a/stat.c
+++ b/stat.c
@@ -462,6 +462,30 @@ static void display_lat(const char *name, unsigned long long min,
 	free(maxp);
 }
 
+static struct thread_stat *gen_mixed_ddir_stats_from_ts(struct thread_stat *ts)
+{
+	struct thread_stat *ts_lcl;
+
+	/*
+	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
+	 * Trims (ddir = 2)
+	 */
+	ts_lcl = malloc(sizeof(struct thread_stat));
+	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
+	/* calculate mixed stats  */
+	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
+	init_thread_stat_min_vals(ts_lcl);
+	ts_lcl->lat_percentiles = ts->lat_percentiles;
+	ts_lcl->clat_percentiles = ts->clat_percentiles;
+	ts_lcl->slat_percentiles = ts->slat_percentiles;
+	ts_lcl->percentile_precision = ts->percentile_precision;
+	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
+
+	sum_thread_stats(ts_lcl, ts);
+
+	return ts_lcl;
+}
+
 static double convert_agg_kbytes_percent(struct group_run_stats *rs, int ddir, int mean)
 {
 	double p_of_agg = 100.0;
@@ -644,24 +668,7 @@ static void show_mixed_ddir_status(struct group_run_stats *rs,
 				   struct thread_stat *ts,
 				   struct buf_output *out)
 {
-	struct thread_stat *ts_lcl;
-
-	/*
-	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
-	 * Trims (ddir = 2)
-	 */
-	ts_lcl = malloc(sizeof(struct thread_stat));
-	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
-	/* calculate mixed stats  */
-	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
-	init_thread_stat_min_vals(ts_lcl);
-	ts_lcl->lat_percentiles = ts->lat_percentiles;
-	ts_lcl->clat_percentiles = ts->clat_percentiles;
-	ts_lcl->slat_percentiles = ts->slat_percentiles;
-	ts_lcl->percentile_precision = ts->percentile_precision;
-	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
-
-	sum_thread_stats(ts_lcl, ts);
+	struct thread_stat *ts_lcl = gen_mixed_ddir_stats_from_ts(ts);
 
 	show_ddir_status(rs, ts_lcl, DDIR_READ, out);
 	free(ts_lcl);
@@ -1332,24 +1339,7 @@ static void show_mixed_ddir_status_terse(struct thread_stat *ts,
 				   struct group_run_stats *rs,
 				   int ver, struct buf_output *out)
 {
-	struct thread_stat *ts_lcl;
-
-	/*
-	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
-	 * Trims (ddir = 2)
-	 */
-	ts_lcl = malloc(sizeof(struct thread_stat));
-	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
-	/* calculate mixed stats  */
-	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
-	init_thread_stat_min_vals(ts_lcl);
-	ts_lcl->lat_percentiles = ts->lat_percentiles;
-	ts_lcl->clat_percentiles = ts->clat_percentiles;
-	ts_lcl->slat_percentiles = ts->slat_percentiles;
-	ts_lcl->percentile_precision = ts->percentile_precision;
-	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
-
-	sum_thread_stats(ts_lcl, ts);
+	struct thread_stat *ts_lcl = gen_mixed_ddir_stats_from_ts(ts);
 
 	show_ddir_status_terse(ts_lcl, rs, DDIR_READ, ver, out);
 	free(ts_lcl);
@@ -1529,24 +1519,7 @@ static void add_ddir_status_json(struct thread_stat *ts,
 static void add_mixed_ddir_status_json(struct thread_stat *ts,
 		struct group_run_stats *rs, struct json_object *parent)
 {
-	struct thread_stat *ts_lcl;
-
-	/*
-	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
-	 * Trims (ddir = 2)
-	 */
-	ts_lcl = malloc(sizeof(struct thread_stat));
-	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
-	/* calculate mixed stats  */
-	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
-	init_thread_stat_min_vals(ts_lcl);
-	ts_lcl->lat_percentiles = ts->lat_percentiles;
-	ts_lcl->clat_percentiles = ts->clat_percentiles;
-	ts_lcl->slat_percentiles = ts->slat_percentiles;
-	ts_lcl->percentile_precision = ts->percentile_precision;
-	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
-
-	sum_thread_stats(ts_lcl, ts);
+	struct thread_stat *ts_lcl = gen_mixed_ddir_stats_from_ts(ts);
 
 	/* add the aggregated stats to json parent */
 	add_ddir_status_json(ts_lcl, rs, DDIR_READ, parent);
-- 
2.34.1

Re: [PATCH 2/2] stat: move unified=both mixed allocation and calculation to new helper

[Damien Le Moal]

On 1/15/22 00:46, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> When using unified_rw_reporting=both, we need to print both the
> per ddir stats, as well as the mixed stats.
> 
> In order to print both, the regular printing functions are responsible
> for printing the per ddir stats from the unmodified struct thread_stat,
> and show_mixed_ddir_status(), show_mixed_ddir_status_terse()
> or add_mixed_ddir_status_json() is responsible for calculating and
> printing the mixed stats.
> 
> In order to keep the original struct thread_stat intact, these three
> functions have to allocate a new local thread_stat, where the mixed ddir
> result can be stored before printing.
> 
> Move the allocation and calculation of this new struct thread_stat to a
> new helper function, so that the code is easier to follow.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  stat.c | 81 ++++++++++++++++++++--------------------------------------
>  1 file changed, 27 insertions(+), 54 deletions(-)
> 
> diff --git a/stat.c b/stat.c
> index 2b3687bb..ff2393b0 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -462,6 +462,30 @@ static void display_lat(const char *name, unsigned long long min,
>  	free(maxp);
>  }
>  
> +static struct thread_stat *gen_mixed_ddir_stats_from_ts(struct thread_stat *ts)
> +{
> +	struct thread_stat *ts_lcl;
> +
> +	/*
> +	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
> +	 * Trims (ddir = 2)
> +	 */
> +	ts_lcl = malloc(sizeof(struct thread_stat));

No failed allocation check ?

> +	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));

Nit: a blank line here would be nice, for readability.

> +	/* calculate mixed stats  */
> +	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
> +	init_thread_stat_min_vals(ts_lcl);
> +	ts_lcl->lat_percentiles = ts->lat_percentiles;
> +	ts_lcl->clat_percentiles = ts->clat_percentiles;
> +	ts_lcl->slat_percentiles = ts->slat_percentiles;
> +	ts_lcl->percentile_precision = ts->percentile_precision;
> +	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
> +
> +	sum_thread_stats(ts_lcl, ts);
> +
> +	return ts_lcl;
> +}
> +
>  static double convert_agg_kbytes_percent(struct group_run_stats *rs, int ddir, int mean)
>  {
>  	double p_of_agg = 100.0;
> @@ -644,24 +668,7 @@ static void show_mixed_ddir_status(struct group_run_stats *rs,
>  				   struct thread_stat *ts,
>  				   struct buf_output *out)
>  {
> -	struct thread_stat *ts_lcl;
> -
> -	/*
> -	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
> -	 * Trims (ddir = 2)
> -	 */
> -	ts_lcl = malloc(sizeof(struct thread_stat));
> -	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
> -	/* calculate mixed stats  */
> -	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
> -	init_thread_stat_min_vals(ts_lcl);
> -	ts_lcl->lat_percentiles = ts->lat_percentiles;
> -	ts_lcl->clat_percentiles = ts->clat_percentiles;
> -	ts_lcl->slat_percentiles = ts->slat_percentiles;
> -	ts_lcl->percentile_precision = ts->percentile_precision;
> -	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
> -
> -	sum_thread_stats(ts_lcl, ts);
> +	struct thread_stat *ts_lcl = gen_mixed_ddir_stats_from_ts(ts);
>  

If you add allocation failure check in gen_mixed_ddir_stats_from_ts(),
then a NULL check is needed here too, alnd in the following functions
below too.

>  	show_ddir_status(rs, ts_lcl, DDIR_READ, out);
>  	free(ts_lcl);
> @@ -1332,24 +1339,7 @@ static void show_mixed_ddir_status_terse(struct thread_stat *ts,
>  				   struct group_run_stats *rs,
>  				   int ver, struct buf_output *out)
>  {
> -	struct thread_stat *ts_lcl;
> -
> -	/*
> -	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
> -	 * Trims (ddir = 2)
> -	 */
> -	ts_lcl = malloc(sizeof(struct thread_stat));
> -	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
> -	/* calculate mixed stats  */
> -	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
> -	init_thread_stat_min_vals(ts_lcl);
> -	ts_lcl->lat_percentiles = ts->lat_percentiles;
> -	ts_lcl->clat_percentiles = ts->clat_percentiles;
> -	ts_lcl->slat_percentiles = ts->slat_percentiles;
> -	ts_lcl->percentile_precision = ts->percentile_precision;
> -	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
> -
> -	sum_thread_stats(ts_lcl, ts);
> +	struct thread_stat *ts_lcl = gen_mixed_ddir_stats_from_ts(ts);
>  
>  	show_ddir_status_terse(ts_lcl, rs, DDIR_READ, ver, out);
>  	free(ts_lcl);
> @@ -1529,24 +1519,7 @@ static void add_ddir_status_json(struct thread_stat *ts,
>  static void add_mixed_ddir_status_json(struct thread_stat *ts,
>  		struct group_run_stats *rs, struct json_object *parent)
>  {
> -	struct thread_stat *ts_lcl;
> -
> -	/*
> -	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
> -	 * Trims (ddir = 2)
> -	 */
> -	ts_lcl = malloc(sizeof(struct thread_stat));
> -	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
> -	/* calculate mixed stats  */
> -	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
> -	init_thread_stat_min_vals(ts_lcl);
> -	ts_lcl->lat_percentiles = ts->lat_percentiles;
> -	ts_lcl->clat_percentiles = ts->clat_percentiles;
> -	ts_lcl->slat_percentiles = ts->slat_percentiles;
> -	ts_lcl->percentile_precision = ts->percentile_precision;
> -	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
> -
> -	sum_thread_stats(ts_lcl, ts);
> +	struct thread_stat *ts_lcl = gen_mixed_ddir_stats_from_ts(ts);
>  
>  	/* add the aggregated stats to json parent */
>  	add_ddir_status_json(ts_lcl, rs, DDIR_READ, parent);


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] stat: remove duplicated code in show_mixed_ddir_status()

[Damien Le Moal]

On 1/15/22 00:46, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> When using unified_rw_reporting=mixed, show_ddir_status() is called,
> and is solely responsible for printing the mixed stats.
> 
> When using unified_rw_reporting=both, show_ddir_status() is called
> and prints the regular output, after that, show_mixed_ddir_status()
> is called to print the mixed stats.
> 
> The way that show_mixed_ddir_status_terse() and
> add_mixed_ddir_status_json() is implemented, is to alloc a new local ts
> that will hold the mixed result, and then simply call the regular non-mixed
> print function show_ddir_status_terse()/add_ddir_status_json() with this
> local ts.
> 
> show_mixed_ddir_status() also allocates a new local ts, but fails to
> initialize the lat percentiles and the percentile_list in the new local ts.
> Therefore, show_mixed_ddir_status() has duplicated all the code from
> show_ddir_status(), except that it uses the lat percentiles and the
> percentile_list from the original ts.
> 
> Simplify show_mixed_ddir_status(), to behave in the same way as
> show_mixed_ddir_status_terse() and add_mixed_ddir_status_json().
> 
> In other words, initialize the lat percentiles and the percentile_list in
> the new local ts, and replace all the duplicated code with a simple call to
> the regular non-mixed print function (show_ddir_status()).
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  stat.c | 187 +++++++++------------------------------------------------
>  1 file changed, 28 insertions(+), 159 deletions(-)
> 
> diff --git a/stat.c b/stat.c
> index 36742a25..2b3687bb 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -474,163 +474,6 @@ static double convert_agg_kbytes_percent(struct group_run_stats *rs, int ddir, i
>  	return p_of_agg;
>  }
>  
> -static void show_mixed_ddir_status(struct group_run_stats *rs,
> -				   struct thread_stat *ts,
> -				   struct buf_output *out)
> -{
> -	unsigned long runt;
> -	unsigned long long min, max, bw, iops;
> -	double mean, dev;
> -	char *io_p, *bw_p, *bw_p_alt, *iops_p, *post_st = NULL;
> -	struct thread_stat *ts_lcl;
> -	int i2p;
> -	int ddir = 0;
> -
> -	/*
> -	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
> -	 * Trims (ddir = 2) */
> -	ts_lcl = malloc(sizeof(struct thread_stat));
> -	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
> -	/* calculate mixed stats  */
> -	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
> -	init_thread_stat_min_vals(ts_lcl);
> -
> -	sum_thread_stats(ts_lcl, ts);
> -
> -	assert(ddir_rw(ddir));
> -
> -	if (!ts_lcl->runtime[ddir]) {
> -		free(ts_lcl);
> -		return;
> -	}
> -
> -	i2p = is_power_of_2(rs->kb_base);
> -	runt = ts_lcl->runtime[ddir];
> -
> -	bw = (1000 * ts_lcl->io_bytes[ddir]) / runt;
> -	io_p = num2str(ts_lcl->io_bytes[ddir], ts->sig_figs, 1, i2p, N2S_BYTE);
> -	bw_p = num2str(bw, ts->sig_figs, 1, i2p, ts->unit_base);
> -	bw_p_alt = num2str(bw, ts->sig_figs, 1, !i2p, ts->unit_base);
> -
> -	iops = (1000 * ts_lcl->total_io_u[ddir]) / runt;
> -	iops_p = num2str(iops, ts->sig_figs, 1, 0, N2S_NONE);
> -
> -	log_buf(out, "  mixed: IOPS=%s, BW=%s (%s)(%s/%llumsec)%s\n",
> -			iops_p, bw_p, bw_p_alt, io_p,
> -			(unsigned long long) ts_lcl->runtime[ddir],
> -			post_st ? : "");
> -
> -	free(post_st);
> -	free(io_p);
> -	free(bw_p);
> -	free(bw_p_alt);
> -	free(iops_p);
> -
> -	if (calc_lat(&ts_lcl->slat_stat[ddir], &min, &max, &mean, &dev))
> -		display_lat("slat", min, max, mean, dev, out);
> -	if (calc_lat(&ts_lcl->clat_stat[ddir], &min, &max, &mean, &dev))
> -		display_lat("clat", min, max, mean, dev, out);
> -	if (calc_lat(&ts_lcl->lat_stat[ddir], &min, &max, &mean, &dev))
> -		display_lat(" lat", min, max, mean, dev, out);
> -	if (calc_lat(&ts_lcl->clat_high_prio_stat[ddir], &min, &max, &mean, &dev)) {
> -		display_lat(ts_lcl->lat_percentiles ? "high prio_lat" : "high prio_clat",
> -				min, max, mean, dev, out);
> -		if (calc_lat(&ts_lcl->clat_low_prio_stat[ddir], &min, &max, &mean, &dev))
> -			display_lat(ts_lcl->lat_percentiles ? "low prio_lat" : "low prio_clat",
> -					min, max, mean, dev, out);
> -	}
> -
> -	if (ts->slat_percentiles && ts_lcl->slat_stat[ddir].samples > 0)
> -		show_clat_percentiles(ts_lcl->io_u_plat[FIO_SLAT][ddir],
> -				ts_lcl->slat_stat[ddir].samples,
> -				ts->percentile_list,
> -				ts->percentile_precision, "slat", out);
> -	if (ts->clat_percentiles && ts_lcl->clat_stat[ddir].samples > 0)
> -		show_clat_percentiles(ts_lcl->io_u_plat[FIO_CLAT][ddir],
> -				ts_lcl->clat_stat[ddir].samples,
> -				ts->percentile_list,
> -				ts->percentile_precision, "clat", out);
> -	if (ts->lat_percentiles && ts_lcl->lat_stat[ddir].samples > 0)
> -		show_clat_percentiles(ts_lcl->io_u_plat[FIO_LAT][ddir],
> -				ts_lcl->lat_stat[ddir].samples,
> -				ts->percentile_list,
> -				ts->percentile_precision, "lat", out);
> -
> -	if (ts->clat_percentiles || ts->lat_percentiles) {
> -		const char *name = ts->lat_percentiles ? "lat" : "clat";
> -		char prio_name[32];
> -		uint64_t samples;
> -
> -		if (ts->lat_percentiles)
> -			samples = ts_lcl->lat_stat[ddir].samples;
> -		else
> -			samples = ts_lcl->clat_stat[ddir].samples;
> -
> -		/* Only print if high and low priority stats were collected */
> -		if (ts_lcl->clat_high_prio_stat[ddir].samples > 0 &&
> -				ts_lcl->clat_low_prio_stat[ddir].samples > 0) {
> -			sprintf(prio_name, "high prio (%.2f%%) %s",
> -					100. * (double) ts_lcl->clat_high_prio_stat[ddir].samples / (double) samples,
> -					name);
> -			show_clat_percentiles(ts_lcl->io_u_plat_high_prio[ddir],
> -					ts_lcl->clat_high_prio_stat[ddir].samples,
> -					ts->percentile_list,
> -					ts->percentile_precision, prio_name, out);
> -
> -			sprintf(prio_name, "low prio (%.2f%%) %s",
> -					100. * (double) ts_lcl->clat_low_prio_stat[ddir].samples / (double) samples,
> -					name);
> -			show_clat_percentiles(ts_lcl->io_u_plat_low_prio[ddir],
> -					ts_lcl->clat_low_prio_stat[ddir].samples,
> -					ts->percentile_list,
> -					ts->percentile_precision, prio_name, out);
> -		}
> -	}
> -
> -	if (calc_lat(&ts_lcl->bw_stat[ddir], &min, &max, &mean, &dev)) {
> -		double p_of_agg = 100.0, fkb_base = (double)rs->kb_base;
> -		const char *bw_str;
> -
> -		if ((rs->unit_base == 1) && i2p)
> -			bw_str = "Kibit";
> -		else if (rs->unit_base == 1)
> -			bw_str = "kbit";
> -		else if (i2p)
> -			bw_str = "KiB";
> -		else
> -			bw_str = "kB";
> -
> -		p_of_agg = convert_agg_kbytes_percent(rs, ddir, mean);
> -
> -		if (rs->unit_base == 1) {
> -			min *= 8.0;
> -			max *= 8.0;
> -			mean *= 8.0;
> -			dev *= 8.0;
> -		}
> -
> -		if (mean > fkb_base * fkb_base) {
> -			min /= fkb_base;
> -			max /= fkb_base;
> -			mean /= fkb_base;
> -			dev /= fkb_base;
> -			bw_str = (rs->unit_base == 1 ? "Mibit" : "MiB");
> -		}
> -
> -		log_buf(out, "   bw (%5s/s): min=%5llu, max=%5llu, per=%3.2f%%, "
> -			"avg=%5.02f, stdev=%5.02f, samples=%" PRIu64 "\n",
> -			bw_str, min, max, p_of_agg, mean, dev,
> -			(&ts_lcl->bw_stat[ddir])->samples);
> -	}
> -	if (calc_lat(&ts_lcl->iops_stat[ddir], &min, &max, &mean, &dev)) {
> -		log_buf(out, "   iops        : min=%5llu, max=%5llu, "
> -			"avg=%5.02f, stdev=%5.02f, samples=%" PRIu64 "\n",
> -			min, max, mean, dev, (&ts_lcl->iops_stat[ddir])->samples);
> -	}
> -
> -	free(ts_lcl);
> -}
> -
>  static void show_ddir_status(struct group_run_stats *rs, struct thread_stat *ts,
>  			     int ddir, struct buf_output *out)
>  {
> @@ -797,6 +640,33 @@ static void show_ddir_status(struct group_run_stats *rs, struct thread_stat *ts,
>  	}
>  }
>  
> +static void show_mixed_ddir_status(struct group_run_stats *rs,
> +				   struct thread_stat *ts,
> +				   struct buf_output *out)
> +{
> +	struct thread_stat *ts_lcl;
> +
> +	/*
> +	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
> +	 * Trims (ddir = 2)
> +	 */
> +	ts_lcl = malloc(sizeof(struct thread_stat));

Allocation failure check ? This is a void function, so that will be hard
to propagate... Granted, malloc() failing is likely very rare, but it is
possible. And for these cases, getting a "no memory" error would be
nicer than an obscure segfault :)

> +	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));

Canging the above malloc() to a calloc(), you can get rid of this memset.

> +	/* calculate mixed stats  */
> +	ts_lcl->unified_rw_rep = UNIFIED_MIXED;
> +	init_thread_stat_min_vals(ts_lcl);
> +	ts_lcl->lat_percentiles = ts->lat_percentiles;
> +	ts_lcl->clat_percentiles = ts->clat_percentiles;
> +	ts_lcl->slat_percentiles = ts->slat_percentiles;
> +	ts_lcl->percentile_precision = ts->percentile_precision;
> +	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
> +
> +	sum_thread_stats(ts_lcl, ts);
> +
> +	show_ddir_status(rs, ts_lcl, DDIR_READ, out);
> +	free(ts_lcl);
> +}
> +
>  static bool show_lat(double *io_u_lat, int nr, const char **ranges,
>  		     const char *msg, struct buf_output *out)
>  {
> @@ -1478,10 +1348,9 @@ static void show_mixed_ddir_status_terse(struct thread_stat *ts,
>  	ts_lcl->slat_percentiles = ts->slat_percentiles;
>  	ts_lcl->percentile_precision = ts->percentile_precision;
>  	memcpy(ts_lcl->percentile_list, ts->percentile_list, sizeof(ts->percentile_list));
> -	
> +
>  	sum_thread_stats(ts_lcl, ts);
>  
> -	/* add the aggregated stats to json parent */
>  	show_ddir_status_terse(ts_lcl, rs, DDIR_READ, ver, out);
>  	free(ts_lcl);
>  }


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] stat: move unified=both mixed allocation and calculation to new helper

[Niklas Cassel]

On Mon, Jan 17, 2022 at 09:45:36AM +0900, Damien Le Moal wrote:
> On 1/15/22 00:46, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> > 
> > When using unified_rw_reporting=both, we need to print both the
> > per ddir stats, as well as the mixed stats.
> > 
> > In order to print both, the regular printing functions are responsible
> > for printing the per ddir stats from the unmodified struct thread_stat,
> > and show_mixed_ddir_status(), show_mixed_ddir_status_terse()
> > or add_mixed_ddir_status_json() is responsible for calculating and
> > printing the mixed stats.
> > 
> > In order to keep the original struct thread_stat intact, these three
> > functions have to allocate a new local thread_stat, where the mixed ddir
> > result can be stored before printing.
> > 
> > Move the allocation and calculation of this new struct thread_stat to a
> > new helper function, so that the code is easier to follow.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  stat.c | 81 ++++++++++++++++++++--------------------------------------
> >  1 file changed, 27 insertions(+), 54 deletions(-)
> > 
> > diff --git a/stat.c b/stat.c
> > index 2b3687bb..ff2393b0 100644
> > --- a/stat.c
> > +++ b/stat.c
> > @@ -462,6 +462,30 @@ static void display_lat(const char *name, unsigned long long min,
> >  	free(maxp);
> >  }
> >  
> > +static struct thread_stat *gen_mixed_ddir_stats_from_ts(struct thread_stat *ts)
> > +{
> > +	struct thread_stat *ts_lcl;
> > +
> > +	/*
> > +	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
> > +	 * Trims (ddir = 2)
> > +	 */
> > +	ts_lcl = malloc(sizeof(struct thread_stat));
> 
> No failed allocation check ?

This patch simply creates a helper function.
The actual code should be identical to before.

If we want to add error handling to malloc() in fio, in all the places
where it is missing (which is a lot!), that is probably more suited for
a major cleanup series which addresses all the places where error handling
for malloc() is missing.

Right now, I'm not sure if fio is missing it because that is the chosen
design pattern (maybe because Linux memory overcommitment is enabled by
default?), or if it is by mistake, so I decided to keep the code as is.


> 
> > +	memset((void *)ts_lcl, 0, sizeof(struct thread_stat));
> 
> Nit: a blank line here would be nice, for readability.

I will send out a V2 that adds a blank line, and use init_thread_stat()
instead (which does the memset() and calls init_thread_stat_min_vals(),
so that we can remove those two lines from this function).


Kind regards,
Niklas

Re: [PATCH 2/2] stat: move unified=both mixed allocation and calculation to new helper

[Jens Axboe]

On 1/16/22 5:45 PM, Damien Le Moal wrote:
>> +static struct thread_stat *gen_mixed_ddir_stats_from_ts(struct thread_stat *ts)
>> +{
>> +	struct thread_stat *ts_lcl;
>> +
>> +	/*
>> +	 * Handle aggregation of Reads (ddir = 0), Writes (ddir = 1), and
>> +	 * Trims (ddir = 2)
>> +	 */
>> +	ts_lcl = malloc(sizeof(struct thread_stat));
> 
> No failed allocation check ?

For better or worse, fio generally just happily ignores allocation
failures... That doesn't mean that we should not move towards making
that better, particularly for new additions and where it's easy to
handle it.

-- 
Jens Axboe