On 2021/11/10 8:38, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> Add a new field "mode", in order to know if we are determining IO
> priorities according to cmdprio_percentage or to cmdprio_bssplit.
>
> This makes the logic easier to reason about, and allows us to
> remove the "use_cmdprio" variable from the ioengines themselves.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> ---
> engines/cmdprio.c | 45 +++++++++++++++++++++++++++++----------------
> engines/cmdprio.h | 10 ++++++++--
> engines/io_uring.c | 7 +++----
> engines/libaio.c | 7 +++----
> 4 files changed, 43 insertions(+), 26 deletions(-)
>
> diff --git a/engines/cmdprio.c b/engines/cmdprio.c
> index 9cad76b5..174bed7c 100644
> --- a/engines/cmdprio.c
> +++ b/engines/cmdprio.c
> @@ -70,20 +70,19 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
> 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];
> int i;
>
> - /*
> - * If cmdprio_percentage option was specified, then use that
> - * percentage. Otherwise, use cmdprio_bssplit percentages depending
> - * on the IO size.
> - */
> - if (p)
> - return p;
> -
> - for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
> - if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
> - return cmdprio->bssplit[ddir][i].perc;
> + switch (cmdprio->mode) {
> + case CMDPRIO_MODE_PERC:
> + return cmdprio->percentage[ddir];
> + case CMDPRIO_MODE_BSSPLIT:
> + for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
> + if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
> + return cmdprio->bssplit[ddir][i].perc;
> + }
> + break;
> + default:
> + assert(0);
> }
>
> return 0;
> @@ -99,10 +98,20 @@ 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 p;
> unsigned int cmdprio_value =
> ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
>
> + if (cmdprio->mode == CMDPRIO_MODE_NONE) {
> + /*
> + * An I/O engine should never call this function if cmdprio
> + * is not is use.
> + */
> + assert(0);
> + return false;
> + }
This hunk is not useful as the call to fio_cmdprio_percentage() will cover this
buggy case. So let's remove it. No need to clutter the code with it. You may
want to move the above comment under the if to the default case in
fio_cmdprio_percentage() switch.
> +
> + p = fio_cmdprio_percentage(cmdprio, io_u);
> if (p && rand_between(&td->prio_state, 0, 99) < p) {
> io_u->ioprio = cmdprio_value;
> if (!td->ioprio || cmdprio_value < td->ioprio) {
> @@ -128,8 +137,7 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
> return false;
> }
>
> -int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> - bool *has_cmdprio)
> +int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
> {
> struct thread_options *to = &td->o;
> bool has_cmdprio_percentage = false;
> @@ -163,7 +171,12 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> return 1;
> }
>
> - *has_cmdprio = has_cmdprio_percentage || has_cmdprio_bssplit;
> + if (has_cmdprio_bssplit)
> + cmdprio->mode = CMDPRIO_MODE_BSSPLIT;
> + else if (has_cmdprio_percentage)
> + cmdprio->mode = CMDPRIO_MODE_PERC;
> + else
> + cmdprio->mode = CMDPRIO_MODE_NONE;
>
> return 0;
> }
> diff --git a/engines/cmdprio.h b/engines/cmdprio.h
> index 732b087e..7e4fcf6c 100644
> --- a/engines/cmdprio.h
> +++ b/engines/cmdprio.h
> @@ -11,12 +11,19 @@
> /* read and writes only, no trim */
> #define CMDPRIO_RWDIR_CNT 2
>
> +enum {
> + CMDPRIO_MODE_NONE,
> + CMDPRIO_MODE_PERC,
> + CMDPRIO_MODE_BSSPLIT,
> +};
> +
> struct cmdprio {
> unsigned int percentage[CMDPRIO_RWDIR_CNT];
> unsigned int class[CMDPRIO_RWDIR_CNT];
> unsigned int level[CMDPRIO_RWDIR_CNT];
> unsigned int bssplit_nr[CMDPRIO_RWDIR_CNT];
> struct bssplit *bssplit[CMDPRIO_RWDIR_CNT];
> + unsigned int mode;
> };
>
> int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
> @@ -25,7 +32,6 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
> 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);
> +int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio);
>
> #endif
> diff --git a/engines/io_uring.c b/engines/io_uring.c
> index 230f08a5..979d07d4 100644
> --- a/engines/io_uring.c
> +++ b/engines/io_uring.c
> @@ -68,8 +68,6 @@ struct ioring_data {
> int prepped;
>
> struct ioring_mmap mmap[3];
> -
> - bool use_cmdprio;
> };
>
> struct ioring_options {
> @@ -472,6 +470,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
> {
> struct ioring_data *ld = td->io_ops_data;
> struct io_sq_ring *ring = &ld->sq_ring;
> + struct ioring_options *o = td->eo;
> unsigned tail, next_tail;
>
> fio_ro_check(td, io_u);
> @@ -494,7 +493,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
> if (next_tail == atomic_load_acquire(ring->head))
> return FIO_Q_BUSY;
>
> - if (ld->use_cmdprio)
> + if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
> fio_ioring_cmdprio_prep(td, io_u);
>
> ring->array[tail & ld->sq_ring_mask] = io_u->index;
> @@ -831,7 +830,7 @@ static int fio_ioring_init(struct thread_data *td)
>
> td->io_ops_data = ld;
>
> - ret = fio_cmdprio_init(td, cmdprio, &ld->use_cmdprio);
> + ret = fio_cmdprio_init(td, cmdprio);
> if (ret) {
> td_verror(td, EINVAL, "fio_ioring_init");
> return 1;
> diff --git a/engines/libaio.c b/engines/libaio.c
> index dc6ad08e..53fe7428 100644
> --- a/engines/libaio.c
> +++ b/engines/libaio.c
> @@ -51,8 +51,6 @@ struct libaio_data {
> unsigned int queued;
> unsigned int head;
> unsigned int tail;
> -
> - bool use_cmdprio;
> };
>
> struct libaio_options {
> @@ -320,6 +318,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
> struct io_u *io_u)
> {
> struct libaio_data *ld = td->io_ops_data;
> + struct libaio_options *o = td->eo;
>
> fio_ro_check(td, io_u);
>
> @@ -350,7 +349,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
> return FIO_Q_COMPLETED;
> }
>
> - if (ld->use_cmdprio)
> + if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
> fio_libaio_cmdprio_prep(td, io_u);
>
> ld->iocbs[ld->head] = &io_u->iocb;
> @@ -507,7 +506,7 @@ static int fio_libaio_init(struct thread_data *td)
>
> td->io_ops_data = ld;
>
> - ret = fio_cmdprio_init(td, cmdprio, &ld->use_cmdprio);
> + ret = fio_cmdprio_init(td, cmdprio);
> if (ret) {
> td_verror(td, EINVAL, "fio_libaio_init");
> return 1;
>
With the above fix, this is a nice cleanup.
Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
