On 2021/11/23 23:27, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> add_lat_percentile_prio_sample() currently adds both a per priority sample
> and a regular sample.
>
> Since these two samples are completely unrelated, it is very confusing that
> the add_lat_percentile_prio_sample() also adds a regular sample.
>
> Remove the add_lat_percentile_sample() function call from
> add_lat_percentile_prio_sample(), and let functions calling
> add_lat_percentile_prio_sample() call add_lat_percentile_sample()
> explicitly. This makes the flow in e.g. add_clat_sample() much easier to
> follow.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> ---
> stat.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/stat.c b/stat.c
> index 4eeb1c02..678fe953 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -3065,12 +3065,10 @@ static void add_lat_percentile_sample(struct thread_stat *ts,
> static void add_lat_percentile_prio_sample(struct thread_stat *ts,
> unsigned long long nsec,
> enum fio_ddir ddir,
> - bool high_prio, enum fio_lat lat)
> + bool high_prio)
> {
> unsigned int idx = plat_val_to_idx(nsec);
>
> - add_lat_percentile_sample(ts, nsec, ddir, lat);
> -
> if (!high_prio)
> ts->io_u_plat_low_prio[ddir][idx]++;
> else
> @@ -3112,17 +3110,16 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
> offset, ioprio);
>
> if (ts->clat_percentiles) {
> + add_lat_percentile_sample(ts, nsec, ddir, FIO_CLAT);
It would be nicer to move this after the comment.
> /*
> * Because of the above definition, only let this function add a
> * prio lat percentile sample when lat_percentiles=0
> * (add_lat_sample() will add the prio lat percentile sample
> * when lat_percentiles=1).
> */
And update the comment to:
/*
* Because of the above definition, add a prio lat
* percentile sample only when lat_percentiles=0.
* add_lat_sample() will add the prio lat percentile sample
* when lat_percentiles=1.
*/
> - if (ts->lat_percentiles)
> - add_lat_percentile_sample(ts, nsec, ddir, FIO_CLAT);
> - else
> - add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio,
> - FIO_CLAT);
> + if (!ts->lat_percentiles)
> + add_lat_percentile_prio_sample(ts, nsec, ddir,
> + high_prio);
> }
>
> if (iolog && iolog->hist_msec) {
> @@ -3223,8 +3220,8 @@ void add_lat_sample(struct thread_data *td, enum fio_ddir ddir,
> * sample when lat_percentiles=0).
> */
> if (ts->lat_percentiles) {
> - add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio,
> - FIO_LAT);
> + add_lat_percentile_sample(ts, nsec, ddir, FIO_LAT);
> + add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio);
> if (high_prio)
> add_stat_sample(&ts->clat_high_prio_stat[ddir], nsec);
> else
>
With the above fixed,
Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
