On Thu, Jul 06, 2023 at 07:13:02AM +0900, Damien Le Moal wrote:
> From: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxx>
