On 2021/11/23 23:27, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> Commit 56440e63ac17 ("fio: report percentiles for slat, clat, lat")
> together with commit 38ec5c514104 ("stat: make priority summary statistics
> consistent with percentiles") changed so that per prio stats track either
> completion latency (clat) or total latency (lat), depending on the option
> lat_percentiles.
>
> It is not obvious why add_clat_sample() shouldn't add a high/low clat prio
> sample when option lat_percentiles is set, especially considering that
> option lat_percentiles is usually used for controlling if total latency
> percentiles should be displayed or not.
>
> Add comments to describe why add_clat_sample() has to care about option
> lat_percentiles.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> ---
> stat.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/stat.c b/stat.c
> index e0dc99b6..44ca3894 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -3089,6 +3089,15 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
>
> add_stat_sample(&ts->clat_stat[ddir], nsec);
>
> + /*
> + * When lat_percentiles=1 (default 0), the reported high/low priority
> + * percentiles and stats are used for describing total latency values,
> + * even though the variable names themselves start with clat_.
> + *
> + * Because of the above definition, only let this function add a prio
> + * stat sample when lat_percentiles=0 (add_lat_sample() will add the
> + * prio stat sample when lat_percentiles=1).
> + */
> if (!ts->lat_percentiles) {
> if (high_prio)
> add_stat_sample(&ts->clat_high_prio_stat[ddir], nsec);
> @@ -3101,6 +3110,12 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
> offset, ioprio);
>
> if (ts->clat_percentiles) {
> + /*
> + * 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).
> + */
> if (ts->lat_percentiles)
> add_lat_percentile_sample_noprio(ts, nsec, ddir, FIO_CLAT);
> else
> @@ -3194,6 +3209,16 @@ void add_lat_sample(struct thread_data *td, enum fio_ddir ddir,
> add_log_sample(td, td->lat_log, sample_val(nsec), ddir, bs,
> offset, ioprio);
>
> + /*
> + * When lat_percentiles=1 (default 0), the reported high/low priority
> + * percentiles and stats are used for describing total latency values,
> + * even though the variable names themselves start with clat_.
> + *
> + * Because of the above definition, only let this function add a prio
> + * stat and prio lat percentile sample when lat_percentiles=1
> + * (add_clat_sample() will add the prio stat and prio lat percentile
> + * sample when lat_percentiles=0).
> + */
> if (ts->lat_percentiles) {
> add_lat_percentile_sample(ts, nsec, ddir, high_prio, FIO_LAT);
> if (high_prio)
>
That does help understand the code. Nice.
Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
