From: Damien Le Moal <dlemoal@kernel.org>
To: Jens Axboe <axboe@kernel.dk>,
fio@vger.kernel.org, Vincent Fu <vincentfu@gmail.com>
Cc: Niklas Cassel <niklas.cassel@wdc.com>
Subject: Re: [PATCH v2 4/5] cmdprio: Add support for per I/O priority hint
Date: Fri, 21 Jul 2023 10:07:09 +0900 [thread overview]
Message-ID: <ac531535-fad3-c336-4a7c-2dda04c3bae4@kernel.org> (raw)
In-Reply-To: <7bf0279a-0d54-03ab-15a0-84ae1b89e62e@kernel.dk>
On 7/21/23 04:36, Jens Axboe wrote:
> On 7/19/23 7:31?PM, Damien Le Moal wrote:
>> diff --git a/engines/io_uring.c b/engines/io_uring.c
>> index f30a3c00..d55af6bc 100644
>> --- a/engines/io_uring.c
>> +++ b/engines/io_uring.c
>> @@ -157,6 +157,21 @@ static struct fio_option options[] = {
>> .category = FIO_OPT_C_ENGINE,
>> .group = FIO_OPT_G_IOURING,
>> },
>> + {
>> + .name = "cmdprio_hint",
>> + .lname = "Asynchronous I/O priority hint",
>> + .type = FIO_OPT_INT,
>> + .off1 = offsetof(struct ioring_options,
>> + cmdprio_options.hint[DDIR_READ]),
>> + .off2 = offsetof(struct ioring_options,
>> + cmdprio_options.hint[DDIR_WRITE]),
>> + .help = "Set asynchronous IO priority hint",
>> + .minval = IOPRIO_MIN_PRIO_HINT,
>> + .maxval = IOPRIO_MAX_PRIO_HINT,
>> + .interval = 1,
>> + .category = FIO_OPT_C_ENGINE,
>> + .group = FIO_OPT_G_IOURING,
>> + },
>> {
>> .name = "cmdprio",
>> .lname = "Asynchronous I/O priority level",
>> @@ -195,6 +210,12 @@ static struct fio_option options[] = {
>> .type = FIO_OPT_UNSUPPORTED,
>> .help = "Your platform does not support I/O priority classes",
>> },
>> + {
>> + .name = "cmdprio_hint",
>> + .lname = "Asynchronous I/O priority hint",
>> + .type = FIO_OPT_UNSUPPORTED,
>> + .help = "Your platform does not support I/O priority classes",
>> + },
>> {
>> .name = "cmdprio",
>> .lname = "Asynchronous I/O priority level",
>
> Since neither this nor libaio actually do anything with these values,
> why isn't this just a generic option? You could flag the two engines
> where it actually works with an engine flag, and check if it's set and
> we don't have that flag and error out in the generic code.
Hmmm. I would like to keep the async IO prio option definitions and storage
together with the cmdprio_options struct, which is common to both libaio and
iouring engines. If we move the hint option definition to be a generic one, we
cannot use that struct and would need a job field. I can make that work, but
that makes the code a little messier by spreading out the controls for async IO
prio.
I agree that we can drop the error output though as indeed the hint value is
unused on systems without hint support. But warning the user about the lack of
support is nice I think.
Checking the code again though, one thing I noticed is that iouring and libaio
cmdprio_xxx options definition is identical, modulo the group. So all these
definitions could be moved to engines/cmdprio.h as macros and IO engines
supporting these options can use these to add the options. The other option is
to move the definition of all these options to be generic ones and use the
engine flag trick you mention to indicate support.
--
Damien Le Moal
Western Digital Research
next prev parent reply other threads:[~2023-07-21 1:07 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-20 1:31 [PATCH v2 0/5] Add support for I/O priority hints Damien Le Moal
2023-07-20 1:31 ` [PATCH v2 1/5] os-linux: Cleanup IO priority class and value macros Damien Le Moal
2023-07-20 9:18 ` Niklas Cassel
2023-07-20 1:31 ` [PATCH v2 2/5] os-linux: add initial support for IO priority hints Damien Le Moal
2023-07-20 1:31 ` [PATCH v2 3/5] options: add priohint option Damien Le Moal
2023-07-20 19:33 ` Jens Axboe
2023-07-20 1:31 ` [PATCH v2 4/5] cmdprio: Add support for per I/O priority hint Damien Le Moal
2023-07-20 19:36 ` Jens Axboe
2023-07-21 1:07 ` Damien Le Moal [this message]
2023-07-20 1:31 ` [PATCH v2 5/5] stats: Add hint information to per priority level stats Damien Le Moal
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=ac531535-fad3-c336-4a7c-2dda04c3bae4@kernel.org \
--to=dlemoal@kernel.org \
--cc=axboe@kernel.dk \
--cc=fio@vger.kernel.org \
--cc=niklas.cassel@wdc.com \
--cc=vincentfu@gmail.com \
/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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).