On 2021/11/09 9:28, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> Add a new field "mode", in order to know if we are using/running
> cmdprio in cmdprio_percentage or cmdprio_bssplit mode.
...if we are determining IO priorities according to cmdprio_percentage or to
cmdprio_bssplit.
(I think that is clearer)
>
> 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, 39 insertions(+), 30 deletions(-)
>
> diff --git a/engines/cmdprio.c b/engines/cmdprio.c
> index 01e0a729..cafe21d7 100644
> --- a/engines/cmdprio.c
> +++ b/engines/cmdprio.c
> @@ -70,17 +70,12 @@ 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.
> - */
Personally, I would keep the comment. It makes it very clear, in human natural
langage, what the relation between cmdprio_percentage and cmdprio_bssplit is.
That is, cmdprio_percentage has precedence.
> - if (p)
> - return p;
> + if (cmdprio->mode == CMDPRIO_MODE_PERC)
> + return cmdprio->percentage[ddir];
>
> + assert(cmdprio->mode == CMDPRIO_MODE_BSSPLIT);
Is this really necessary ? What about using switch()/case with a default that
has the assert for debugging ? This way, the assert will not be uselessly tested
whenever cmdprio_bssplit is used.
> for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
> if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
> return cmdprio->bssplit[ddir][i].perc;
> @@ -99,10 +94,20 @@ bool fio_cmdprio_ioprio_was_updated(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;
> + }
> +
> + 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) {
> @@ -127,8 +132,7 @@ bool fio_cmdprio_ioprio_was_updated(struct thread_data *td,
> 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;
> @@ -140,16 +144,12 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
> * is not set, default to RT priority class.
> */
> for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
> - if (cmdprio->percentage[i]) {
> - if (!cmdprio->class[i])
> - cmdprio->class[i] = IOPRIO_CLASS_RT;
Why do you change this ? If cmdprio->percentage[i] is 0, you do not want to set
the calss to RT. Or is it that cmdprio->percentage[i] cannot be 0 ? (I did not
check the range of the option).
> + if (!cmdprio->class[i])
> + cmdprio->class[i] = IOPRIO_CLASS_RT;
> + if (cmdprio->percentage[i])
> has_cmdprio_percentage = true;
> - }
> - if (cmdprio->bssplit_nr[i]) {
> - if (!cmdprio->class[i])
> - cmdprio->class[i] = IOPRIO_CLASS_RT;
> + if (cmdprio->bssplit_nr[i])
> has_cmdprio_bssplit = true;
> - }
> }
>
> /*
> @@ -162,7 +162,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 c05679e4..7d14b3a6 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_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);
> +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 46f4bc2a..f82fa1fd 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 f0d3df7a..b33461f4 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;
>
--
Damien Le Moal
Western Digital Research
