From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: "axboe@kernel.dk" <axboe@kernel.dk>
Cc: "fio@vger.kernel.org" <fio@vger.kernel.org>,
"damien.lemoal@opensource.wdc.com"
<damien.lemoal@opensource.wdc.com>,
Niklas Cassel <Niklas.Cassel@wdc.com>
Subject: [PATCH v3 8/8] libaio,io_uring: make it possible to cleanup cmdprio malloced data
Date: Fri, 12 Nov 2021 09:54:44 +0000 [thread overview]
Message-ID: <20211112095428.158300-9-Niklas.Cassel@wdc.com> (raw)
In-Reply-To: <20211112095428.158300-1-Niklas.Cassel@wdc.com>
From: Niklas Cassel <niklas.cassel@wdc.com>
The way that fio currently handles engine options:
options_free() will call free() only for options that have the type
FIO_OPT_STR_STORE. This means that any option that has a pointer in
either td->o or td->eo, which is not of type FIO_OPT_STR_STORE will
leak memory. This is true even for numjobs == 1.
When running with numjobs > 1, fio_options_mem_dupe() will memcpy
td->eo into the new td. Since off1 of the pointers in the first td
has already been set, the pointers in the new td will point to the
same data. (Regardless, options_free() will never try to free the
memory, for neither td.) Neither can we manually free the memory in
cleanup(), since the other td will still point to the same memory,
so this would lead to a double free.
These memory leaks are reported by e.g. valgrind.
The most obvious way to solve this is to put dynamically allocated
memory in {ioring,libaio}_data instead of {ioring,libaio}_options.
This solves the problem since {ioring,libaio}_data is dynamically
allocated by each td during the ioengine init callback, and is freed
when the ioengine cleanup callback for that td is called.
The downside of this is that the parsing has to be done in
fio_cmdprio_init() instead of in the option .cb callback, since the
.cb callback is called before {ioring,libaio}_data is available.
This patch keeps the static cmdprio options in
{ioring,libaio}_options, but moves the dynamically allocated memory
needed by cmdprio to {ioring,libaio}_data.
No cmdprio related memory leaks are reported after this patch.
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
engines/cmdprio.c | 114 ++++++++++++++++++++++++++++++++++-----------
engines/cmdprio.h | 15 ++++--
engines/io_uring.c | 41 +++++++---------
engines/libaio.c | 43 +++++++----------
4 files changed, 131 insertions(+), 82 deletions(-)
diff --git a/engines/cmdprio.c b/engines/cmdprio.c
index 8c52dabd..92b752ae 100644
--- a/engines/cmdprio.c
+++ b/engines/cmdprio.c
@@ -46,22 +46,15 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
struct cmdprio *cmdprio)
{
char *str, *p;
- int i, ret = 0;
+ int ret = 0;
p = str = strdup(input);
strip_blank_front(&str);
strip_blank_end(str);
- ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio, false);
-
- if (parse_dryrun()) {
- for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
- free(cmdprio->bssplit[i]);
- cmdprio->bssplit[i] = NULL;
- cmdprio->bssplit_nr[i] = 0;
- }
- }
+ ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio,
+ false);
free(p);
return ret;
@@ -70,11 +63,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;
+ struct cmdprio_options *options = cmdprio->options;
int i;
switch (cmdprio->mode) {
case CMDPRIO_MODE_PERC:
- return cmdprio->percentage[ddir];
+ return options->percentage[ddir];
case CMDPRIO_MODE_BSSPLIT:
for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
@@ -107,9 +101,10 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
struct io_u *io_u)
{
enum fio_ddir ddir = io_u->ddir;
+ struct cmdprio_options *options = cmdprio->options;
unsigned int p;
unsigned int cmdprio_value =
- ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
+ ioprio_value(options->class[ddir], options->level[ddir]);
p = fio_cmdprio_percentage(cmdprio, io_u);
if (p && rand_between(&td->prio_state, 0, 99) < p) {
@@ -138,30 +133,91 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
return false;
}
-int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
+static int fio_cmdprio_parse_and_gen_bssplit(struct thread_data *td,
+ struct cmdprio *cmdprio)
{
- struct thread_options *to = &td->o;
- bool has_cmdprio_percentage = false;
- bool has_cmdprio_bssplit = false;
- int i;
+ struct cmdprio_options *options = cmdprio->options;
+ int ret;
+
+ ret = fio_cmdprio_bssplit_parse(td, options->bssplit_str, cmdprio);
+ if (ret)
+ goto err;
+
+ return 0;
+
+err:
+ fio_cmdprio_cleanup(cmdprio);
+
+ return ret;
+}
+
+static int fio_cmdprio_parse_and_gen(struct thread_data *td,
+ struct cmdprio *cmdprio)
+{
+ struct cmdprio_options *options = cmdprio->options;
+ int i, ret;
+
+ switch (cmdprio->mode) {
+ case CMDPRIO_MODE_BSSPLIT:
+ ret = fio_cmdprio_parse_and_gen_bssplit(td, cmdprio);
+ break;
+ case CMDPRIO_MODE_PERC:
+ ret = 0;
+ break;
+ default:
+ assert(0);
+ return 1;
+ }
/*
* If cmdprio_percentage/cmdprio_bssplit is set and cmdprio_class
* 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;
- has_cmdprio_percentage = true;
- }
- if (cmdprio->bssplit_nr[i]) {
- if (!cmdprio->class[i])
- cmdprio->class[i] = IOPRIO_CLASS_RT;
- has_cmdprio_bssplit = true;
+ if (options->percentage[i] || cmdprio->bssplit_nr[i]) {
+ if (!options->class[i])
+ options->class[i] = IOPRIO_CLASS_RT;
}
}
+ return ret;
+}
+
+void fio_cmdprio_cleanup(struct cmdprio *cmdprio)
+{
+ int ddir;
+
+ for (ddir = 0; ddir < CMDPRIO_RWDIR_CNT; ddir++) {
+ free(cmdprio->bssplit[ddir]);
+ cmdprio->bssplit[ddir] = NULL;
+ cmdprio->bssplit_nr[ddir] = 0;
+ }
+
+ /*
+ * options points to a cmdprio_options struct that is part of td->eo.
+ * td->eo itself will be freed by free_ioengine().
+ */
+ cmdprio->options = NULL;
+}
+
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
+ struct cmdprio_options *options)
+{
+ struct thread_options *to = &td->o;
+ bool has_cmdprio_percentage = false;
+ bool has_cmdprio_bssplit = false;
+ int i;
+
+ cmdprio->options = options;
+
+ if (options->bssplit_str && strlen(options->bssplit_str))
+ has_cmdprio_bssplit = true;
+
+ for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
+ if (options->percentage[i])
+ has_cmdprio_percentage = true;
+ }
+
/*
* Check for option conflicts
*/
@@ -179,5 +235,9 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
else
cmdprio->mode = CMDPRIO_MODE_NONE;
- return 0;
+ /* Nothing left to do if cmdprio is not used */
+ if (cmdprio->mode == CMDPRIO_MODE_NONE)
+ return 0;
+
+ return fio_cmdprio_parse_and_gen(td, cmdprio);
}
diff --git a/engines/cmdprio.h b/engines/cmdprio.h
index 7e4fcf6c..0c7bd6cf 100644
--- a/engines/cmdprio.h
+++ b/engines/cmdprio.h
@@ -17,21 +17,26 @@ enum {
CMDPRIO_MODE_BSSPLIT,
};
-struct cmdprio {
+struct cmdprio_options {
unsigned int percentage[CMDPRIO_RWDIR_CNT];
unsigned int class[CMDPRIO_RWDIR_CNT];
unsigned int level[CMDPRIO_RWDIR_CNT];
+ char *bssplit_str;
+};
+
+struct cmdprio {
+ struct cmdprio_options *options;
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,
- struct cmdprio *cmdprio);
-
bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
struct io_u *io_u);
-int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio);
+void fio_cmdprio_cleanup(struct cmdprio *cmdprio);
+
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
+ struct cmdprio_options *options);
#endif
diff --git a/engines/io_uring.c b/engines/io_uring.c
index 1f55ad17..8b8f35f1 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -68,12 +68,14 @@ struct ioring_data {
int prepped;
struct ioring_mmap mmap[3];
+
+ struct cmdprio cmdprio;
};
struct ioring_options {
struct thread_data *td;
unsigned int hipri;
- struct cmdprio cmdprio;
+ struct cmdprio_options cmdprio_options;
unsigned int fixedbufs;
unsigned int registerfiles;
unsigned int sqpoll_thread;
@@ -104,15 +106,6 @@ static int fio_ioring_sqpoll_cb(void *data, unsigned long long *val)
return 0;
}
-static int str_cmdprio_bssplit_cb(void *data, const char *input)
-{
- struct ioring_options *o = data;
- struct thread_data *td = o->td;
- struct cmdprio *cmdprio = &o->cmdprio;
-
- return fio_cmdprio_bssplit_parse(td, input, cmdprio);
-}
-
static struct fio_option options[] = {
{
.name = "hipri",
@@ -129,9 +122,9 @@ static struct fio_option options[] = {
.lname = "high priority percentage",
.type = FIO_OPT_INT,
.off1 = offsetof(struct ioring_options,
- cmdprio.percentage[DDIR_READ]),
+ cmdprio_options.percentage[DDIR_READ]),
.off2 = offsetof(struct ioring_options,
- cmdprio.percentage[DDIR_WRITE]),
+ cmdprio_options.percentage[DDIR_WRITE]),
.minval = 0,
.maxval = 100,
.help = "Send high priority I/O this percentage of the time",
@@ -143,9 +136,9 @@ static struct fio_option options[] = {
.lname = "Asynchronous I/O priority class",
.type = FIO_OPT_INT,
.off1 = offsetof(struct ioring_options,
- cmdprio.class[DDIR_READ]),
+ cmdprio_options.class[DDIR_READ]),
.off2 = offsetof(struct ioring_options,
- cmdprio.class[DDIR_WRITE]),
+ cmdprio_options.class[DDIR_WRITE]),
.help = "Set asynchronous IO priority class",
.minval = IOPRIO_MIN_PRIO_CLASS + 1,
.maxval = IOPRIO_MAX_PRIO_CLASS,
@@ -158,9 +151,9 @@ static struct fio_option options[] = {
.lname = "Asynchronous I/O priority level",
.type = FIO_OPT_INT,
.off1 = offsetof(struct ioring_options,
- cmdprio.level[DDIR_READ]),
+ cmdprio_options.level[DDIR_READ]),
.off2 = offsetof(struct ioring_options,
- cmdprio.level[DDIR_WRITE]),
+ cmdprio_options.level[DDIR_WRITE]),
.help = "Set asynchronous IO priority level",
.minval = IOPRIO_MIN_PRIO,
.maxval = IOPRIO_MAX_PRIO,
@@ -171,9 +164,9 @@ static struct fio_option options[] = {
{
.name = "cmdprio_bssplit",
.lname = "Priority percentage block size split",
- .type = FIO_OPT_STR_ULL,
- .cb = str_cmdprio_bssplit_cb,
- .off1 = offsetof(struct ioring_options, cmdprio.bssplit),
+ .type = FIO_OPT_STR_STORE,
+ .off1 = offsetof(struct ioring_options,
+ cmdprio_options.bssplit_str),
.help = "Set priority percentages for different block sizes",
.category = FIO_OPT_C_ENGINE,
.group = FIO_OPT_G_IOURING,
@@ -458,8 +451,7 @@ static inline void fio_ioring_cmdprio_prep(struct thread_data *td,
struct io_u *io_u)
{
struct ioring_data *ld = td->io_ops_data;
- struct ioring_options *o = td->eo;
- struct cmdprio *cmdprio = &o->cmdprio;
+ struct cmdprio *cmdprio = &ld->cmdprio;
if (fio_cmdprio_set_ioprio(td, cmdprio, io_u))
ld->sqes[io_u->index].ioprio = io_u->ioprio;
@@ -470,7 +462,6 @@ 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);
@@ -493,7 +484,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 (o->cmdprio.mode != CMDPRIO_MODE_NONE)
+ if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
fio_ioring_cmdprio_prep(td, io_u);
ring->array[tail & ld->sq_ring_mask] = io_u->index;
@@ -599,6 +590,7 @@ static void fio_ioring_cleanup(struct thread_data *td)
if (!(td->flags & TD_F_CHILD))
fio_ioring_unmap(ld);
+ fio_cmdprio_cleanup(&ld->cmdprio);
free(ld->io_u_index);
free(ld->iovecs);
free(ld->fds);
@@ -805,7 +797,6 @@ static int fio_ioring_init(struct thread_data *td)
{
struct ioring_options *o = td->eo;
struct ioring_data *ld;
- struct cmdprio *cmdprio = &o->cmdprio;
int ret;
/* sqthread submission requires registered files */
@@ -830,7 +821,7 @@ static int fio_ioring_init(struct thread_data *td)
td->io_ops_data = ld;
- ret = fio_cmdprio_init(td, cmdprio);
+ ret = fio_cmdprio_init(td, &ld->cmdprio, &o->cmdprio_options);
if (ret) {
td_verror(td, EINVAL, "fio_ioring_init");
return 1;
diff --git a/engines/libaio.c b/engines/libaio.c
index 53fe7428..9c278d06 100644
--- a/engines/libaio.c
+++ b/engines/libaio.c
@@ -51,24 +51,17 @@ struct libaio_data {
unsigned int queued;
unsigned int head;
unsigned int tail;
+
+ struct cmdprio cmdprio;
};
struct libaio_options {
struct thread_data *td;
unsigned int userspace_reap;
- struct cmdprio cmdprio;
+ struct cmdprio_options cmdprio_options;
unsigned int nowait;
};
-static int str_cmdprio_bssplit_cb(void *data, const char *input)
-{
- struct libaio_options *o = data;
- struct thread_data *td = o->td;
- struct cmdprio *cmdprio = &o->cmdprio;
-
- return fio_cmdprio_bssplit_parse(td, input, cmdprio);
-}
-
static struct fio_option options[] = {
{
.name = "userspace_reap",
@@ -85,9 +78,9 @@ static struct fio_option options[] = {
.lname = "high priority percentage",
.type = FIO_OPT_INT,
.off1 = offsetof(struct libaio_options,
- cmdprio.percentage[DDIR_READ]),
+ cmdprio_options.percentage[DDIR_READ]),
.off2 = offsetof(struct libaio_options,
- cmdprio.percentage[DDIR_WRITE]),
+ cmdprio_options.percentage[DDIR_WRITE]),
.minval = 0,
.maxval = 100,
.help = "Send high priority I/O this percentage of the time",
@@ -99,9 +92,9 @@ static struct fio_option options[] = {
.lname = "Asynchronous I/O priority class",
.type = FIO_OPT_INT,
.off1 = offsetof(struct libaio_options,
- cmdprio.class[DDIR_READ]),
+ cmdprio_options.class[DDIR_READ]),
.off2 = offsetof(struct libaio_options,
- cmdprio.class[DDIR_WRITE]),
+ cmdprio_options.class[DDIR_WRITE]),
.help = "Set asynchronous IO priority class",
.minval = IOPRIO_MIN_PRIO_CLASS + 1,
.maxval = IOPRIO_MAX_PRIO_CLASS,
@@ -114,9 +107,9 @@ static struct fio_option options[] = {
.lname = "Asynchronous I/O priority level",
.type = FIO_OPT_INT,
.off1 = offsetof(struct libaio_options,
- cmdprio.level[DDIR_READ]),
+ cmdprio_options.level[DDIR_READ]),
.off2 = offsetof(struct libaio_options,
- cmdprio.level[DDIR_WRITE]),
+ cmdprio_options.level[DDIR_WRITE]),
.help = "Set asynchronous IO priority level",
.minval = IOPRIO_MIN_PRIO,
.maxval = IOPRIO_MAX_PRIO,
@@ -127,9 +120,9 @@ static struct fio_option options[] = {
{
.name = "cmdprio_bssplit",
.lname = "Priority percentage block size split",
- .type = FIO_OPT_STR_ULL,
- .cb = str_cmdprio_bssplit_cb,
- .off1 = offsetof(struct libaio_options, cmdprio.bssplit),
+ .type = FIO_OPT_STR_STORE,
+ .off1 = offsetof(struct libaio_options,
+ cmdprio_options.bssplit_str),
.help = "Set priority percentages for different block sizes",
.category = FIO_OPT_C_ENGINE,
.group = FIO_OPT_G_LIBAIO,
@@ -206,8 +199,8 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
static inline 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;
+ struct libaio_data *ld = td->io_ops_data;
+ struct cmdprio *cmdprio = &ld->cmdprio;
if (fio_cmdprio_set_ioprio(td, cmdprio, io_u)) {
io_u->iocb.aio_reqprio = io_u->ioprio;
@@ -318,7 +311,6 @@ 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);
@@ -349,7 +341,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
return FIO_Q_COMPLETED;
}
- if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
+ if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
fio_libaio_cmdprio_prep(td, io_u);
ld->iocbs[ld->head] = &io_u->iocb;
@@ -468,6 +460,8 @@ static void fio_libaio_cleanup(struct thread_data *td)
*/
if (!(td->flags & TD_F_CHILD))
io_destroy(ld->aio_ctx);
+
+ fio_cmdprio_cleanup(&ld->cmdprio);
free(ld->aio_events);
free(ld->iocbs);
free(ld->io_us);
@@ -493,7 +487,6 @@ static int fio_libaio_init(struct thread_data *td)
{
struct libaio_data *ld;
struct libaio_options *o = td->eo;
- struct cmdprio *cmdprio = &o->cmdprio;
int ret;
ld = calloc(1, sizeof(*ld));
@@ -506,7 +499,7 @@ static int fio_libaio_init(struct thread_data *td)
td->io_ops_data = ld;
- ret = fio_cmdprio_init(td, cmdprio);
+ ret = fio_cmdprio_init(td, &ld->cmdprio, &o->cmdprio_options);
if (ret) {
td_verror(td, EINVAL, "fio_libaio_init");
return 1;
--
2.33.1
next prev parent reply other threads:[~2021-11-12 9:54 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-12 9:54 [PATCH v3 0/8] cmdprio cleanup series Niklas Cassel
2021-11-12 9:54 ` [PATCH v3 1/8] docs: update cmdprio_percentage documentation Niklas Cassel
2021-11-12 9:54 ` [PATCH v3 3/8] cmdprio: do not allocate memory for unused data direction Niklas Cassel
2021-11-12 9:54 ` [PATCH v3 2/8] cmdprio: move cmdprio function definitions to a new cmdprio.c file Niklas Cassel
2021-11-12 9:54 ` [PATCH v3 5/8] libaio,io_uring: rename prio_prep() to include cmdprio in the name Niklas Cassel
2021-11-12 9:54 ` [PATCH v3 4/8] io_uring: set async IO priority to td->ioprio in fio_ioring_prep() Niklas Cassel
2021-11-12 9:54 ` [PATCH v3 6/8] libaio,io_uring: move common cmdprio_prep() code to cmdprio Niklas Cassel
2021-11-12 9:54 ` [PATCH v3 7/8] cmdprio: add mode to make the logic easier to reason about Niklas Cassel
2021-11-12 9:54 ` Niklas Cassel [this message]
2021-11-12 15:50 ` [PATCH v3 0/8] cmdprio cleanup series Jens Axboe
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=20211112095428.158300-9-Niklas.Cassel@wdc.com \
--to=niklas.cassel@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.