Skip to contentSkip to conversation

Add a 'trigger' callback on struct cftype.

PATCH In review Pavel Emelyanov 2 participants 7 messages Mar 11, 2008 – Mar 13, 2008 6 messages not in the archive

Subsystems (3)
Memory management cgroup memcg
lore
Share & tools
lore

Pavel Emelyanov

author
#0 lore raw

PATCH 1/1 Add a 'trigger' callback on struct cftype.

If the patch with max_usage for res_counter will be accepted we'll have two :) files, that a event-triggers essentially, i.e. they don't care what the user actually write to then, but are interested in the writing by its own.

So the proposal is to make cgroups infrastructure handle this case.

Signed-off-by Pavel Emelyanov <[email protected]>

---

include/linux/cgroup.h +1 -0
index 785a01c..df579e3 100644
@@ -243,6 +243,7 @@ struct cftype {
 	 */
 	int (*write_s64) (struct cgroup *cgrp, struct cftype *cft, s64 val);
 
+	void (*trigger) (struct cgroup *cgrp, unsigned int event);
 	int (*release) (struct inode *inode, struct file *file);
 };
kernel/cgroup.c +4 -0
index e8e8ec4..7d73c2b 100644
@@ -1410,6 +1410,10 @@ static ssize_t cgroup_file_write(struct file *file, const char __user *buf,
 		return cft->write(cgrp, cft, file, buf, nbytes, ppos);
 	if (cft->write_u64 || cft->write_s64)
 		return cgroup_write_X64(cgrp, cft, file, buf, nbytes, ppos);
+	if (cft->trigger) {
+		cft->trigger(cgrp, (unsigned int)cft->private);
+		return nbytes;
+	}
 	return -EINVAL;
 }
mm/memcontrol.c +6 -19
index c27141d..4c1d24c 100644
@@ -868,27 +868,14 @@ static ssize_t mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
 				mem_cgroup_write_strategy);
 }
 
-static ssize_t mem_cgroup_max_reset(struct cgroup *cont, struct cftype *cft,
-				struct file *file, const char __user *userbuf,
-				size_t nbytes, loff_t *ppos)
+static void mem_cgroup_max_reset(struct cgroup *cont, unsigned int event)
 {
-	struct mem_cgroup *mem;
-
-	mem = mem_cgroup_from_cont(cont);
-	res_counter_reset_max(&mem->res);
-	return nbytes;
+	res_counter_reset_max(&mem_cgroup_from_cont(cont)->res);
 }
 
