cgroup: Enable weight tests with cgroup-v2

PATCH In review Andreas Herrmann 3 participants 8 messages Aug 28, 2020 – Aug 31, 2020

lore

Share & tools

lore

Andreas Herrmann

author

2020-08-28 10:02 UTC #0 lore raw

cover cgroup: Enable weight tests with cgroup-v2

Hi,

To allow tests with weight attribute with cgroup-v2
it would be nice to be able to specify which attribute to use -- either the generic or the BFQ specific one. Patch #1 adds an option to do that.
Patch #2 enables tests with cgroup.weight when cgroup-v2 is used.

Signature · 6 lines

Regards,
Andreas

SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer (HRB 36809, AG Nürnberg)

Andreas Herrmann

author

2020-08-28 10:04 UTC #1 lore raw

PATCH 1/2 thread_options: Add cgroup_use_bfq

Signed-off-by Andreas Herrmann <[email protected]>

---
 HOWTO            |  6 ++++++
 cconv.c          |  2 ++
 fio.1            |  5 +++++
 options.c        | 11 +++++++++++
 thread_options.h |  7 +++++--
 5 files changed, 29 insertions(+), 2 deletions(-)

HOWTO +6 -0

index e0403b08..4a16534f 100644
@@ -2846,6 +2846,12 @@ Threads, processes and job synchronization
 	job completion, set ``cgroup_nodelete=1``.  This can be useful if one wants
 	to inspect various cgroup files after job completion. Default: false.
 
+.. option:: cgroup_use_bfq=bool
+
+	Normally fio will use generic sysfs attributes to set cgroup_weight.
+	To use attributes specific to BFQ IO scheduler set ``cgroup_use_bfq=1``.
+	Default: false.
+
 .. option:: flow_id=int
 
 	The ID of the flow. If not specified, it defaults to being a global

cconv.c +2 -0

