[PATCH 0/2] help configure some cgroup limits (v2)

[PATCH 0/2] help configure some cgroup limits (v2)

[Topi Miettinen]

Hello,

There are many basic ways to control processes, including capabilities,
cgroups and resource limits. However, there are far fewer ways to find out
useful values for the limits, except blind trial and error.

This patch series attempts to fix that by giving at least a nice starting
point for configuration of PID and device cgroups.

Thanks to the commenters for the previous version.

-Topi

Topi Miettinen (2):
  cgroup_pids: track highwater mark of pids
  device_cgroup: track and present accessed devices

 kernel/cgroup_pids.c     | 51 ++++++++++++++++++++++++++--
 security/device_cgroup.c | 86 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 117 insertions(+), 20 deletions(-)

-- 
2.8.1

[PATCH 1/2] cgroup_pids: highwater mark of pids

[Topi Miettinen]

Track maximum number of processes in cgroup, to be able to configure
cgroup pids limits. The information is available in cgroup FS as file
pids.highwater_mark.

Example case demonstrating how to use the figure for systemd configuration:
root@debian:~# cat /sys/fs/cgroup/system.slice/systemd-timesyncd.service/pids.highwater_mark
2
root@debian:~# cat /etc/systemd/system/systemd-timesyncd.service.d/local.conf
[Service]
TasksMax=2
root@debian:~# systemctl status systemd-timesyncd.service | grep Tasks
    Tasks: 2 (limit: 2)

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 kernel/cgroup_pids.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_pids.c b/kernel/cgroup_pids.c
index 303097b..da5a696 100644
--- a/kernel/cgroup_pids.c
+++ b/kernel/cgroup_pids.c
@@ -48,6 +48,7 @@ struct pids_cgroup {
 	 * %PIDS_MAX = (%PID_MAX_LIMIT + 1).
 	 */
 	atomic64_t			counter;
+	atomic64_t			highwater_mark;
 	int64_t				limit;
 };
 
@@ -72,6 +73,7 @@ pids_css_alloc(struct cgroup_subsys_state *parent)
 
 	pids->limit = PIDS_MAX;
 	atomic64_set(&pids->counter, 0);
+	atomic64_set(&pids->highwater_mark, 0);
 	return &pids->css;
 }
 
@@ -80,6 +82,25 @@ static void pids_css_free(struct cgroup_subsys_state *css)
 	kfree(css_pids(css));
 }
 
+static void pids_update_highwater_mark(struct pids_cgroup *p)
+{
+	while (1) {
+		int64_t old_mark, new_mark, cur_mark;
+
+		old_mark = atomic64_read(&p->highwater_mark);
+		new_mark = atomic64_read(&p->counter);
+		if (old_mark >= new_mark)
+			return;
+		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
+					    new_mark);
+
+		/* It's OK if the counter was decreased meanwhile */
+		if (cur_mark == old_mark &&
+		    atomic64_read(&p->counter) <= new_mark)
+			return;
+	}
+}
+
 /**
  * pids_cancel - uncharge the local pid count
  * @pids: the pid cgroup state
@@ -106,8 +127,10 @@ static void pids_uncharge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		pids_cancel(p, num);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -123,8 +146,10 @@ static void pids_charge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		atomic64_add(num, &p->counter);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -152,6 +177,7 @@ static int pids_try_charge(struct pids_cgroup *pids, int num)
 			goto revert;
 	}
 
+	pids_update_highwater_mark(p);
 	return 0;
 
 revert:
@@ -236,6 +262,13 @@ static void pids_free(struct task_struct *task)
 	pids_uncharge(pids, 1);
 }
 
+static void pids_fork(struct task_struct *task)
+{
+	struct pids_cgroup *pids = css_pids(task_css(task, pids_cgrp_id));
+
+	pids_update_highwater_mark(pids);
+}
+
 static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf,
 			      size_t nbytes, loff_t off)
 {
@@ -288,6 +321,14 @@ static s64 pids_current_read(struct cgroup_subsys_state *css,
 	return atomic64_read(&pids->counter);
 }
 
+static s64 pids_highwater_mark_read(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	struct pids_cgroup *pids = css_pids(css);
+
+	return atomic64_read(&pids->highwater_mark);
+}
+
 static struct cftype pids_files[] = {
 	{
 		.name = "max",
@@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
 		.read_s64 = pids_current_read,
 		.flags = CFTYPE_NOT_ON_ROOT,
 	},
+	{
+		.name = "highwater_mark",
+		.read_s64 = pids_highwater_mark_read,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
 	{ }	/* terminate */
 };
 
@@ -313,4 +359,5 @@ struct cgroup_subsys pids_cgrp_subsys = {
 	.free		= pids_free,
 	.legacy_cftypes	= pids_files,
 	.dfl_cftypes	= pids_files,
+	.fork		= pids_fork,
 };
-- 
2.8.1

[PATCH 1/2] cgroup_pids: track highwater mark of pids

[Topi Miettinen]

Track maximum number of processes in cgroup, to be able to configure
cgroup pids limits. The information is available in cgroup FS as file
pids.highwater_mark.