-static ssize_t mem_force_empty_write(struct cgroup *cont,
-				struct cftype *cft, struct file *file,
-				const char __user *userbuf,
-				size_t nbytes, loff_t *ppos)
+static void mem_force_empty_write(struct cgroup *cont, unsigned int event)
 {
-	struct mem_cgroup *mem = mem_cgroup_from_cont(cont);
-	int ret = mem_cgroup_force_empty(mem);
-	if (!ret)
-		ret = nbytes;
-	return ret;
+	mem_cgroup_force_empty(mem_cgroup_from_cont(cont));
 }
 
 static const struct mem_cgroup_stat_desc {

@@ -936,7 +923,7 @@ static struct cftype mem_cgroup_files[] = {
 	{
 		.name = "max_usage_in_bytes",
 		.private = RES_MAX_USAGE,
-		.write = mem_cgroup_max_reset,
+		.trigger = mem_cgroup_max_reset,
 		.read_u64 = mem_cgroup_read,
 	},
 	{

@@ -952,7 +939,7 @@ static struct cftype mem_cgroup_files[] = {
 	},
 	{
 		.name = "force_empty",
-		.write = mem_force_empty_write,
+		.trigger = mem_force_empty_write,
 	},
 	{
 		.name = "stat",

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 6 lines · Pavel Emelyanov

If the patch with max_usage for res_counter will be accepted we'll have
two :) files, that a event-triggers essentially, i.e. they don't care what the user actually write to then, but are interested in the writing by its own.

So the proposal is to make cgroups infrastructure handle this case.

This could be useful, but in the case of force_empty don't we lose the ability to report an error (EBUSY?) in the event that the cgroup still has tasks?

Paul

A message that is not in the archive (sent off-list or not yet received): 6599ad830803110900j7d47bd6eqf8747bdb704c9226-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

author
#4 lore raw
Quoted 11 lines · Paul Menage
Quoted 6 lines · Pavel Emelyanov

If the patch with max_usage for res_counter will be accepted we'll have
two :) files, that a event-triggers essentially, i.e. they don't care what the user actually write to then, but are interested in the writing by its own.

So the proposal is to make cgroups infrastructure handle this case.

This could be useful, but in the case of force_empty don't we lose the ability to report an error (EBUSY?) in the event that the cgroup still has tasks?

Yikes :( Good catch. The fix, however, is pretty small.

include/linux/cgroup.h +1 -1
index df579e3..f6b882d 100644
@@ -243,7 +243,7 @@ struct cftype {
         */
        int (*write_s64) (struct cgroup *cgrp, struct cftype *cft, s64 val);
 
-       void (*trigger) (struct cgroup *cgrp, unsigned int event);
+       int (*trigger) (struct cgroup *cgrp, unsigned int event);
        int (*release) (struct inode *inode, struct file *file);
 };
kernel/cgroup.c +2 -2
index 7d73c2b..f2d8f25 100644
@@ -1411,8 +1411,8 @@ static ssize_t cgroup_file_write(struct file *file, const char __user *buf,
        if (cft->write_u64 || cft->write_s64)
                return cgroup_write_X64(cgrp, cft, file, buf, nbytes, ppos);
        if (cft->trigger) {
-               cft->trigger(cgrp, (unsigned int)cft->private);
-               return nbytes;
+               int ret = cft->trigger(cgrp, (unsigned int)cft->private);
+               return ret ? ret : nbytes;
        }
        return -EINVAL;
 }
mm/memcontrol.c +4 -3
index 4c1d24c..ab1a862 100644
@@ -868,14 +868,15 @@ static ssize_t mem_cgroup_write(struct cgroup *cont, struct cftype *cft,
                                mem_cgroup_write_strategy);
 }
 
-static void mem_cgroup_max_reset(struct cgroup *cont, unsigned int event)
+static int mem_cgroup_max_reset(struct cgroup *cont, unsigned int event)
 {
        res_counter_reset_max(&mem_cgroup_from_cont(cont)->res);
+       return 0;
 }
 
-static void mem_force_empty_write(struct cgroup *cont, unsigned int event)
+static int mem_force_empty_write(struct cgroup *cont, unsigned int event)
 {
-       mem_cgroup_force_empty(mem_cgroup_from_cont(cont));
+       return mem_cgroup_force_empty(mem_cgroup_from_cont(cont));
 }
 
 static const struct mem_cgroup_stat_desc {
Quoted 2 lines

Paul

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw
Quoted 10 lines · Pavel Emelyanov

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -243,7 +243,7 @@ struct cftype {

         */
        int (*write_s64) (struct cgroup *cgrp, struct cftype *cft, s64 val);

- void (*trigger) (struct cgroup *cgrp, unsigned int event);
+ int (*trigger) (struct cgroup *cgrp, unsigned int event);

To be more name-compatible with the other read_X/write_X functions, how about write_void rather than trigger?

Paul

A message that is not in the archive (sent off-list or not yet received): 6599ad830803110916i1bbc7c7an2412c0facb3496ef-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

author
#8 lore raw
Quoted 5 lines · Paul Menage
Quoted 4 lines · Pavel Emelyanov

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -243,7 +243,7 @@ struct cftype {

Quoted 8 lines
Quoted 5 lines
         */
        int (*write_s64) (struct cgroup *cgrp, struct cftype *cft, s64 val);

- void (*trigger) (struct cgroup *cgrp, unsigned int event);
+ int (*trigger) (struct cgroup *cgrp, unsigned int event);

To be more name-compatible with the other read_X/write_X functions, how about write_void rather than trigger?

Because it's not a write actually, this is just some kick-up which came from the user space. And the fact, that it is triggered via the sys_write is just a VFS-based API constraints. Besides, if we ever have a binary API with cgroups, this trigger can be triggered :) via some other system call, rather than write.

Quoted 2 lines

Paul

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#10 lore raw
Quoted 14 lines · Pavel Emelyanov
Quoted 5 lines · Paul Menage
Quoted 4 lines · Pavel Emelyanov

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -243,7 +243,7 @@ struct cftype {

Quoted 7 lines
Quoted 5 lines
         */
        int (*write_s64) (struct cgroup *cgrp, struct cftype *cft, s64 val);

- void (*trigger) (struct cgroup *cgrp, unsigned int event);
+ int (*trigger) (struct cgroup *cgrp, unsigned int event);

To be more name-compatible with the other read_X/write_X functions, how about write_void rather than trigger?

Quoted 5 lines

Because it's not a write actually, this is just some kick-up which came from the user space. And the fact, that it is triggered via the sys_write is just a VFS-based API constraints. Besides, if we ever have a binary API with cgroups, this trigger can be triggered :) via some other system call, rather than write.

So, Paul, do you have any more objections to the patch? If no, I will prepare the set for Andrew, all the more so, I noticed, that there's no ability to reset the failcounter, which is required and can be easily implemented with the triggers.

Quoted 4 lines
Quoted 2 lines

Paul

A message that is not in the archive (sent off-list or not yet received): [email protected]

#12 lore raw
Quoted 6 lines · Pavel Emelyanov

So, Paul, do you have any more objections to the patch? If no, I will prepare the set for Andrew, all the more so, I noticed, that there's no ability to reset the failcounter, which is required and can be easily implemented with the triggers.

No objections to the implementation, other than not being sure that the name is the best one. But since no-one else seems to have opinions either way, I'm fine with going with the name "trigger".

Paul