[PATCH 0/2] cgroup: pids: extend pids.events

[PATCH 0/2] cgroup: pids: extend pids.events

[Aleksa Sarai]

While reading the patchset by Kenny Yu, I realised that not having a
field for the "recent" number of failed forks means that userspace would
have trouble accurately deciding whether or not it should increase the
limits.

In addition, by having hits_since, we get to maintain the
on-reset-we-log-failures-again semantics that Tejun said he liked.

Aleksa Sarai (2):
  cgroup: pids: show number of failed forks since limit reset
  docs: cgroup/pids: update documentation to include pids.events

 Documentation/cgroup-v1/pids.txt | 18 ++++++++++++++++++
 kernel/cgroup_pids.c             | 31 ++++++++++++++++++++++---------
 2 files changed, 40 insertions(+), 9 deletions(-)

-- 
2.8.4

[PATCH 1/2] cgroup: pids: show number of failed forks since limit reset

[Aleksa Sarai]

This allows users to dynamically adjust their limits based on how many
failed forks happened since they last reset their limits, otherwise they
would have to track (in a racy way) how many limit failures there were
since the last limit change manually. In addition, we log the first
failure since the limit was reset (which was the original semantics of
the patchset).

In addition, I clarified the licensing for this file.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
---
 kernel/cgroup_pids.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/kernel/cgroup_pids.c b/kernel/cgroup_pids.c
index 2bd673783f1a..54ec3e4f3b71 100644
--- a/kernel/cgroup_pids.c
+++ b/kernel/cgroup_pids.c
@@ -26,9 +26,10 @@
  *
  * Copyright (C) 2015 Aleksa Sarai <cyphar@cyphar.com>
  *
- * This file is subject to the terms and conditions of version 2 of the GNU
- * General Public License.  See the file COPYING in the main directory of the
- * Linux distribution for more details.
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
  */
 
 #include <linux/kernel.h>
@@ -53,8 +54,11 @@ struct pids_cgroup {
 	/* Handle for "pids.events" */
 	struct cgroup_file		events_file;
 
-	/* Number of times fork failed because limit was hit. */
-	atomic64_t			events_limit;
+	/* Total number of times fork failed because limit was hit. */
+	atomic64_t			hits_total;
+
+	/* Number of times fork failed since limit was last set. */
+	atomic64_t			hits_since;
 };
 
 static struct pids_cgroup *css_pids(struct cgroup_subsys_state *css)
@@ -78,7 +82,8 @@ pids_css_alloc(struct cgroup_subsys_state *parent)
 
 	pids->limit = PIDS_MAX;
 	atomic64_set(&pids->counter, 0);
-	atomic64_set(&pids->events_limit, 0);
+	atomic64_set(&pids->hits_total, 0);
+	atomic64_set(&pids->hits_since, 0);
 	return &pids->css;
 }
 
