diff mbox series

[4/6] API/cgroups: Add cpu controller

Message ID 20210513152125.25766-5-rpalethorpe@suse.com
State Changes Requested
Headers show
Series cfs_bandwidth01 and CGroup API | expand

Commit Message

Richard Palethorpe May 13, 2021, 3:21 p.m. UTC
Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 lib/tst_cgroup.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Cyril Hrubis May 19, 2021, 11:30 a.m. UTC | #1
Hi!
>  lib/tst_cgroup.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index 54636fd7e..da177a1ad 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -82,7 +82,8 @@ struct cgroup_root {
>  /* Controller sub-systems */
>  enum cgroup_ctrl_indx {
>  	CTRL_MEMORY = 1,
> -	CTRL_CPUSET = 2,
> +	CTRL_CPU,
> +	CTRL_CPUSET,
>  };
>  #define CTRLS_MAX CTRL_CPUSET
>  
> @@ -162,6 +163,18 @@ static const files_t memory_ctrl_files = {
>  	{ }
>  };
>  
> +static const files_t cpu_ctrl_files = {
> +	/* The V1 quota and period files were combined in the V2 max
> +	 * file. The quota is in the first column and if we just print
> +	 * a single value to the file, it will be treated as the
> +	 * quota. To get or set the period we need to branch on the
> +	 * API version.
> +	 */

I wonder if this is worth a helper function, something as:

#define SAFE_CGROUP_CPU_SET_MAX(cg, quota_us, period_us) \
	tst_cgroup_cpu_set_max(__FILE__, __LINENO__, cg, quota_us, period_us)

void tst_cgroup_cpu_set_max(const char *const file, const int lineno,
                            const struct tst_cgroup_group *const cg,
                            unsigned int quota_us, unsigned int period_us);

#define SAFE_CGROUP_CPU_GET_MAX(cg, quota_us, period_us) \
	tst_cgroup_cpu_get_max(__FILE__, __LINENO__, cg, quota_us, period_us)

void tst_cgroup_cpu_get_max(const char *const file, const int lineno,
                            const struct tst_cgroup_group *const cg,
                            unsigned int *quota_us, unsigned int *period_us);

I guess that if we are going to add more tests we would end up with such
functions somewhere anyways.
Richard Palethorpe May 20, 2021, 8:35 a.m. UTC | #2
Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>>  lib/tst_cgroup.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
>> index 54636fd7e..da177a1ad 100644
>> --- a/lib/tst_cgroup.c
>> +++ b/lib/tst_cgroup.c
>> @@ -82,7 +82,8 @@ struct cgroup_root {
>>  /* Controller sub-systems */
>>  enum cgroup_ctrl_indx {
>>  	CTRL_MEMORY = 1,
>> -	CTRL_CPUSET = 2,
>> +	CTRL_CPU,
>> +	CTRL_CPUSET,
>>  };
>>  #define CTRLS_MAX CTRL_CPUSET
>>  
>> @@ -162,6 +163,18 @@ static const files_t memory_ctrl_files = {
>>  	{ }
>>  };
>>  
>> +static const files_t cpu_ctrl_files = {
>> +	/* The V1 quota and period files were combined in the V2 max
>> +	 * file. The quota is in the first column and if we just print
>> +	 * a single value to the file, it will be treated as the
>> +	 * quota. To get or set the period we need to branch on the
>> +	 * API version.
>> +	 */
>
> I wonder if this is worth a helper function, something as:
>
> #define SAFE_CGROUP_CPU_SET_MAX(cg, quota_us, period_us) \
> 	tst_cgroup_cpu_set_max(__FILE__, __LINENO__, cg, quota_us, period_us)
>
> void tst_cgroup_cpu_set_max(const char *const file, const int lineno,
>                             const struct tst_cgroup_group *const cg,
>                             unsigned int quota_us, unsigned int period_us);
>
> #define SAFE_CGROUP_CPU_GET_MAX(cg, quota_us, period_us) \
> 	tst_cgroup_cpu_get_max(__FILE__, __LINENO__, cg, quota_us, period_us)
>
> void tst_cgroup_cpu_get_max(const char *const file, const int lineno,
>                             const struct tst_cgroup_group *const cg,
>                             unsigned int *quota_us, unsigned int *period_us);
>
> I guess that if we are going to add more tests we would end up with such
> functions somewhere anyways.

Yes, when we have more tests. Because there is an alternative that we
add a mapping/filter layer to transform V2 writes into V1 writes. e.g.

SAFE_CGROUP_PRINTF(cg, "cpu.max", "%d %d", "50 100");

When done on V1 it will write to a buffer with sprintf and then split
that into two writes to the V1 files.

This is much more complicated for this case, but there are hundreds of
these knobs. In some cases we just need to convert a string to an
integer e.g. "max" => "-1" or whatever memory.max takes.

In most cases it will just be a case of sprintf into a buffer and then
do something with that.

I can think of problems with both approaches. :-)
diff mbox series

Patch

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 54636fd7e..da177a1ad 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -82,7 +82,8 @@  struct cgroup_root {
 /* Controller sub-systems */
 enum cgroup_ctrl_indx {
 	CTRL_MEMORY = 1,
-	CTRL_CPUSET = 2,
+	CTRL_CPU,
+	CTRL_CPUSET,
 };
 #define CTRLS_MAX CTRL_CPUSET
 
@@ -162,6 +163,18 @@  static const files_t memory_ctrl_files = {
 	{ }
 };
 
+static const files_t cpu_ctrl_files = {
+	/* The V1 quota and period files were combined in the V2 max
+	 * file. The quota is in the first column and if we just print
+	 * a single value to the file, it will be treated as the
+	 * quota. To get or set the period we need to branch on the
+	 * API version.
+	 */
+	{ "cpu.max", "cpu.cfs_quota_us", CTRL_CPU },
+	{ "cpu.cfs_period_us", "cpu.cfs_period_us", CTRL_CPU },
+	{ }
+};
+
 static const files_t cpuset_ctrl_files = {
 	{ "cpuset.cpus", "cpuset.cpus", CTRL_CPUSET },
 	{ "cpuset.mems", "cpuset.mems", CTRL_CPUSET },
@@ -174,6 +187,9 @@  static struct cgroup_ctrl controllers[] = {
 	[CTRL_MEMORY] = {
 		"memory", memory_ctrl_files, CTRL_MEMORY, NULL, 0
 	},
+	[CTRL_CPU] = {
+		"cpu", cpu_ctrl_files, CTRL_CPU, NULL, 0
+	},
 	[CTRL_CPUSET] = {
 		"cpuset", cpuset_ctrl_files, CTRL_CPUSET, NULL, 0
 	},