[PATCH v2 0/6] cleanup clat prio stat handling

[PATCH v2 0/6] cleanup clat prio stat handling

[Niklas Cassel]

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

Hello Jens,

Please consider this minor series which adds missing information about the
existing behavior of the per priority stats.
It also cleans up the per priority stat code, no functional changes,
so that it is easier to follow.

Changes since v1:
-Update man-page and HOWTO about the existing behavior of per prio stats.
-Move the patch that adds comments to stat.c to the beginning of the series,
 to more clearly highlight that this series has no functional changes.


Kind regards,
Niklas

Niklas Cassel (6):
  docs: document quirky implementation of per priority stats reporting
  stat: add comments describing the quirky behavior of clat prio samples
  stat: rename add_lat_percentile_sample()
  stat: rename add_lat_percentile_sample_noprio()
  stat: simplify add_lat_percentile_prio_sample()
  stat: make add lat percentile functions inline

 HOWTO  |  6 +++++-
 fio.1  |  5 ++++-
 stat.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 49 insertions(+), 15 deletions(-)

-- 
2.33.1

[PATCH v2 1/6] docs: document quirky implementation of per priority stats reporting

[Niklas Cassel]

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

Commit 56440e63ac17 ("fio: report percentiles for slat, clat, lat") changed
many things. One of the changes, from the commit message:
"- for the new cmdprio_percentage latencies, if lat_percentiles=1,
*total* latency percentiles will be tracked. Otherwise, *completion*
latency percentiles will be tracked."

In other words, the commit changed the per prio stats from always tracking
(and reporting) clat latency, to instead either track (and report) clat or
lat latency.

Considering that a certain latency type reports two things:
1) min/max/avg latency for the the specific latency type
2) latency percentiles for the specific latency type

If disable_clat/disable_lat is used, neither 1) nor 2) will be reported.
If clat_percentiles/lat_percentiles is false, 2) will not be reported.

Therefore it is unintuitive that setting lat_percentiles=1, an option
usually used to enable/disable percentile reporting, also affects which
type of latency that will be tracked (and reported) for per prio stats.

The fact that the variables are named e.g. clat_prio_stat, regardless of
the type of latency being tracked does not help.

Anyway, let's document the way that the current implementation works,
so that a user can know how per priority stats are handled, without having
to read the source, since the commit that introduced this behavior forgot
to update the documentation.

Fixes: 56440e63ac17 ("fio: report percentiles for slat, clat, lat")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 HOWTO | 6 +++++-
 fio.1 | 5 ++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/HOWTO b/HOWTO
index a3b3acfe..8c9e4135 100644
--- a/HOWTO
+++ b/HOWTO
@@ -2169,7 +2169,11 @@ with the caveat that when used on the command line, they must come after the
     Default: 0. A single value applies to reads and writes. Comma-separated
     values may be specified for reads and writes. For this option to be
     effective, NCQ priority must be supported and enabled, and `direct=1'
-    option must be used. fio must also be run as the root user.
+    option must be used. fio must also be run as the root user. Unlike
+    slat/clat/lat stats, which can be tracked and reported independently, per
+    priority stats only track and report a single type of latency. By default,
+    completion latency (clat) will be reported, if :option:`lat_percentiles` is
+    set, total latency (lat) will be reported.
 
 .. option:: cmdprio_class=int[,int] : [io_uring] [libaio]
 
diff --git a/fio.1 b/fio.1
index a6469541..a3ebb67d 100644
--- a/fio.1
+++ b/fio.1
@@ -1967,7 +1967,10 @@ Set the percentage of I/O that will be issued with the highest priority.
 Default: 0. A single value applies to reads and writes. Comma-separated
 values may be specified for reads and writes. For this option to be effective,
 NCQ priority must be supported and enabled, and `direct=1' option must be
-used. fio must also be run as the root user.
+used. fio must also be run as the root user. Unlike slat/clat/lat stats, which
+can be tracked and reported independently, per priority stats only track and
+report a single type of latency. By default, completion latency (clat) will be
+reported, if \fBlat_percentiles\fR is set, total latency (lat) will be reported.
 .TP
 .BI (io_uring,libaio)cmdprio_class \fR=\fPint[,int]
 Set the I/O priority class to use for I/Os that must be issued with a
