Re: [PATCH 4/4] stats: Add hint information to per priority level stats

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Thu, Jul 06, 2023 at 07:13:02AM +0900, Damien Le Moal wrote:
> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
> Introduce the OS dependent ioprio_hint() macro to extract an IO
> priority hint from an ioprio value. This macro defaults to always
> returning 0 for OSes that do not support IO priority hints.
> 
> The json and standard output stats are modified to display the hint
> value together with the priority class and level.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  os/os-linux.h |  4 ++++
>  os/os.h       |  3 +++
>  stat.c        | 10 +++++++---
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/os/os-linux.h b/os/os-linux.h
> index 6f241d09..b9c07891 100644
> --- a/os/os-linux.h
> +++ b/os/os-linux.h
> @@ -150,6 +150,10 @@ static inline int ioprio_value(int ioprio_class, int ioprio, int ioprio_hint)
>  		ioprio;
>  }
>  
> +#define ioprio_class(ioprio)	((ioprio) >> IOPRIO_CLASS_SHIFT)
> +#define ioprio_hint(ioprio)	(((ioprio) >> IOPRIO_HINT_SHIFT) & 0x3ff)
> +#define ioprio(ioprio)		((ioprio) & 7)

1)
ioprio_class() and ioprio() are already defined in os/os-linux.h,
so if you add these again, we will have duplicates in os/os-linux.h.
You probably only want to add ioprio_hint(), after the existing defines.


2)
Perhaps you can also change:
#define ioprio(ioprio)          ((ioprio) & 7)
to
#define ioprio(ioprio)          ((ioprio) & IOPRIO_MAX_PRIO)

and
#define ioprio_hint(ioprio)  (((ioprio) >> IOPRIO_HINT_SHIFT) & 0x3ff)
to
#define ioprio_hint(ioprio)  (((ioprio) >> IOPRIO_HINT_SHIFT) & IOPRIO_MAX_PRIO_HINT)

so we avoid magic numbers.


> +
>  static inline bool ioprio_value_is_class_rt(unsigned int priority)
>  {
>  	return (priority >> IOPRIO_CLASS_SHIFT) == IOPRIO_CLASS_RT;
> diff --git a/os/os.h b/os/os.h
> index 2217d5f8..0f182324 100644
> --- a/os/os.h
> +++ b/os/os.h
> @@ -120,6 +120,9 @@ extern int fio_cpus_split(os_cpu_mask_t *mask, unsigned int cpu);
>  #define ioprio_value_is_class_rt(prio)	(false)
>  #define IOPRIO_MIN_PRIO_CLASS		0
>  #define IOPRIO_MAX_PRIO_CLASS		0
> +#define ioprio_hint(prio)		0

> +#define IOPRIO_MIN_PRIO_HINT		0
> +#define IOPRIO_MAX_PRIO_HINT		0

3)
The MIN_ and MAX_ define should probably go to patch 1/4,
which added the same for os/os-linux.h.


>  #endif
>  #ifndef FIO_HAVE_IOPRIO
>  #define ioprio_value(prioclass, prio, priohint)	(0)
> diff --git a/stat.c b/stat.c
> index 015b8e28..4f943602 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -597,10 +597,11 @@ static void show_ddir_status(struct group_run_stats *rs, struct thread_stat *ts,
>  				continue;
>  
>  			snprintf(buf, sizeof(buf),
> -				 "%s prio %u/%u",
> +				 "%s prio %u/%u/%u",
>  				 clat_type,
>  				 ioprio_class(ts->clat_prio[ddir][i].ioprio),
> -				 ioprio(ts->clat_prio[ddir][i].ioprio));
> +				 ioprio(ts->clat_prio[ddir][i].ioprio),
> +				 ioprio_hint(ts->clat_prio[ddir][i].ioprio));
>  			display_lat(buf, min, max, mean, dev, out);
>  		}
>  	}
> @@ -640,10 +641,11 @@ static void show_ddir_status(struct group_run_stats *rs, struct thread_stat *ts,
>  					continue;
>  
>  				snprintf(prio_name, sizeof(prio_name),
> -					 "%s prio %u/%u (%.2f%% of IOs)",
> +					 "%s prio %u/%u/%u (%.2f%% of IOs)",
>  					 clat_type,
>  					 ioprio_class(ts->clat_prio[ddir][i].ioprio),
>  					 ioprio(ts->clat_prio[ddir][i].ioprio),
> +					 ioprio_hint(ts->clat_prio[ddir][i].ioprio),
>  					 100. * (double) prio_samples / (double) samples);
>  				show_clat_percentiles(ts->clat_prio[ddir][i].io_u_plat,
>  						prio_samples, ts->percentile_list,
> @@ -1522,6 +1524,8 @@ static void add_ddir_status_json(struct thread_stat *ts,
>  				ioprio_class(ts->clat_prio[ddir][i].ioprio));
>  			json_object_add_value_int(obj, "prio",
>  				ioprio(ts->clat_prio[ddir][i].ioprio));
> +			json_object_add_value_int(obj, "priohint",
> +				ioprio_hint(ts->clat_prio[ddir][i].ioprio));
>  
>  			tmp_object = add_ddir_lat_json(ts,
>  					ts->clat_percentiles | ts->lat_percentiles,
> -- 
> 2.41.0
> 

With the three nits fixed:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>