On 2021/11/10 8:38, 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 | 41 ++++++++++++++++++++++++++++++++++++++++-
> engines/cmdprio.h | 3 ++-
> engines/io_uring.c | 34 ++++++----------------------------
> engines/libaio.c | 28 +++++-----------------------
> 4 files changed, 53 insertions(+), 53 deletions(-)
>
> diff --git a/engines/cmdprio.c b/engines/cmdprio.c
> index 2c764e49..9cad76b5 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,45 @@ int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
> return 0;
> }
>
> +/**
> + * fio_cmdprio_set_ioprio - 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.
> + */
This comment is not very precise in its description. What about this:
/**
* fio_cmdprio_set_ioprio - Set an io_u ioprio according to cmdprio options
*
* Generates a random percentage value to determine if an io_u ioprio needs
* to be set. If the random percentage 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 an ioprio
* value as defined by the cmdprio/cdmprio_class or cmdprio_bssplit options.
*
* Return true if the io_u ioprio was changed and false otherwise.
*/
> +bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
> + struct io_u *io_u)
> +{
> + 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;
> + }
Nit: a blank line here would be nice, for readability.
> + 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;
> + }
> +
> + 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..732b087e 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_set_ioprio(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 6f90e0a7..230f08a5 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -456,37 +456,15 @@ static int fio_ioring_getevents(struct thread_data *td, unsigned int min,
> return r < 0 ? r : events;
> }
>
> -static void fio_ioring_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
> +static inline 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;
> - }
> +
> + if (fio_cmdprio_set_ioprio(td, cmdprio, io_u))
> + ld->sqes[io_u->index].ioprio = io_u->ioprio;
> }
>
> static enum fio_q_status fio_ioring_queue(struct thread_data *td,
> diff --git a/engines/libaio.c b/engines/libaio.c
> index 9c14dd88..dc6ad08e 100644
> --- a/engines/libaio.c
> +++ b/engines/libaio.c
> @@ -205,33 +205,15 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
> return 0;
> }
>
> -static void fio_libaio_cmdprio_prep(struct thread_data *td, struct io_u *io_u)
> +static inline 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;
> +
> + 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;
> - 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;
> }
> }
>
>
With the above comment fix, looks good.
Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