-- 
2.33.1

[PATCH v2 2/6] stat: add comments describing the quirky behavior of clat prio samples

[Niklas Cassel]

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

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@wdc.com>
---
 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)
-- 
2.33.1

[PATCH v2 3/6] stat: rename add_lat_percentile_sample()

[Niklas Cassel]

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

The name for add_lat_percentile_sample() is confusing, since the function
actually adds a per priority percentile sample (it also adds a regular
sample), yet it doesn't have prio as part of the function name.

Rename the function so that it is more obvious that this function should
be used if you want to add a prio percentile sample.

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

diff --git a/stat.c b/stat.c
index 44ca3894..f5d5cd24 100644
--- a/stat.c
+++ b/stat.c
@@ -3061,9 +3061,10 @@ static void add_lat_percentile_sample_noprio(struct thread_stat *ts,
 	ts->io_u_plat[lat][ddir][idx]++;
 }
 
-static void add_lat_percentile_sample(struct thread_stat *ts,
-				unsigned long long nsec, enum fio_ddir ddir,
-				bool high_prio, enum fio_lat lat)
+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)
 {
 	unsigned int idx = plat_val_to_idx(nsec);
 
@@ -3119,7 +3120,8 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
 		if (ts->lat_percentiles)
 			add_lat_percentile_sample_noprio(ts, nsec, ddir, FIO_CLAT);
 		else
-			add_lat_percentile_sample(ts, nsec, ddir, high_prio, FIO_CLAT);
+			add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio,
+						       FIO_CLAT);
 	}
 
 	if (iolog && iolog->hist_msec) {
@@ -3220,7 +3222,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_sample(ts, nsec, ddir, high_prio, FIO_LAT);
+		add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio,
+					       FIO_LAT);
 		if (high_prio)
 			add_stat_sample(&ts->clat_high_prio_stat[ddir], nsec);
 		else
-- 
2.33.1

[PATCH v2 4/6] stat: rename add_lat_percentile_sample_noprio()

[Niklas Cassel]

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

add_lat_percentile_sample_noprio() is the regular function to add a latency
percentile sample. It adds a regular sample (it doesn't add any per
priority sample). Therefore, it makes sense that this function has no
suffix, neither _noprio nor _prio.

Drop the _noprio suffix from add_lat_percentile_sample_noprio(), to make it
more obvious that this function should be used if you want to add a regular
percentile sample.

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

diff --git a/stat.c b/stat.c
index f5d5cd24..4eeb1c02 100644
--- a/stat.c
+++ b/stat.c
@@ -3052,8 +3052,9 @@ void add_sync_clat_sample(struct thread_stat *ts, unsigned long long nsec)
 	add_stat_sample(&ts->sync_stat, nsec);
 }
 
-static void add_lat_percentile_sample_noprio(struct thread_stat *ts,
-				unsigned long long nsec, enum fio_ddir ddir, enum fio_lat lat)
+static void add_lat_percentile_sample(struct thread_stat *ts,
+				      unsigned long long nsec,
+				      enum fio_ddir ddir, enum fio_lat lat)
 {
 	unsigned int idx = plat_val_to_idx(nsec);
 	assert(idx < FIO_IO_U_PLAT_NR);
@@ -3068,7 +3069,7 @@ static void add_lat_percentile_prio_sample(struct thread_stat *ts,
 {
 	unsigned int idx = plat_val_to_idx(nsec);
 
-	add_lat_percentile_sample_noprio(ts, nsec, ddir, lat);
+	add_lat_percentile_sample(ts, nsec, ddir, lat);
 
 	if (!high_prio)
 		ts->io_u_plat_low_prio[ddir][idx]++;
@@ -3118,7 +3119,7 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
 		 * when lat_percentiles=1).
 		 */
 		if (ts->lat_percentiles)
-			add_lat_percentile_sample_noprio(ts, nsec, ddir, FIO_CLAT);
+			add_lat_percentile_sample(ts, nsec, ddir, FIO_CLAT);
 		else
 			add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio,
 						       FIO_CLAT);
@@ -3186,7 +3187,7 @@ void add_slat_sample(struct thread_data *td, enum fio_ddir ddir,
 			       offset, ioprio);
 
 	if (ts->slat_percentiles)
-		add_lat_percentile_sample_noprio(ts, nsec, ddir, FIO_SLAT);
+		add_lat_percentile_sample(ts, nsec, ddir, FIO_SLAT);
 
 	if (needs_lock)
 		__td_io_u_unlock(td);
-- 
2.33.1

[PATCH v2 5/6] stat: simplify add_lat_percentile_prio_sample()

[Niklas Cassel]

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

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@wdc.com>
---
 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);
 		/*
 		 * 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(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
-- 
2.33.1

[PATCH v2 6/6] stat: make add lat percentile functions inline

[Niklas Cassel]

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

Now when add_lat_percentile_prio_sample() has been simplified,
make both add lat percentile functions inline, just like add_stat_sample().

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

diff --git a/stat.c b/stat.c
index 678fe953..48c70072 100644
--- a/stat.c
+++ b/stat.c
@@ -3052,9 +3052,10 @@ void add_sync_clat_sample(struct thread_stat *ts, unsigned long long nsec)
 	add_stat_sample(&ts->sync_stat, nsec);
 }
 
-static void add_lat_percentile_sample(struct thread_stat *ts,
-				      unsigned long long nsec,
-				      enum fio_ddir ddir, enum fio_lat lat)
+static inline void add_lat_percentile_sample(struct thread_stat *ts,
+					     unsigned long long nsec,
+					     enum fio_ddir ddir,
+					     enum fio_lat lat)
 {
 	unsigned int idx = plat_val_to_idx(nsec);
 	assert(idx < FIO_IO_U_PLAT_NR);
@@ -3062,10 +3063,10 @@ static void add_lat_percentile_sample(struct thread_stat *ts,
 	ts->io_u_plat[lat][ddir][idx]++;
 }
 
-static void add_lat_percentile_prio_sample(struct thread_stat *ts,
-					   unsigned long long nsec,
-					   enum fio_ddir ddir,
-					   bool high_prio)
+static inline void add_lat_percentile_prio_sample(struct thread_stat *ts,
+						  unsigned long long nsec,
+						  enum fio_ddir ddir,
+						  bool high_prio)
 {
 	unsigned int idx = plat_val_to_idx(nsec);
 
-- 
2.33.1

Re: [PATCH v2 1/6] docs: document quirky implementation of per priority stats reporting

[Damien Le Moal]

On 2021/11/23 23:27, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Commit 56440e63ac17 ("fio: report percentiles for slat, clat, lat") changed
> many things. One of the changes, from the commit message:
> "- for the new cmdprio_percentage latencies, if lat_percentiles=1,
> *total* latency percentiles will be tracked. Otherwise, *completion*
> latency percentiles will be tracked."
> 
> In other words, the commit changed the per prio stats from always tracking
> (and reporting) clat latency, to instead either track (and report) clat or
> lat latency.
> 
> Considering that a certain latency type reports two things:
> 1) min/max/avg latency for the the specific latency type
> 2) latency percentiles for the specific latency type
> 
> If disable_clat/disable_lat is used, neither 1) nor 2) will be reported.
> If clat_percentiles/lat_percentiles is false, 2) will not be reported.
> 
> Therefore it is unintuitive that setting lat_percentiles=1, an option
> usually used to enable/disable percentile reporting, also affects which
> type of latency that will be tracked (and reported) for per prio stats.
> 
> The fact that the variables are named e.g. clat_prio_stat, regardless of
> the type of latency being tracked does not help.
> 
> Anyway, let's document the way that the current implementation works,
> so that a user can know how per priority stats are handled, without having
> to read the source, since the commit that introduced this behavior forgot
> to update the documentation.
> 
> Fixes: 56440e63ac17 ("fio: report percentiles for slat, clat, lat")
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  HOWTO | 6 +++++-
>  fio.1 | 5 ++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/HOWTO b/HOWTO
> index a3b3acfe..8c9e4135 100644
> --- a/HOWTO
> +++ b/HOWTO
> @@ -2169,7 +2169,11 @@ with the caveat that when used on the command line, they must come after the
>      Default: 0. A single value applies to reads and writes. Comma-separated
>      values may be specified for reads and writes. For this option to be
>      effective, NCQ priority must be supported and enabled, and `direct=1'
> -    option must be used. fio must also be run as the root user.
> +    option must be used. fio must also be run as the root user. Unlike
> +    slat/clat/lat stats, which can be tracked and reported independently, per
> +    priority stats only track and report a single type of latency. By default,
> +    completion latency (clat) will be reported, if :option:`lat_percentiles` is
> +    set, total latency (lat) will be reported.
>  
>  .. option:: cmdprio_class=int[,int] : [io_uring] [libaio]
>  
> diff --git a/fio.1 b/fio.1
> index a6469541..a3ebb67d 100644
> --- a/fio.1
> +++ b/fio.1
> @@ -1967,7 +1967,10 @@ Set the percentage of I/O that will be issued with the highest priority.
>  Default: 0. A single value applies to reads and writes. Comma-separated
>  values may be specified for reads and writes. For this option to be effective,
>  NCQ priority must be supported and enabled, and `direct=1' option must be
> -used. fio must also be run as the root user.
> +used. fio must also be run as the root user. Unlike slat/clat/lat stats, which
> +can be tracked and reported independently, per priority stats only track and
> +report a single type of latency. By default, completion latency (clat) will be
> +reported, if \fBlat_percentiles\fR is set, total latency (lat) will be reported.
>  .TP
>  .BI (io_uring,libaio)cmdprio_class \fR=\fPint[,int]
>  Set the I/O priority class to use for I/Os that must be issued with a
> 

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/6] stat: add comments describing the quirky behavior of clat prio samples

[Damien Le Moal]

On 2021/11/23 23:27, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> 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@wdc.com>
> ---
>  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@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/6] stat: rename add_lat_percentile_sample()

[Damien Le Moal]

On 2021/11/23 23:27, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> The name for add_lat_percentile_sample() is confusing, since the function
> actually adds a per priority percentile sample (it also adds a regular
> sample), yet it doesn't have prio as part of the function name.
> 
> Rename the function so that it is more obvious that this function should
> be used if you want to add a prio percentile sample.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  stat.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/stat.c b/stat.c
> index 44ca3894..f5d5cd24 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -3061,9 +3061,10 @@ static void add_lat_percentile_sample_noprio(struct thread_stat *ts,
>  	ts->io_u_plat[lat][ddir][idx]++;
>  }
>  
> -static void add_lat_percentile_sample(struct thread_stat *ts,
> -				unsigned long long nsec, enum fio_ddir ddir,
> -				bool high_prio, enum fio_lat lat)
> +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)
>  {
>  	unsigned int idx = plat_val_to_idx(nsec);
>  
> @@ -3119,7 +3120,8 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
>  		if (ts->lat_percentiles)
>  			add_lat_percentile_sample_noprio(ts, nsec, ddir, FIO_CLAT);
>  		else
> -			add_lat_percentile_sample(ts, nsec, ddir, high_prio, FIO_CLAT);
> +			add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio,
> +						       FIO_CLAT);
>  	}
>  
>  	if (iolog && iolog->hist_msec) {
> @@ -3220,7 +3222,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_sample(ts, nsec, ddir, high_prio, FIO_LAT);
> +		add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio,
> +					       FIO_LAT);
>  		if (high_prio)
>  			add_stat_sample(&ts->clat_high_prio_stat[ddir], nsec);
>  		else
> 

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 4/6] stat: rename add_lat_percentile_sample_noprio()

[Damien Le Moal]

On 2021/11/23 23:27, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> add_lat_percentile_sample_noprio() is the regular function to add a latency
> percentile sample. It adds a regular sample (it doesn't add any per
> priority sample). Therefore, it makes sense that this function has no
> suffix, neither _noprio nor _prio.
> 
> Drop the _noprio suffix from add_lat_percentile_sample_noprio(), to make it
> more obvious that this function should be used if you want to add a regular
> percentile sample.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  stat.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/stat.c b/stat.c
> index f5d5cd24..4eeb1c02 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -3052,8 +3052,9 @@ void add_sync_clat_sample(struct thread_stat *ts, unsigned long long nsec)
>  	add_stat_sample(&ts->sync_stat, nsec);
>  }
>  
> -static void add_lat_percentile_sample_noprio(struct thread_stat *ts,
> -				unsigned long long nsec, enum fio_ddir ddir, enum fio_lat lat)
> +static void add_lat_percentile_sample(struct thread_stat *ts,
> +				      unsigned long long nsec,
> +				      enum fio_ddir ddir, enum fio_lat lat)
>  {
>  	unsigned int idx = plat_val_to_idx(nsec);
>  	assert(idx < FIO_IO_U_PLAT_NR);
> @@ -3068,7 +3069,7 @@ static void add_lat_percentile_prio_sample(struct thread_stat *ts,
>  {
>  	unsigned int idx = plat_val_to_idx(nsec);
>  
> -	add_lat_percentile_sample_noprio(ts, nsec, ddir, lat);
> +	add_lat_percentile_sample(ts, nsec, ddir, lat);
>  
>  	if (!high_prio)
>  		ts->io_u_plat_low_prio[ddir][idx]++;
> @@ -3118,7 +3119,7 @@ void add_clat_sample(struct thread_data *td, enum fio_ddir ddir,
>  		 * when lat_percentiles=1).
>  		 */
>  		if (ts->lat_percentiles)
> -			add_lat_percentile_sample_noprio(ts, nsec, ddir, FIO_CLAT);
> +			add_lat_percentile_sample(ts, nsec, ddir, FIO_CLAT);
>  		else
>  			add_lat_percentile_prio_sample(ts, nsec, ddir, high_prio,
>  						       FIO_CLAT);
> @@ -3186,7 +3187,7 @@ void add_slat_sample(struct thread_data *td, enum fio_ddir ddir,
>  			       offset, ioprio);
>  
>  	if (ts->slat_percentiles)
> -		add_lat_percentile_sample_noprio(ts, nsec, ddir, FIO_SLAT);
> +		add_lat_percentile_sample(ts, nsec, ddir, FIO_SLAT);
>  
>  	if (needs_lock)
>  		__td_io_u_unlock(td);
> 

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 5/6] stat: simplify add_lat_percentile_prio_sample()

