On 2021/11/09 9:28, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> Move common cmdprio_prep() code to cmdprio.c to avoid code duplication.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> ---
> engines/cmdprio.c | 40 +++++++++++++++++++++++++++++++++++++++-
> engines/cmdprio.h | 3 ++-
> engines/io_uring.c | 32 +++++---------------------------
> engines/libaio.c | 26 ++++----------------------
> 4 files changed, 50 insertions(+), 51 deletions(-)
>
> diff --git a/engines/cmdprio.c b/engines/cmdprio.c
> index 2c764e49..01e0a729 100644
> --- a/engines/cmdprio.c
> +++ b/engines/cmdprio.c
> @@ -67,7 +67,7 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
> return ret;
> }
>
> -int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
> +static int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
> {
> enum fio_ddir ddir = io_u->ddir;
> unsigned int p = cmdprio->percentage[ddir];
> @@ -89,6 +89,44 @@ int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
> return 0;
> }
>
> +/**
> + * fio_cmdprio_ioprio_was_updated - Generates a random value. If the random
> + * value is within the user specified percentage of I/Os that should use a
> + * cmdprio priority value (rather than the default priority), then this
> + * function updates the io_u with a cmdprio priority value.
> + */
> +bool fio_cmdprio_ioprio_was_updated(struct thread_data *td,
> + struct cmdprio *cmdprio, struct io_u *io_u)
What about simply calling this fio_set_cmdprio() or fio_cmdprio_set_ioprio() ?
That is a lot shorter and the name matches the return true/false depending on if
the io_u->ioprio was set (changed) or not.
> +{
> + enum fio_ddir ddir = io_u->ddir;
> + unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
> + unsigned int cmdprio_value =
> + ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
> +
> + if (p && rand_between(&td->prio_state, 0, 99) < p) {
> + io_u->ioprio = cmdprio_value;
> + if (!td->ioprio || cmdprio_value < td->ioprio) {
> + /*
> + * The async IO priority is higher (has a lower value)
> + * than the default priority (which is either 0 or the
> + * value set by "prio" and "prioclass" options).
> + */
> + io_u->flags |= IO_U_F_HIGH_PRIO;
> + }
> + return true;
> + } else if (td->ioprio && td->ioprio < cmdprio_value) {
No need for the else here.
> + /*
> + * The IO will be executed with the default priority (which is
> + * either 0 or the value set by "prio" and "prioclass options),
> + * and this priority is higher (has a lower value) than the
> + * async IO priority.
> + */
> + io_u->flags |= IO_U_F_HIGH_PRIO;
> + }
> +
> + return false;
> +}
> +
> int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> bool *has_cmdprio)
> {
> diff --git a/engines/cmdprio.h b/engines/cmdprio.h
> index d3265741..c05679e4 100644
> --- a/engines/cmdprio.h
> +++ b/engines/cmdprio.h
> @@ -22,7 +22,8 @@ struct cmdprio {
> int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
> struct cmdprio *cmdprio);
>
> -int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u);
> +bool fio_cmdprio_ioprio_was_updated(struct thread_data *td,
> + struct cmdprio *cmdprio, struct io_u *io_u);
>
> int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> bool *has_cmdprio);
> diff --git a/engines/io_uring.c b/engines/io_uring.c
> index de1e6cc9..46f4bc2a 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -458,35 +458,13 @@ static int fio_ioring_getevents(struct thread_data *td, unsigned int min,
>
> static void fio_ioring_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
> {
> - struct ioring_options *o = td->eo;
> struct ioring_data *ld = td->io_ops_data;
> - struct io_uring_sqe *sqe = &ld->sqes[io_u->index];
> + struct ioring_options *o = td->eo;
> struct cmdprio *cmdprio = &o->cmdprio;
> - enum fio_ddir ddir = io_u->ddir;
> - unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
> - unsigned int cmdprio_value =
> - ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
> -
> - if (p && rand_between(&td->prio_state, 0, 99) < p) {
> - io_u->ioprio = cmdprio_value;
> - sqe->ioprio = cmdprio_value;
> - if (!td->ioprio || cmdprio_value < td->ioprio) {
> - /*
> - * The async IO priority is higher (has a lower value)
> - * than the default priority (which is either 0 or the
> - * value set by "prio" and "prioclass" options).
> - */
> - io_u->flags |= IO_U_F_HIGH_PRIO;
> - }
> - } else if (td->ioprio && td->ioprio < cmdprio_value) {
> - /*
> - * The IO will be executed with the default priority (which is
> - * either 0 or the value set by "prio" and "prioclass options),
> - * and this priority is higher (has a lower value) than the
> - * async IO priority.
> - */
> - io_u->flags |= IO_U_F_HIGH_PRIO;
> - }
> + bool ioprio_updated = fio_cmdprio_ioprio_was_updated(td, cmdprio, io_u);
> +
> + if (ioprio_updated)
> + ld->sqes[io_u->index].ioprio = io_u->ioprio;
I do not see why ioprio_updated is needed. Also, since the sqes initialization
also needs to be done with the defualt priority, can't this be moved outside of
this function to be common with the default prio case ?
> }
>
> static enum fio_q_status fio_ioring_queue(struct thread_data *td,
> diff --git a/engines/libaio.c b/engines/libaio.c
> index 9c14dd88..f0d3df7a 100644
> --- a/engines/libaio.c
> +++ b/engines/libaio.c
> @@ -209,29 +209,11 @@ static void fio_libaio_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
> {
> struct libaio_options *o = td->eo;
> struct cmdprio *cmdprio = &o->cmdprio;
> - enum fio_ddir ddir = io_u->ddir;
> - unsigned int p = fio_cmdprio_percentage(cmdprio, io_u);
> - unsigned int cmdprio_value =
> - ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
> -
> - if (p && rand_between(&td->prio_state, 0, 99) < p) {
> - io_u->ioprio = cmdprio_value;
> - io_u->iocb.aio_reqprio = cmdprio_value;
> + bool ioprio_updated = fio_cmdprio_ioprio_was_updated(td, cmdprio, io_u);
> +
> + if (ioprio_updated) {
With fio_cmdprio_ioprio_was_updated() renamed as proposed above, I think you can
get rid of the local boolean and simply do:
if (fio_cmdprio_set_ioprio(td, cmdprio, io_u)) {
io_u->iocb.aio_reqprio = io_u->ioprio;
io_u->iocb.u.c.flags |= IOCB_FLAG_IOPRIO;
}
And I wonder if the fio_libaio_cmdprio_prep() helper is really useful given that
it is reduced to the above tiny code. Same for io_uring.
> + io_u->iocb.aio_reqprio = io_u->ioprio;
> io_u->iocb.u.c.flags |= IOCB_FLAG_IOPRIO;
> - if (!td->ioprio || cmdprio_value < td->ioprio) {
> - /*
> - * The async IO priority is higher (has a lower value)
> - * than the default context priority.
> - */
> - io_u->flags |= IO_U_F_HIGH_PRIO;
> - }
> - } else if (td->ioprio && td->ioprio < cmdprio_value) {
> - /*
> - * The IO will be executed with the default context priority,
> - * and this priority is higher (has a lower value) than the
> - * async IO priority.
> - */
> - io_u->flags |= IO_U_F_HIGH_PRIO;
> }
> }
>
>
--
Damien Le Moal
Western Digital Research
