Consolidate cgroup files creation for resource counters
Subsystems (3)
cover Consolidate cgroup files creation for resource counters
Right now we have only one controller in -mm tree - the memory one - and it initializes all its files manually. After I developed the kernel memory one it turned out, that the names of resource counter specific files has changed. In the future, if anything in the resource counters change, we'll have to rewrite all the controllers we will have by that time.
To make all the containers working with the resource counters look similar I propose to create the resource counters specific files in one place.
Any comments/wishes before I send it to Andrew are welcome.
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/3 Typedefs the read and write functions in cftype
This is just to reduce the code amount in the future.
---
include/linux/cgroup.h +11 -6
@@ -178,6 +178,15 @@ struct css_set {* - the 'cftype' of the file is file->f_dentry->d_fsdata */ +struct cftype ; + +typedef ssize_t (*cft_read ) (struct cgroup *cont,struct cftype *cft, +struct file *file, +char __user *buf,size_t nbytes,loff_t *ppos); +typedef ssize_t (*cft_write ) (struct cgroup *cont,struct cftype *cft, +struct file *file, +const char __user *buf,size_t nbytes,loff_t *ppos); +#define MAX_CFTYPE_NAME 64struct cftype {/* By convention, the name should begin with the name of the
@@ -185,18 +194,14 @@ struct cftype {char name[MAX_CFTYPE_NAME ];int private;int (*open) (struct inode *inode,struct file *file); -ssize_t (*read) (struct cgroup *cont,struct cftype *cft, -struct file *file, -char __user *buf,size_t nbytes,loff_t *ppos); +cft_read read;/* * read_uint() is a shortcut for the common case of returning a * single integer. Use it in place of read() */ u64 (*read_uint) (struct cgroup *cont,struct cftype *cft); -ssize_t (*write) (struct cgroup *cont,struct cftype *cft, -struct file *file, -const char __user *buf,size_t nbytes,loff_t *ppos); +cft_write write;/* * write_uint() is a shortcut for the common case of accepting * a single integer (as parsed by simple_strtoull) from
PATCH 2/3 Introduce the res_counter_populate() function
In reply to a message not in the archiveThis one is responsible for initializing the RES_CFT_MAX files properly and register them inside the container.
The caller must provide the cgroup, the cgroup_subsys, the RES_CFT_MAX * sizeof(cftype) chunk of uninitialized memory and the read and write callbacks.
In the future, if we add more res_counter files, change their names or anything else, no caller will be affected.
---
include/linux/res_counter.h +5 -0
@@ -58,6 +58,9 @@ ssize_t res_counter_write(struct res_cou const char __user *buf, size_t nbytes, loff_t *pos, int (*write_strategy)(char *buf, unsigned long long *val)); +int res_counter_populate (struct cgroup_subsys *ss,struct cgroup *cont, +struct cftype files[],cft_read read_fn,cft_write write_fn); +/* * the field descriptors. one for each member of res_counter */
@@ -66,6 +69,8 @@ enum {RES_USAGE ,RES_LIMIT ,RES_FAILCNT , + + RES_CFT_MAX, };/*
kernel/res_counter.c +25 -0
@@ -10,9 +10,34 @@#include <linux/types.h> #include <linux/parser.h> #include <linux/fs.h> +#include <linux/cgroup.h> #include <linux/res_counter.h> #include <linux/uaccess.h> +int res_counter_populate (struct cgroup_subsys *ss,struct cgroup *cont, +struct cftype files[],cft_read read_fn,cft_write write_fn) +{ +/* +* to be on the safe side +*/ +memset (files,0 ,RES_CFT_MAX *sizeof (struct cftype )); + +strcpy (files[RES_USAGE ].name,"usage_in_bytes" ); + files[RES_USAGE ].private =RES_USAGE ; + files[RES_USAGE ].read = read_fn; + +strcpy (files[RES_LIMIT ].name,"limit_in_bytes" ); + files[RES_LIMIT ].private =RES_LIMIT ; + files[RES_LIMIT ].read = read_fn; + files[RES_LIMIT ].write = write_fn; + +strcpy (files[RES_FAILCNT ].name,"failcnt" ); + files[RES_FAILCNT ].private =RES_FAILCNT ; + files[RES_FAILCNT ].read = read_fn; + +return cgroup_add_files (cont, ss, files,RES_CFT_MAX ); +} +void res_counter_init (struct res_counter *counter) {spin_lock_init (&counter->lock);
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 2/3] Introduce the res_counter_populate() function
kernel/res_counter.c line 25
Quoted 2 lines · Pavel Emelyanov
kernel/res_counter.c +2
-0+ +strcpy (files[RES_USAGE ].name,"usage_in_bytes" );
This prevents the resource counters from being used for anything that doesn't measure its limit/usage in bytes. E.g. number of tasks.
I like the idea of having a unified interface, but can't they just be called "usage" and "limit"?
Paul
A message that is not in the archive (sent off-list or not yet received): 6599ad830710030902h71737aban4e7cf4e4d1e4d4d1-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Re: [PATCH 2/3] Introduce the res_counter_populate() function
Quoted 9 lines · Paul Menage
kernel/res_counter.c line 25
Quoted 2 lines · Pavel Emelyanov
kernel/res_counter.c +2
-0+ +strcpy (files[RES_USAGE ].name,"usage_in_bytes" );This prevents the resource counters from being used for anything that doesn't measure its limit/usage in bytes. E.g. number of tasks.
I like the idea of having a unified interface, but can't they just be called "usage" and "limit"?
I see. OK, I will rework the patches to make them more generic. Moreover, I've just faced one more race in this set :(
Quoted 2 lines
Paul
Thanks,
Pavel
Re: [PATCH 2/3] Introduce the res_counter_populate() function
In reply to a message not in the archiveQuoted 16 lines · Pavel Emelyanov
include/linux/res_counter.h +3
-0@@ -58,6 +58,9 @@ ssize_t res_counter_write(struct res_cou const char __user *buf, size_t nbytes, loff_t *pos, int (*write_strategy)(char *buf, unsigned long long *val)); +int res_counter_populate (struct cgroup_subsys *ss,struct cgroup *cont, +struct cftype files[],cft_read read_fn,cft_write write_fn); +/* * the field descriptors. one for each member of res_counter */
include/linux/res_counter.h enum { · line 75
Quoted 7 lines
include/linux/res_counter.h +2
-0@@ -66,6 +69,8 @@ enum {RES_USAGE ,RES_LIMIT ,RES_FAILCNT , + + RES_CFT_MAX, };
This is good
Quoted 12 lines
include/linux/res_counter.h +0
-0/* kernel/res_counter.c +1
-0@@ -10,9 +10,34 @@#include <linux/types.h> #include <linux/parser.h> #include <linux/fs.h> +#include <linux/cgroup.h> #include <linux/res_counter.h> #include <linux/uaccess.h>
kernel/res_counter.c line 25
Quoted 10 lines
kernel/res_counter.c +9
-0+int res_counter_populate (struct cgroup_subsys *ss,struct cgroup *cont, +struct cftype files[],cft_read read_fn,cft_write write_fn) +{ +/* +* to be on the safe side +*/ +memset (files,0 ,RES_CFT_MAX *sizeof (struct cftype )); + +strcpy (files[RES_USAGE ].name,"usage_in_bytes" );
This needs to be controller pluggable
kernel/res_counter.c line 29
Quoted 4 lines
kernel/res_counter.c +4
-0+ files[RES_USAGE ].private =RES_USAGE ; + files[RES_USAGE ].read = read_fn; + +strcpy (files[RES_LIMIT ].name,"limit_in_bytes" );
This needs to be controller pluggable
kernel/res_counter.c line 40
Quoted 11 lines
kernel/res_counter.c +11
-0+ files[RES_LIMIT ].private =RES_LIMIT ; + files[RES_LIMIT ].read = read_fn; + files[RES_LIMIT ].write = write_fn; + +strcpy (files[RES_FAILCNT ].name,"failcnt" ); + files[RES_FAILCNT ].private =RES_FAILCNT ; + files[RES_FAILCNT ].read = read_fn; + +return cgroup_add_files (cont, ss, files,RES_CFT_MAX ); +} +
I've been thinking of writing a tool, that will generate all the code necessary to write a controller under cgroups. Another TODO (low priority).
Quoted 8 lines
kernel/res_counter.c +0
-0void res_counter_init (struct res_counter *counter) {spin_lock_init (&counter->lock);_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers
Signature · 4 lines
Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL
PATCH 3/3 Use the res_counter_populate in memory controller
In reply to a message not in the archiveNote, that the controller code dealing with the cftype files for resource counters becomes much shorter and won't have to be changed in the future.
---
mm/memcontrol.c +9 -16
@@ -519,28 +519,14 @@ static ssize_t mem_control_type_read(strstatic struct cftype mem_cgroup_files[] = { { - .name ="usage_in_bytes" , - .private =RES_USAGE , - .read = mem_cgroup_read, - }, - { - .name ="limit_in_bytes" , - .private =RES_LIMIT , - .write = mem_cgroup_write, - .read = mem_cgroup_read, - }, - { - .name ="failcnt" , - .private =RES_FAILCNT , - .read = mem_cgroup_read, - }, - { .name ="control_type" , .write = mem_control_type_write, .read = mem_control_type_read, }, }; +static struct cftype mem_res_counter_files[RES_CFT_MAX ]; +static struct mem_cgroup init_mem_cgroup; static struct cgroup_subsys_state *
@@ -574,6 +560,13 @@ static void mem_cgroup_destroy(struct cgstatic int mem_cgroup_populate (struct cgroup_subsys *ss,struct cgroup *cont) { +int ret; + + ret =res_counter_populate (ss, cont, mem_res_counter_files, + mem_cgroup_read, mem_cgroup_write); +if (ret) +return ret; +return cgroup_add_files (cont, ss, mem_cgroup_files,ARRAY_SIZE (mem_cgroup_files)); }
Quoted 7 lines · Pavel Emelyanov
Right now we have only one controller in -mm tree - the memory one - and it initializes all its files manually. After I developed the kernel memory one it turned out, that the names of resource counter specific files has changed. In the future, if anything in the resource counters change, we'll have to rewrite all the controllers we will have by that time.
Quoted 7 lines
To make all the containers working with the resource counters look similar I propose to create the resource counters specific files in one place.
Any comments/wishes before I send it to Andrew are welcome.
Signed-off-by Pavel Emelyanov <[email protected]>
For memory your changes make sense, but the file names cannot be limit_in_bytes and usage_in_bytes for other controllers (if and when they use res_counters)
Signature · 4 lines
Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL