On Tue, Nov 09, 2021 at 03:29:33PM +0900, Damien Le Moal wrote:
> 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.
Yes, that sounds like a slightly better name.
> > @@ -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 ?
I'm quite sure that the compiler will optimize this and generate the same code,
but sure, I will remove the boolean.
Regarding moving it, I assume that the original author (of cmdprio_percentage)
who put the prio_prep() call in _queue() rather than _prep() had a good reason
to do so. E.g. the _prio_prep() call is after the busy check.
The most important thing to me is that the call to fio_ioring_cmdprio_prep() is
done in the same callback function for libaio and io_uring. So I don't want it
to be in _prep() for io_uring and to be in _queue() for libaio. Also looking at
libaio, it doesn't like I can add it to _prep() in a nice way, I would need to
add it to if (io_u->ddir == DDIR_READ) and if (io_u->ddir == DDIR_WRITE), this
alone make me want to keep the fio_ioring_cmdprio_prep() call in _queue().
So, all in all, I'd rather keep the call to _prio_prep() in _queue() for both
ioengines for now.
>
> > }
> >
> > 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;
> }
Yes, I will rename it, and like you suggested for io_uring, I will remove the
bool ioprio_updated.
>
> 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.
Since I'd rather keep the call in _queue() for both libaio and io_uring,
I guess we could either keep the function as static inline, or do something
like the following in _queue() (this is how it would look after patch 8/8):
@@ -341,8 +341,11 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
return FIO_Q_COMPLETED;
}
- if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
- fio_libaio_cmdprio_prep(td, io_u);
+ if (ld->cmdprio.mode != CMDPRIO_MODE_NONE &&
+ fio_cmdprio_set_ioprio(td, &ld->cmdprio, io_u)) {
+ io_u->iocb.aio_reqprio = io_u->ioprio;
+ io_u->iocb.u.c.flags |= IOCB_FLAG_IOPRIO;
+ }
Personally, I don't think that the ++ looks better than the --,
so I would prefer to keep the function, but make it static inline.
Kind regards,
Niklas
