Re: [PATCH 6/8] libaio,io_uring: move common cmdprio_prep() code to cmdprio

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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@wdc.com>
> >
> > Move common cmdprio_prep() code to cmdprio.c to avoid code duplication.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >  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