From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: "axboe@kernel.dk" <axboe@kernel.dk>,
"fio@vger.kernel.org" <fio@vger.kernel.org>,
Damien Le Moal <Damien.LeMoal@wdc.com>
Subject: Re: [PATCH 7/8] cmdprio: add mode to make the logic easier to reason about
Date: Tue, 9 Nov 2021 13:29:34 +0000 [thread overview]
Message-ID: <YYp3vESWV74In8Eo@x1-carbon> (raw)
In-Reply-To: <e9dc1315-540e-f69f-6814-f8c95a2c077e@opensource.wdc.com>
On Tue, Nov 09, 2021 at 03:38:36PM +0900, Damien Le Moal wrote:
> On 2021/11/09 9:28, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > 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)
Will update.
>
> >
> > 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@wdc.com>
> > ---
> > 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.
No, cmdprio_percentage does not have precedence.
Since cmdprio_percentage and cmdprio_bssplit is mutually exclusive,
what we should use simply depends on the mode.
I will update it to a switch, like you suggest below to make things
more clear.
>
> > - 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.
Will do.
>
> > 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).
option cmdprio_percentage has minval 0.
option cmdprio_class has minval 1, however it can still be 0 if the user
omitted the option and only used option cmdprio to specify a priority level.
I was thinking that cmdprio_value is only used when the percentage != 0,
which is the case in my cmdprio branch that allows you to have different
cmdprio class + levels, but you are right, in this specific patch
fio_cmdprio_ioprio_was_updated()/fio_cmdprio_set_ioprio() still uses
cmdprio->class to determine if IO_U_F_HIGH_PRIO flag should be set,
even when fio_cmdprio_percentage() returned zero..
The question is, if percentage == 0, for e.g. writes,
but the user specified cmdprio=3
(which sets cmdprio->level[] for both DDIR read and write),
should we set the HIGH_PRIO / LOW_PRIO flag?
I think the best way is to simply not set any of the flags,
If cmdprio percentage is 0 for a certain ddir, then everything
will be sent with the default prio.
So I will keep the unconditional reassignment of cmdprio->class[]
here, since it shouldn't matter. If the percentage is 0 for a ddir,
the value should never be used anyway.
Will add a if (!p) return false;
after the p = fio_cmdprio_percentage(cmdprio, io_u);
assignment that this patch does instead.
Kind regards,
Niklas
>
> > + 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
next prev parent reply other threads:[~2021-11-09 13:29 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-09 0:28 [PATCH 0/8] cmdprio cleanup series Niklas Cassel
2021-11-09 0:28 ` [PATCH 1/8] docs: update cmdprio_percentage documentation Niklas Cassel
2021-11-09 6:09 ` Damien Le Moal
2021-11-09 0:28 ` [PATCH 2/8] cmdprio: move cmdprio function definitions to a new cmdprio.c file Niklas Cassel
2021-11-09 6:11 ` Damien Le Moal
2021-11-09 0:28 ` [PATCH 3/8] cmdprio: do not allocate memory for unused data direction Niklas Cassel
2021-11-09 6:12 ` Damien Le Moal
2021-11-09 0:28 ` [PATCH 5/8] libaio,io_uring: rename prio_prep() to include cmdprio in the name Niklas Cassel
2021-11-09 6:19 ` Damien Le Moal
2021-11-09 0:28 ` [PATCH 4/8] io_uring: set async IO priority to td->ioprio in fio_ioring_prep() Niklas Cassel
2021-11-09 6:16 ` Damien Le Moal
2021-11-09 0:28 ` [PATCH 6/8] libaio,io_uring: move common cmdprio_prep() code to cmdprio Niklas Cassel
2021-11-09 6:29 ` Damien Le Moal
2021-11-09 10:43 ` Niklas Cassel
2021-11-09 0:28 ` [PATCH 7/8] cmdprio: add mode to make the logic easier to reason about Niklas Cassel
2021-11-09 6:38 ` Damien Le Moal
2021-11-09 13:29 ` Niklas Cassel [this message]
2021-11-09 23:17 ` Niklas Cassel
2021-11-10 5:57 ` Damien Le Moal
2021-11-10 11:34 ` Niklas Cassel
2021-11-09 0:28 ` [PATCH 8/8] libaio,io_uring: make it possible to cleanup cmdprio malloced data Niklas Cassel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YYp3vESWV74In8Eo@x1-carbon \
--to=niklas.cassel@wdc.com \
--cc=Damien.LeMoal@wdc.com \
--cc=axboe@kernel.dk \
--cc=damien.lemoal@opensource.wdc.com \
--cc=fio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox