[PATCH 0/2] cgroup: Enable weight tests with cgroup-v2

[PATCH 0/2] cgroup: Enable weight tests with cgroup-v2

[Andreas Herrmann]

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.

-- 
Regards,
Andreas

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

[PATCH 1/2] thread_options: Add cgroup_use_bfq

[Andreas Herrmann]

Signed-off-by: Andreas Herrmann <aherrmann@suse.com>
---
 HOWTO            |  6 ++++++
 cconv.c          |  2 ++
 fio.1            |  5 +++++
 options.c        | 11 +++++++++++
 thread_options.h |  7 +++++--
 5 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/HOWTO b/HOWTO
index e0403b08..4a16534f 100644
--- a/HOWTO
+++ b/HOWTO
@@ -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
diff --git a/cconv.c b/cconv.c
index 4b0c3490..3f99e8d7 100644
--- a/cconv.c
+++ b/cconv.c
@@ -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);
diff --git a/fio.1 b/fio.1
index 1c90e4a5..a0896235 100644
--- a/fio.1
+++ b/fio.1
@@ -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.
diff --git a/options.c b/options.c
index 251ad2c1..c84e911a 100644
--- a/options.c
+++ b/options.c
@@ -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",
diff --git a/thread_options.h b/thread_options.h
index 14f1cbe9..0331e881 100644
--- a/thread_options.h
+++ b/thread_options.h
@@ -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;
-- 
2.28.0

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

[Elliott, Robert (Servers)]

> -----Original Message-----
> From: fio-owner@vger.kernel.org <fio-owner@vger.kernel.org> On Behalf
> Of Andreas Herrmann
> Sent: Friday, August 28, 2020 5:05 AM
> To: Jens Axboe <axboe@kernel.dk>
> Cc: fio@vger.kernel.org
> Subject: [PATCH 1/2] thread_options: Add cgroup_use_bfq
> 
...
> @@ -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;

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.

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

[Jens Axboe]

On 8/28/20 12:36 PM, Elliott, Robert (Servers) wrote:
> 
> 
>> -----Original Message-----
>> From: fio-owner@vger.kernel.org <fio-owner@vger.kernel.org> On Behalf
>> Of Andreas Herrmann
>> Sent: Friday, August 28, 2020 5:05 AM
>> To: Jens Axboe <axboe@kernel.dk>
>> Cc: fio@vger.kernel.org
>> Subject: [PATCH 1/2] thread_options: Add cgroup_use_bfq
>>
> ...
>> @@ -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;
> 
> 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.

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.

-- 
Jens Axboe

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

[Elliott, Robert (Servers)]

> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Friday, August 28, 2020 6:52 PM
> To: Elliott, Robert (Servers) <elliott@hpe.com>; Andreas Herrmann
> <aherrmann@suse.com>
> Subject: Re: [PATCH 1/2] thread_options: Add cgroup_use_bfq
> 
...
> > ...
> >> @@ -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;
> >
> > 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.
> 
> 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");

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

[Andreas Herrmann]

On Fri, Aug 28, 2020 at 05:52:11PM -0600, Jens Axboe wrote:
> On 8/28/20 12:36 PM, Elliott, Robert (Servers) wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: fio-owner@vger.kernel.org <fio-owner@vger.kernel.org> On Behalf
> >> Of Andreas Herrmann
> >> Sent: Friday, August 28, 2020 5:05 AM
> >> To: Jens Axboe <axboe@kernel.dk>
> >> Cc: fio@vger.kernel.org
> >> Subject: [PATCH 1/2] thread_options: Add cgroup_use_bfq
> >>
> > ...
> >> @@ -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;
> > 
> > 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.
> 
> 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.

> 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.

-- 
Regards,
Andreas

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

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

[Andreas Herrmann]

On Fri, Aug 28, 2020 at 06:36:56PM +0000, Elliott, Robert (Servers) wrote:
> 
> 
> > -----Original Message-----
> > From: fio-owner@vger.kernel.org <fio-owner@vger.kernel.org> On Behalf
> > Of Andreas Herrmann
> > Sent: Friday, August 28, 2020 5:05 AM
> > To: Jens Axboe <axboe@kernel.dk>
> > Cc: fio@vger.kernel.org
> > Subject: [PATCH 1/2] thread_options: Add cgroup_use_bfq
> > 
> ...
> > @@ -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;
> 
> 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.

Good point.
I'll modify my patch.

-- 
Regards,
Andreas

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

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

[Andreas Herrmann]

Signed-off-by: Andreas Herrmann <aherrmann@suse.com>
---
 cgroup.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/cgroup.c b/cgroup.c
index 6edb5b14..9fcf44ad 100644
--- a/cgroup.c
+++ b/cgroup.c
@@ -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;
+			}
 		}
 	}
 
-- 
2.28.0