[Damien Le Moal]

On 2021/11/23 23:27, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> 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@wdc.com>
> ---
>  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@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 6/6] stat: make add lat percentile functions inline

[Damien Le Moal]

On 2021/11/23 23:27, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Now when add_lat_percentile_prio_sample() has been simplified,

s/when/that

> make both add lat percentile functions inline, just like add_stat_sample().
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  stat.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/stat.c b/stat.c
> index 678fe953..48c70072 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -3052,9 +3052,10 @@ void add_sync_clat_sample(struct thread_stat *ts, unsigned long long nsec)
>  	add_stat_sample(&ts->sync_stat, nsec);
>  }
>  
> -static void add_lat_percentile_sample(struct thread_stat *ts,
> -				      unsigned long long nsec,
> -				      enum fio_ddir ddir, enum fio_lat lat)
> +static inline void add_lat_percentile_sample(struct thread_stat *ts,
> +					     unsigned long long nsec,
> +					     enum fio_ddir ddir,
> +					     enum fio_lat lat)
>  {
>  	unsigned int idx = plat_val_to_idx(nsec);
>  	assert(idx < FIO_IO_U_PLAT_NR);
> @@ -3062,10 +3063,10 @@ static void add_lat_percentile_sample(struct thread_stat *ts,
>  	ts->io_u_plat[lat][ddir][idx]++;
>  }
>  
> -static void add_lat_percentile_prio_sample(struct thread_stat *ts,
> -					   unsigned long long nsec,
> -					   enum fio_ddir ddir,
> -					   bool high_prio)
> +static inline void add_lat_percentile_prio_sample(struct thread_stat *ts,
> +						  unsigned long long nsec,
> +						  enum fio_ddir ddir,
> +						  bool high_prio)
>  {
>  	unsigned int idx = plat_val_to_idx(nsec);
>  
> 

With the commit message typo fixed,

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research