Example case demonstrating how to use the figure for systemd configuration:
root@debian:~# cat /sys/fs/cgroup/system.slice/systemd-timesyncd.service/pids.highwater_mark
2
root@debian:~# cat /etc/systemd/system/systemd-timesyncd.service.d/local.conf
[Service]
TasksMax=2
root@debian:~# systemctl status systemd-timesyncd.service | grep Tasks
    Tasks: 2 (limit: 2)

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 kernel/cgroup_pids.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_pids.c b/kernel/cgroup_pids.c
index 303097b..da5a696 100644
--- a/kernel/cgroup_pids.c
+++ b/kernel/cgroup_pids.c
@@ -48,6 +48,7 @@ struct pids_cgroup {
 	 * %PIDS_MAX = (%PID_MAX_LIMIT + 1).
 	 */
 	atomic64_t			counter;
+	atomic64_t			highwater_mark;
 	int64_t				limit;
 };
 
@@ -72,6 +73,7 @@ pids_css_alloc(struct cgroup_subsys_state *parent)
 
 	pids->limit = PIDS_MAX;
 	atomic64_set(&pids->counter, 0);
+	atomic64_set(&pids->highwater_mark, 0);
 	return &pids->css;
 }
 
@@ -80,6 +82,25 @@ static void pids_css_free(struct cgroup_subsys_state *css)
 	kfree(css_pids(css));
 }
 
+static void pids_update_highwater_mark(struct pids_cgroup *p)
+{
+	while (1) {
+		int64_t old_mark, new_mark, cur_mark;
+
+		old_mark = atomic64_read(&p->highwater_mark);
+		new_mark = atomic64_read(&p->counter);
+		if (old_mark >= new_mark)
+			return;
+		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
+					    new_mark);
+
+		/* It's OK if the counter was decreased meanwhile */
+		if (cur_mark == old_mark &&
+		    atomic64_read(&p->counter) <= new_mark)
+			return;
+	}
+}
+
 /**
  * pids_cancel - uncharge the local pid count
  * @pids: the pid cgroup state
@@ -106,8 +127,10 @@ static void pids_uncharge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		pids_cancel(p, num);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -123,8 +146,10 @@ static void pids_charge(struct pids_cgroup *pids, int num)
 {
 	struct pids_cgroup *p;
 
-	for (p = pids; parent_pids(p); p = parent_pids(p))
+	for (p = pids; parent_pids(p); p = parent_pids(p)) {
 		atomic64_add(num, &p->counter);
+		pids_update_highwater_mark(p);
+	}
 }
 
 /**
@@ -152,6 +177,7 @@ static int pids_try_charge(struct pids_cgroup *pids, int num)
 			goto revert;
 	}
 
+	pids_update_highwater_mark(p);
 	return 0;
 
 revert:
@@ -236,6 +262,13 @@ static void pids_free(struct task_struct *task)
 	pids_uncharge(pids, 1);
 }
 
+static void pids_fork(struct task_struct *task)
+{
+	struct pids_cgroup *pids = css_pids(task_css(task, pids_cgrp_id));
+
+	pids_update_highwater_mark(pids);
+}
+
 static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf,
 			      size_t nbytes, loff_t off)
 {
@@ -288,6 +321,14 @@ static s64 pids_current_read(struct cgroup_subsys_state *css,
 	return atomic64_read(&pids->counter);
 }
 
+static s64 pids_highwater_mark_read(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	struct pids_cgroup *pids = css_pids(css);
+
+	return atomic64_read(&pids->highwater_mark);
+}
+
 static struct cftype pids_files[] = {
 	{
 		.name = "max",
@@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
 		.read_s64 = pids_current_read,
 		.flags = CFTYPE_NOT_ON_ROOT,
 	},
+	{
+		.name = "highwater_mark",
+		.read_s64 = pids_highwater_mark_read,
+		.flags = CFTYPE_NOT_ON_ROOT,
+	},
 	{ }	/* terminate */
 };
 
@@ -313,4 +359,5 @@ struct cgroup_subsys pids_cgrp_subsys = {
 	.free		= pids_free,
 	.legacy_cftypes	= pids_files,
 	.dfl_cftypes	= pids_files,
+	.fork		= pids_fork,
 };
-- 
2.8.1

Re: [PATCH 1/2] cgroup_pids: track highwater mark of pids

[Tejun Heo]

Hello,

On Sun, Jul 17, 2016 at 11:03:38PM +0300, Topi Miettinen wrote:
> +static void pids_update_highwater_mark(struct pids_cgroup *p)
> +{
> +	while (1) {
> +		int64_t old_mark, new_mark, cur_mark;
> +
> +		old_mark = atomic64_read(&p->highwater_mark);
> +		new_mark = atomic64_read(&p->counter);
> +		if (old_mark >= new_mark)
> +			return;
> +		cur_mark = atomic64_cmpxchg(&p->highwater_mark, old_mark,
> +					    new_mark);
> +
> +		/* It's OK if the counter was decreased meanwhile */
> +		if (cur_mark == old_mark &&
> +		    atomic64_read(&p->counter) <= new_mark)
> +			return;
> +	}
> +}

I think it'd be better to make this part of pids_charge() - maybe use
atomic64_add_return() to get the current value and track the maximum?

> @@ -300,6 +341,11 @@ static struct cftype pids_files[] = {
>  		.read_s64 = pids_current_read,
>  		.flags = CFTYPE_NOT_ON_ROOT,
>  	},
> +	{
> +		.name = "highwater_mark",
> +		.read_s64 = pids_highwater_mark_read,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},

This should be an entry in the pids.stats file.  Please also update
the documentation accordingly.

Thanks.

-- 
tejun