@@ -226,8 +231,12 @@ static int pids_can_fork(struct task_struct *task)
 	pids = css_pids(css);
 	err = pids_try_charge(pids, 1);
 	if (err) {
-		/* Only log the first time events_limit is incremented. */
-		if (atomic64_inc_return(&pids->events_limit) == 1) {
+		/*
+		 * We only log the first time a fork fails after a limit has
+		 * been set. Resetting the limit will cause us to log again.
+		 */
+		atomic64_inc(&pids->hits_total);
+		if (atomic64_inc_return(&pids->hits_since) == 1) {
 			pr_info("cgroup: fork rejected by pids controller in ");
 			pr_cont_cgroup_path(task_cgroup(current, pids_cgrp_id));
 			pr_cont("\n");
@@ -281,6 +290,9 @@ set_limit:
 	 * critical that any racing fork()s follow the new limit.
 	 */
 	pids->limit = limit;
+	/* Reset the since counter and notify userspace. */
+	atomic64_set(&pids->hits_since, 0);
+	cgroup_file_notify(&pids->events_file);
 	return nbytes;
 }
 
@@ -310,7 +322,8 @@ static int pids_events_show(struct seq_file *sf, void *v)
 {
 	struct pids_cgroup *pids = css_pids(seq_css(sf));
 
-	seq_printf(sf, "max %lld\n", (s64)atomic64_read(&pids->events_limit));
+	seq_printf(sf, "since\t%lld\n", (s64)atomic64_read(&pids->hits_since));
+	seq_printf(sf, "total\t%lld\n", (s64)atomic64_read(&pids->hits_total));
 	return 0;
 }
 
-- 
2.8.4

[PATCH 2/2] docs: cgroup/pids: update documentation to include pids.events

[Aleksa Sarai]

So that users know what the interface and meaning of the keyed values
are. In addition, mention that the only time that since=0 is when the
limit was changed.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
---
 Documentation/cgroup-v1/pids.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/cgroup-v1/pids.txt b/Documentation/cgroup-v1/pids.txt
index 1a078b5d281a..a9bb7b964c6f 100644
--- a/Documentation/cgroup-v1/pids.txt
+++ b/Documentation/cgroup-v1/pids.txt
@@ -33,6 +33,11 @@ limit in the hierarchy is followed).
 pids.current tracks all child cgroup hierarchies, so parent/pids.current is a
 superset of parent/child/pids.current.
 
+pids.events shows information about the number of failed forks in a particular
+cgroup, both overall (since the cgroup was created) and recently (since the
+last limit reset). Userspace is notified of each time a process failed to fork
+in a cgroup.
+
 Example
 -------
 
@@ -83,3 +88,16 @@ sh: fork: Resource temporary unavailable
 # /bin/echo "We can't even spawn a single process now."
 sh: fork: Resource temporary unavailable
 #
+
+We can also see how many times a particular cgroup has failed to fork. For an
+example cgroup:
+
+# cat /sys/fs/cgroup/pids/some_cgroup/pids.events
+since	1
+total	12
+#
+
+This cgroup has had 12 associated process fail to fork throughout its lifetime,
+and has had 1 process fail to fork since the limit was last set. On setting the
+limit, the since counter becomes 0 and userspace is notified (this is the only
+case where since will be 0 and userspace will get a notification).
-- 
2.8.4

Re: [PATCH 1/2] cgroup: pids: show number of failed forks since limit reset

[Tejun Heo]

Hello,

On Fri, Jun 24, 2016 at 01:00:48PM +1000, Aleksa Sarai wrote:
> This allows users to dynamically adjust their limits based on how many
> failed forks happened since they last reset their limits, otherwise they
> would have to track (in a racy way) how many limit failures there were
> since the last limit change manually. In addition, we log the first
> failure since the limit was reset (which was the original semantics of
> the patchset).

Isn't that trivially achievable by reading the counter and then
calculating the diff?  I don't think it matters all that much whether
the log message is printed once per cgroup or per config-change.  It's
just a hint for the admin to avoid setting her off on a wild goose
chase.

Thanks.

-- 
tejun

Re: [PATCH 1/2] cgroup: pids: show number of failed forks since limit reset

[Aleksa Sarai]

> On Fri, Jun 24, 2016 at 01:00:48PM +1000, Aleksa Sarai wrote:
>> This allows users to dynamically adjust their limits based on how many
>> failed forks happened since they last reset their limits, otherwise they
>> would have to track (in a racy way) how many limit failures there were
>> since the last limit change manually. In addition, we log the first
>> failure since the limit was reset (which was the original semantics of
>> the patchset).
>
> Isn't that trivially achievable by reading the counter and then
> calculating the diff?  I don't think it matters all that much whether
> the log message is printed once per cgroup or per config-change.  It's
> just a hint for the admin to avoid setting her off on a wild goose
> chase.

If a user has a setup where they wait for notifications on changes to 
pids.event, and then auto-adjust the cgroup limits based on the number 
of failures you have a race condition between reading the pids.event 
file and then setting the new limit. Then, upon getting notified again 
there may have been many failed forks with the old limit set, so you 
might decide to bump up the limit again.

It's not a huge deal, I just though it could be useful to alleviate 
problems like the above.

-- 
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Re: [PATCH 1/2] cgroup: pids: show number of failed forks since limit reset

[Tejun Heo]

Hello,

On Sun, Jun 26, 2016 at 09:34:41PM +1000, Aleksa Sarai wrote:
> If a user has a setup where they wait for notifications on changes to
> pids.event, and then auto-adjust the cgroup limits based on the number of
> failures you have a race condition between reading the pids.event file and
> then setting the new limit. Then, upon getting notified again there may have
> been many failed forks with the old limit set, so you might decide to bump
> up the limit again.
> 
> It's not a huge deal, I just though it could be useful to alleviate problems
> like the above.

This is something which can easily be avoided from userland.  I don't
think we need to add extra facilities for this.

Thanks.

-- 
tejun

Re: [PATCH 1/2] cgroup: pids: show number of failed forks since limit reset

[Aleksa Sarai]

> On Sun, Jun 26, 2016 at 09:34:41PM +1000, Aleksa Sarai wrote:
>> If a user has a setup where they wait for notifications on changes to
>> pids.event, and then auto-adjust the cgroup limits based on the number of
>> failures you have a race condition between reading the pids.event file and
>> then setting the new limit. Then, upon getting notified again there may have
>> been many failed forks with the old limit set, so you might decide to bump
>> up the limit again.
>>
>> It's not a huge deal, I just though it could be useful to alleviate problems
>> like the above.
>
> This is something which can easily be avoided from userland.  I don't
> think we need to add extra facilities for this.

Maybe I'm misunderstanding what the purpose of pids.events is meant to 
be. If it's just meant to be a "hint" to the administrator, then that 
seems like an awfully odd way of giving hints (the kernel logging should 
be enough for that). If it's meant so that the administrator can make 
policy decisions, then she won't know if the change in failures happened 
before or after she adjusted the limit.

I also don't see what you mean by "it can be avoided from userland". 
There's a race between changing pids.max and a process forking that 
causes pids.events to be updated.

-- 
Aleksa Sarai
Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Re: [PATCH 1/2] cgroup: pids: show number of failed forks since limit reset

[Tejun Heo]

Hello, Aleksa.

On Wed, Jun 29, 2016 at 03:10:26PM +1000, Aleksa Sarai wrote:
> Maybe I'm misunderstanding what the purpose of pids.events is meant to be.
> If it's just meant to be a "hint" to the administrator, then that seems like
> an awfully odd way of giving hints (the kernel logging should be enough for
> that). If it's meant so that the administrator can make policy decisions,

Kernel logging may be an enough hint for a human admin but is an awful
mechanism for any programs trying to monitor.

> then she won't know if the change in failures happened before or after she
> adjusted the limit.
> 
> I also don't see what you mean by "it can be avoided from userland". There's
> a race between changing pids.max and a process forking that causes
> pids.events to be updated.

First of all, a situation where a cgroup hits its limits and then can
proceed normally after the limit is lifted would be very rare.
Secondly, even in those corner cases, synchronization between config
update and event counter doesn't matter.  For dynamic adjustments to
work, fork failures have to be cleanly recoverable and thus the user
can just start monitoring after the config is bumped up - if it fails
again, adjust again; otherwise, it's all good.  It just doesn't
matter.

Thanks.

-- 
tejun