On 1/15/22 00:46, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> 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@xxxxxxx>
> ---
> 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
