Re: [PATCH 1/2] cgroup: pids: show number of failed forks since limit reset
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
Re: [PATCH 1/2] cgroup: pids: show number of failed forks since limit reset
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
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
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
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
[PATCH 1/2] cgroup: pids: show number of failed forks since limit reset
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
---
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
*
- * 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
@@ -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