index 4b0c3490..3f99e8d7 100644
@@ -278,6 +278,7 @@ void convert_thread_options_to_cpu(struct thread_options *o,
 	o->continue_on_error = le32_to_cpu(top->continue_on_error);
 	o->cgroup_weight = le32_to_cpu(top->cgroup_weight);
 	o->cgroup_nodelete = le32_to_cpu(top->cgroup_nodelete);
+	o->cgroup_use_bfq = le32_to_cpu(top->cgroup_use_bfq);
 	o->uid = le32_to_cpu(top->uid);
 	o->gid = le32_to_cpu(top->gid);
 	o->flow_id = __le32_to_cpu(top->flow_id);

@@ -478,6 +479,7 @@ void convert_thread_options_to_net(struct thread_options_pack *top,
 	top->continue_on_error = cpu_to_le32(o->continue_on_error);
 	top->cgroup_weight = cpu_to_le32(o->cgroup_weight);
 	top->cgroup_nodelete = cpu_to_le32(o->cgroup_nodelete);
+	top->cgroup_use_bfq = cpu_to_le32(o->cgroup_use_bfq);
 	top->uid = cpu_to_le32(o->uid);
 	top->gid = cpu_to_le32(o->gid);
 	top->flow_id = __cpu_to_le32(o->flow_id);

fio.1 +5 -0

index 1c90e4a5..a0896235 100644
@@ -2544,6 +2544,11 @@ completion. To override this behavior and to leave cgroups around after the
 job completion, set `cgroup_nodelete=1'. This can be useful if one wants
 to inspect various cgroup files after job completion. Default: false.
 .TP
+.BI cgroup_use_bfq \fR=\fPbool
+Normally fio will use generic sysfs attributes to set cgroup_weight.
+To use attributes specific to BFQ IO scheduler set ``cgroup_use_bfq=1``.
+Default: false.
+.TP
 .BI flow_id \fR=\fPint
 The ID of the flow. If not specified, it defaults to being a global
 flow. See \fBflow\fR.

options.c +11 -0

index 251ad2c1..c84e911a 100644
@@ -4609,6 +4609,17 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 		.category = FIO_OPT_C_GENERAL,
 		.group	= FIO_OPT_G_CGROUP,
 	},
+	{
+		.name	= "cgroup_use_bfq",
+		.lname	= "Cgroup use-bfq",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct thread_options, cgroup_use_bfq),
+		.help	= "Use cgroup sysfs attributes specific to BFQ",
+		.def	= "0",
+		.parent	= "cgroup",
+		.category = FIO_OPT_C_GENERAL,
+		.group	= FIO_OPT_G_CGROUP,
+	},
 	{
 		.name	= "uid",
 		.lname	= "User ID",

thread_options.h +5 -2

index 14f1cbe9..0331e881 100644
@@ -302,11 +302,12 @@ struct thread_options {
 	char *profile;
 
 	/*
-	 * blkio cgroup support
+	 * (blk)io cgroup support
 	 */
 	char *cgroup;
 	unsigned int cgroup_weight;
 	unsigned int cgroup_nodelete;
+	unsigned int cgroup_use_bfq;
 
 	unsigned int uid;
 	unsigned int gid;

@@ -593,11 +594,13 @@ struct thread_options_pack {
 	uint8_t profile[FIO_TOP_STR_MAX];
 
 	/*
-	 * blkio cgroup support
+	 * (blk)io cgroup support
 	 */
 	uint8_t cgroup[FIO_TOP_STR_MAX];
 	uint32_t cgroup_weight;
 	uint32_t cgroup_nodelete;
+	uint32_t cgroup_use_bfq;
+	uint32_t padding;       /* remove when possible to maintain alignment */
 
 	uint32_t uid;
 	uint32_t gid;

Signature · 1 line

2.28.0

Elliott, Robert (Servers)

2020-08-28 18:36 UTC #2 lore raw

RE: [PATCH 1/2] thread_options: Add cgroup_use_bfq

Quoted 10 lines

-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Andreas Herrmann
Sent: Friday, August 28, 2020 5:05 AM
To: Jens Axboe <[email protected]>
Cc: [email protected]
Subject: [PATCH 1/2] thread_options: Add cgroup_use_bfq

...

Quoted 5 lines

thread_options.h +0 -1

@@ -593,11 +594,13 @@ struct thread_options_pack {
 	uint8_t profile[FIO_TOP_STR_MAX];
 
 	/*
-	 * blkio cgroup support

thread_options.h struct thread_options_pack { · line 606

Quoted 10 lines

thread_options.h +3 -0

+	 * (blk)io cgroup support
 	 */
 	uint8_t cgroup[FIO_TOP_STR_MAX];
 	uint32_t cgroup_weight;
 	uint32_t cgroup_nodelete;
+	uint32_t cgroup_use_bfq;
+	uint32_t padding;       /* remove when possible to maintain alignment */
 
 	uint32_t uid;
 	uint32_t gid;

Since the structure already has three pad fields:

        uint32_t pad;

        uint64_t size;
	...
        uint32_t pad2;
        uint64_t rand_seed;
	...
        uint32_t pad3;
        fio_fp64_t percentile_list[FIO_IO_U_LIST_MAX_LEN];

pad4 would seem like the natural name for another padding field, and the comment doesn't seem necessary.

Jens Axboe

2020-08-28 23:52 UTC #3 lore raw

Re: [PATCH 1/2] thread_options: Add cgroup_use_bfq

Quoted 38 lines · Robert (Servers)

Quoted 10 lines
-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Andreas Herrmann
Sent: Friday, August 28, 2020 5:05 AM
To: Jens Axboe <[email protected]>
Cc: [email protected]
Subject: [PATCH 1/2] thread_options: Add cgroup_use_bfq
...
Quoted 5 lines
thread_options.h +0 -1
@@ -593,11 +594,13 @@ struct thread_options_pack {
 	uint8_t profile[FIO_TOP_STR_MAX];
 
 	/*
-	 * blkio cgroup support
thread_options.h struct thread_options_pack { · line 606
Quoted 10 lines
thread_options.h +3 -0
+	 * (blk)io cgroup support
 	 */
 	uint8_t cgroup[FIO_TOP_STR_MAX];
 	uint32_t cgroup_weight;
 	uint32_t cgroup_nodelete;
+	uint32_t cgroup_use_bfq;
+	uint32_t padding;       /* remove when possible to maintain alignment */
 
 	uint32_t uid;
 	uint32_t gid;
Since the structure already has three pad fields:
        uint32_t pad;

        uint64_t size;
	...
        uint32_t pad2;
        uint64_t rand_seed;
	...
        uint32_t pad3;
        fio_fp64_t percentile_list[FIO_IO_U_LIST_MAX_LEN];

Quoted 2 lines

pad4 would seem like the natural name for another padding field, and the comment doesn't seem necessary.

Also worth checking if you can actually remove padding when updating it, rather than adding more.

BTW, this is also missing a FIO_SERVER_VER increment, that's needed whenever thread_options are changed.

Signature · 1 line

Jens Axboe

Elliott, Robert (Servers)

2020-08-29 02:14 UTC #4 lore raw

RE: [PATCH 1/2] thread_options: Add cgroup_use_bfq

Quoted 9 lines

-----Original Message-----
From: Jens Axboe <[email protected]>
Sent: Friday, August 28, 2020 6:52 PM
To: Elliott, Robert (Servers) <[email protected]>; Andreas Herrmann <[email protected]>
Subject: Re: [PATCH 1/2] thread_options: Add cgroup_use_bfq

...

Quoted 29 lines

Quoted 4 lines
...
Quoted 3 lines
thread_options.h +0 -0
@@ -593,11 +594,13 @@ struct thread_options_pack {
 	uint8_t profile[FIO_TOP_STR_MAX];
 
Quoted 9 lines
thread_options.h struct thread_options_pack {
Quoted 9 lines
thread_options.h +3 -1
 	/*
-	 * blkio cgroup support
+	 * (blk)io cgroup support
 	 */
 	uint8_t cgroup[FIO_TOP_STR_MAX];
 	uint32_t cgroup_weight;
 	uint32_t cgroup_nodelete;
+	uint32_t cgroup_use_bfq;
+	uint32_t padding;       /* remove when possible to maintain
alignment */
Quoted 15 lines
thread_options.h struct thread_options_pack { · line 606
Quoted 3 lines
thread_options.h +0 -0
 	uint32_t uid;
 	uint32_t gid;
Since the structure already has three pad fields:
        uint32_t pad;

        uint64_t size;
	...
        uint32_t pad2;
        uint64_t rand_seed;
	...
        uint32_t pad3;
        fio_fp64_t percentile_list[FIO_IO_U_LIST_MAX_LEN];

Quoted 6 lines

Quoted 1 line
pad4 would seem like the natural name for another padding field,
and
Quoted 1 line
the comment doesn't seem necessary.
Also worth checking if you can actually remove padding when updating it, rather than adding more.

I do think that one is needed, because there's a floating point variable afterwards that libfio.c insists be aligned to an 8-byte boundary.

The pahole utility reports the offsets (pre-patch) are:

/* --- cacheline 395 boundary (25280 bytes) was 24 bytes ago --- */
uint8_t cgroup[256]; /* 25304 256 */
/* --- cacheline 399 boundary (25536 bytes) was 24 bytes ago --- */
uint32_t cgroup_weight; /* 25560 4 */
uint32_t cgroup_nodelete; /* 25564 4 */
uint32_t uid; /* 25568 4 */
uint32_t gid; /* 25572 4 */
int32_t flow_id; /* 25576 4 */
int32_t flow; /* 25580 4 */
int32_t flow_watermark; /* 25584 4 */
uint32_t flow_sleep; /* 25588 4 */
uint64_t offset_increment; /* 25592 8 */
/* --- cacheline 400 boundary (25600 bytes) --- */
uint64_t number_ios; /* 25600 8 */
uint64_t latency_target; /* 25608 8 */
uint64_t latency_window; /* 25616 8 */
uint64_t max_latency; /* 25624 8 */
fio_fp64_t latency_percentile; /* 25632 16 */

libfio.c initialize_fio() excerpt:

        /*
         * We need these to be properly 64-bit aligned, otherwise we
         * can run into problems on archs that fault on unaligned fp
         * access (ARM).
         */
        compiletime_assert((offsetof(struct thread_options_pack, zipf_theta) % 8) == 0, "zipf_theta");
        compiletime_assert((offsetof(struct thread_options_pack, pareto_h) % 8) == 0, "pareto_h");
        compiletime_assert((offsetof(struct thread_options_pack, percentile_list) % 8) == 0, "percentile_list");
        compiletime_assert((offsetof(struct thread_options_pack, latency_percentile) % 8) == 0, "latency_percentile");

Andreas Herrmann

author

2020-08-31 06:07 UTC #5 lore raw

Re: [PATCH 1/2] thread_options: Add cgroup_use_bfq

In reply to Jens Axboe

Quoted 39 lines · Jens Axboe

Quoted 38 lines · Robert (Servers)
Quoted 10 lines
-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Andreas Herrmann
Sent: Friday, August 28, 2020 5:05 AM
To: Jens Axboe <[email protected]>
Cc: [email protected]
Subject: [PATCH 1/2] thread_options: Add cgroup_use_bfq
...
Quoted 5 lines
thread_options.h +0 -1
@@ -593,11 +594,13 @@ struct thread_options_pack {
 	uint8_t profile[FIO_TOP_STR_MAX];
 
 	/*
-	 * blkio cgroup support
thread_options.h struct thread_options_pack { · line 606
Quoted 10 lines
thread_options.h +3 -0
+	 * (blk)io cgroup support
 	 */
 	uint8_t cgroup[FIO_TOP_STR_MAX];
 	uint32_t cgroup_weight;
 	uint32_t cgroup_nodelete;
+	uint32_t cgroup_use_bfq;
+	uint32_t padding;       /* remove when possible to maintain alignment */
 
 	uint32_t uid;
 	uint32_t gid;
Since the structure already has three pad fields:
        uint32_t pad;

        uint64_t size;
	...
        uint32_t pad2;
        uint64_t rand_seed;
	...
        uint32_t pad3;
        fio_fp64_t percentile_list[FIO_IO_U_LIST_MAX_LEN];

Quoted 5 lines

Quoted 2 lines
pad4 would seem like the natural name for another padding field, and the comment doesn't seem necessary.
Also worth checking if you can actually remove padding when updating it, rather than adding more.

I think I can't remove padding due to
fio_fp64_t latency_percentile;
later in this struct.

Quoted 2 lines

BTW, this is also missing a FIO_SERVER_VER increment, that's needed whenever thread_options are changed.

I overlooked that. I'll adapt my change.

Signature · 6 lines

Regards,
Andreas

SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer (HRB 36809, AG Nürnberg)

Andreas Herrmann

author

2020-08-31 06:03 UTC #6 lore raw

Re: [PATCH 1/2] thread_options: Add cgroup_use_bfq

In reply to Elliott, Robert (Servers)

Quoted 38 lines · Robert (Servers)

Quoted 10 lines
-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Andreas Herrmann
Sent: Friday, August 28, 2020 5:05 AM
To: Jens Axboe <[email protected]>
Cc: [email protected]
Subject: [PATCH 1/2] thread_options: Add cgroup_use_bfq
...
Quoted 5 lines
thread_options.h +0 -1
@@ -593,11 +594,13 @@ struct thread_options_pack {
 	uint8_t profile[FIO_TOP_STR_MAX];
 
 	/*
-	 * blkio cgroup support
thread_options.h struct thread_options_pack { · line 606
Quoted 10 lines
thread_options.h +3 -0
+	 * (blk)io cgroup support
 	 */
 	uint8_t cgroup[FIO_TOP_STR_MAX];
 	uint32_t cgroup_weight;
 	uint32_t cgroup_nodelete;
+	uint32_t cgroup_use_bfq;
+	uint32_t padding;       /* remove when possible to maintain alignment */
 
 	uint32_t uid;
 	uint32_t gid;
Since the structure already has three pad fields:
        uint32_t pad;

        uint64_t size;
	...
        uint32_t pad2;
        uint64_t rand_seed;
	...
        uint32_t pad3;
        fio_fp64_t percentile_list[FIO_IO_U_LIST_MAX_LEN];

Quoted 2 lines

pad4 would seem like the natural name for another padding field, and the comment doesn't seem necessary.

Good point.
I'll modify my patch.

Signature · 6 lines

Regards,
Andreas

SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer (HRB 36809, AG Nürnberg)

Andreas Herrmann

author

2020-08-28 10:06 UTC #7 lore raw

PATCH 2/2 cgroup: Allow to use weights with cgroup-v2

In reply to Andreas Herrmann

Signed-off-by Andreas Herrmann <[email protected]>

---
 cgroup.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 11 deletions(-)

cgroup.c +45 ~~-11~~

index 6edb5b14..9fcf44ad 100644
@@ -165,6 +165,26 @@ static int write_int_to_file(struct thread_data *td, const char *path,
 
 }
 
+static int write_str_to_file(struct thread_data *td, const char *path,
+			     const char *filename, const char *str,
+			     const char *onerr)
+{
+	char tmp[256];
+	FILE *f;
+
+	sprintf(tmp, "%s/%s", path, filename);
+	f = fopen(tmp, "w");
+	if (!f) {
+		td_verror(td, errno, onerr);
+		return 1;
+	}
+
+	fprintf(f, "%s", str);
+	fclose(f);
+	return 0;
+
+}
+
 static int cgroup_write_pid(struct thread_data *td, char *path, bool cgroup2)
 {
 	unsigned int val = td->pid;

@@ -213,18 +233,32 @@ int cgroup_setup(struct thread_data *td, struct flist_head *clist, struct cgroup
 
 	if (td->o.cgroup_weight) {
 		if ((*mnt)->cgroup2) {
-			log_err("fio: cgroup weight doesn't work with cgroup2\n");
-			goto err;
-		}
-		if (write_int_to_file(td, root, "blkio.weight",
-					td->o.cgroup_weight,
-					"cgroup open blkio.weight")) {
-			td_clear_error(td);
-			log_err("Trying to use blkio.bfq.weight\n");
-			if (write_int_to_file(td, root, "blkio.bfq.weight",
-						td->o.cgroup_weight,
-						"cgroup open blkio.bfq.weight"))
+			if (write_str_to_file(td, (*mnt)->path, "cgroup.subtree_control",
+					"+io", "cgroup open cgroup.subtree_control"))
 				goto err;
+			if (!td->o.cgroup_use_bfq) {
+				if (write_int_to_file(td, root, "io.weight",
+							td->o.cgroup_weight,
+							"cgroup open io.weight"))
+					goto err;
+			} else {
+				if (write_int_to_file(td, root, "io.bfq.weight",
+							td->o.cgroup_weight,
+							"cgroup open io.bfq.weight"))
+					goto err;
+			}
+		} else {
+			if (!td->o.cgroup_use_bfq) {
+				if (write_int_to_file(td, root, "blkio.weight",
+							td->o.cgroup_weight,
+							"cgroup open blkio.weight"))
+					goto err;
+			} else {
+				if (write_int_to_file(td, root, "blkio.bfq.weight",
+							td->o.cgroup_weight,
+							"cgroup open blkio.bfq.weight"))
+					goto err;
+			}
 		}
 	}

Signature · 1 line

2.28.0