* [PATCH v5 0/6] clone3 & cgroups: allow spawning processes into cgroups
From: Christian Brauner @ 2020-01-21 15:48 UTC (permalink / raw)
To: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Tejun Heo
Cc: Oleg Nesterov, Christian Brauner
Hey Tejun,
This is v5 of the promised series to enable spawning processes into a
target cgroup different from the parent's cgroup.
/* v1 */
Link: https://lore.kernel.org/r/20191218173516.7875-1-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org
/* v2 */
Link: https://lore.kernel.org/r/20191223061504.28716-1-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org
Rework locking and remove unneeded helper functions. Please see
individual patch changelogs for details.
With this I've been able to run the cgroup selftests and stress tests in
loops for a long time without any regressions or deadlocks; lockdep and
kasan did not complain either.
/* v3 */
Link: https://lore.kernel.org/r/20200117002143.15559-1-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org
Split preliminary work into separate patches.
See changelog of individual commits.
/* v4 */
Link: https://lore.kernel.org/r/20200117181219.14542-1-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org
Verify that we have write access to the target cgroup. This is usually
done by the vfs but since we aren't going through the vfs with
CLONE_INTO_CGROUP we need to do it ourselves.
/* v5 */
Don't pass down the parent task_struct as argument, just use current
directly. Put kargs->cset on error.
With this cgroup migration will be a lot easier, and accounting will be
more exact. It also allows for nice features such as creating a frozen
process by spawning it into a frozen cgroup.
The code simplifies container creation and exec logic quite a bit as
well.
I've tried to contain all core changes for this features in
kernel/cgroup/* to avoid exposing cgroup internals. This has mostly
worked.
When a new process is supposed to be spawned in a cgroup different from
the parent's then we briefly acquire the cgroup mutex right before
fork()'s point of no return and drop it once the child process has been
attached to the tasklist and to its css_set. This is done to ensure that
the cgroup isn't removed behind our back. The cgroup mutex is _only_
held in this case; the usual case, where the child is created in the
same cgroup as the parent does not acquire it since the cgroup can't be
removed.
The series already comes with proper testing. Once we've decided that
this approach is good I'll expand the test-suite even more.
The branch can be found in the following locations:
[1]: kernel.org: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=clone_into_cgroup
[2]: github.com: https://github.com/brauner/linux/tree/clone_into_cgroup
[3]: gitlab.com: https://gitlab.com/brauner/linux/commits/clone_into_cgroup
Thanks!
Christian
Christian Brauner (6):
cgroup: unify attach permission checking
cgroup: add cgroup_get_from_file() helper
cgroup: refactor fork helpers
cgroup: add cgroup_may_write() helper
clone3: allow spawning processes into cgroups
selftests/cgroup: add tests for cloning into cgroups
include/linux/cgroup-defs.h | 6 +-
include/linux/cgroup.h | 20 +-
include/linux/sched/task.h | 4 +
include/uapi/linux/sched.h | 5 +
kernel/cgroup/cgroup.c | 297 ++++++++++++++----
kernel/cgroup/pids.c | 15 +-
kernel/fork.c | 19 +-
tools/testing/selftests/cgroup/Makefile | 6 +-
tools/testing/selftests/cgroup/cgroup_util.c | 126 ++++++++
tools/testing/selftests/cgroup/cgroup_util.h | 4 +
tools/testing/selftests/cgroup/test_core.c | 64 ++++
.../selftests/clone3/clone3_selftests.h | 19 +-
12 files changed, 501 insertions(+), 84 deletions(-)
base-commit: b3a987b0264d3ddbb24293ebff10eddfc472f653
--
2.25.0
^ permalink raw reply
* [PATCH v5 1/6] cgroup: unify attach permission checking
From: Christian Brauner @ 2020-01-21 15:48 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo
Cc: Oleg Nesterov, Christian Brauner, Li Zefan, Johannes Weiner,
cgroups
In-Reply-To: <20200121154844.411-1-christian.brauner@ubuntu.com>
The core codepaths to check whether a process can be attached to a
cgroup are the same for threads and thread-group leaders. Only a small
piece of code verifying that source and destination cgroup are in the
same domain differentiates the thread permission checking from
thread-group leader permission checking.
Since cgroup_migrate_vet_dst() only matters cgroup2 - it is a noop on
cgroup1 - we can move it out of cgroup_attach_task().
All checks can now be consolidated into a new helper
cgroup_attach_permissions() callable from both cgroup_procs_write() and
cgroup_threads_write().
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: cgroups@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191218173516.7875-2-christian.brauner@ubuntu.com
/* v2 */
Link: https://lore.kernel.org/r/20191223061504.28716-2-christian.brauner@ubuntu.com
- Christian Brauner <christian.brauner@ubuntu.com>:
- Fix return value of cgroup_attach_permissions. It used to return 0
when it should've returned -EOPNOTSUPP.
- Fix call to cgroup_attach_permissions() in cgroup_procs_write(). It
accidently specified that a thread was moved causing an additional
check for domain-group equality to be executed that is not needed.
/* v3 */
Link: https://lore.kernel.org/r/20200117002143.15559-2-christian.brauner@ubuntu.com
unchanged
/* v4 */
Link: https://lore.kernel.org/r/20200117181219.14542-2-christian.brauner@ubuntu.com
unchanged
/* v5 */
unchanged
---
kernel/cgroup/cgroup.c | 39 +++++++++++++++++++++++++--------------
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 735af8f15f95..7b98cc389dae 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2719,11 +2719,7 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader,
{
DEFINE_CGROUP_MGCTX(mgctx);
struct task_struct *task;
- int ret;
-
- ret = cgroup_migrate_vet_dst(dst_cgrp);
- if (ret)
- return ret;
+ int ret = 0;
/* look up all src csets */
spin_lock_irq(&css_set_lock);
@@ -4690,6 +4686,26 @@ static int cgroup_procs_write_permission(struct cgroup *src_cgrp,
return 0;
}
+static int cgroup_attach_permissions(struct cgroup *src_cgrp,
+ struct cgroup *dst_cgrp,
+ struct super_block *sb, bool thread)
+{
+ int ret = 0;
+
+ ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, sb);
+ if (ret)
+ return ret;
+
+ ret = cgroup_migrate_vet_dst(dst_cgrp);
+ if (ret)
+ return ret;
+
+ if (thread && (src_cgrp->dom_cgrp != dst_cgrp->dom_cgrp))
+ ret = -EOPNOTSUPP;
+
+ return ret;
+}
+
static ssize_t cgroup_procs_write(struct kernfs_open_file *of,
char *buf, size_t nbytes, loff_t off)
{
@@ -4712,8 +4728,8 @@ static ssize_t cgroup_procs_write(struct kernfs_open_file *of,
src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root);
spin_unlock_irq(&css_set_lock);
- ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp,
- of->file->f_path.dentry->d_sb);
+ ret = cgroup_attach_permissions(src_cgrp, dst_cgrp,
+ of->file->f_path.dentry->d_sb, false);
if (ret)
goto out_finish;
@@ -4757,16 +4773,11 @@ static ssize_t cgroup_threads_write(struct kernfs_open_file *of,
spin_unlock_irq(&css_set_lock);
/* thread migrations follow the cgroup.procs delegation rule */
- ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp,
- of->file->f_path.dentry->d_sb);
+ ret = cgroup_attach_permissions(src_cgrp, dst_cgrp,
+ of->file->f_path.dentry->d_sb, true);
if (ret)
goto out_finish;
- /* and must be contained in the same domain */
- ret = -EOPNOTSUPP;
- if (src_cgrp->dom_cgrp != dst_cgrp->dom_cgrp)
- goto out_finish;
-
ret = cgroup_attach_task(dst_cgrp, task, false);
out_finish:
--
2.25.0
^ permalink raw reply related
* [PATCH v5 2/6] cgroup: add cgroup_get_from_file() helper
From: Christian Brauner @ 2020-01-21 15:48 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo
Cc: Oleg Nesterov, Christian Brauner, Johannes Weiner, Li Zefan,
cgroups
In-Reply-To: <20200121154844.411-1-christian.brauner@ubuntu.com>
Add a helper cgroup_get_from_file(). The helper will be used in
subsequent patches to retrieve a cgroup while holding a reference to the
struct file it was taken from.
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: cgroups@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
patch not present
/* v2 */
patch not present
/* v3 */
Link: https://lore.kernel.org/r/20200117002143.15559-3-christian.brauner@ubuntu.com
patch introduced
- Tejun Heo <tj@kernel.org>:
- split cgroup_get_from_file() changes into separate commmit
/* v4 */
Link: https://lore.kernel.org/r/20200117181219.14542-3-christian.brauner@ubuntu.com
unchanged
/* v5 */
unchanged
---
kernel/cgroup/cgroup.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 7b98cc389dae..9b3241d67592 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5875,6 +5875,24 @@ void cgroup_fork(struct task_struct *child)
INIT_LIST_HEAD(&child->cg_list);
}
+static struct cgroup *cgroup_get_from_file(struct file *f)
+{
+ struct cgroup_subsys_state *css;
+ struct cgroup *cgrp;
+
+ css = css_tryget_online_from_dir(f->f_path.dentry, NULL);
+ if (IS_ERR(css))
+ return ERR_CAST(css);
+
+ cgrp = css->cgroup;
+ if (!cgroup_on_dfl(cgrp)) {
+ cgroup_put(cgrp);
+ return ERR_PTR(-EBADF);
+ }
+
+ return cgrp;
+}
+
/**
* cgroup_can_fork - called on a new task before the process is exposed
* @child: the task in question.
@@ -6163,7 +6181,6 @@ EXPORT_SYMBOL_GPL(cgroup_get_from_path);
*/
struct cgroup *cgroup_get_from_fd(int fd)
{
- struct cgroup_subsys_state *css;
struct cgroup *cgrp;
struct file *f;
@@ -6171,17 +6188,8 @@ struct cgroup *cgroup_get_from_fd(int fd)
if (!f)
return ERR_PTR(-EBADF);
- css = css_tryget_online_from_dir(f->f_path.dentry, NULL);
+ cgrp = cgroup_get_from_file(f);
fput(f);
- if (IS_ERR(css))
- return ERR_CAST(css);
-
- cgrp = css->cgroup;
- if (!cgroup_on_dfl(cgrp)) {
- cgroup_put(cgrp);
- return ERR_PTR(-EBADF);
- }
-
return cgrp;
}
EXPORT_SYMBOL_GPL(cgroup_get_from_fd);
--
2.25.0
^ permalink raw reply related
* [PATCH v5 3/6] cgroup: refactor fork helpers
From: Christian Brauner @ 2020-01-21 15:48 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo
Cc: Oleg Nesterov, Christian Brauner, Johannes Weiner, Li Zefan,
cgroups
In-Reply-To: <20200121154844.411-1-christian.brauner@ubuntu.com>
This refactors the fork helpers so they can be easily modified in the
next patches. The patch just moves the cgroup threadgroup rwsem grab and
release into the helpers. They don't need to be directly exposed in fork.c.
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: cgroups@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
patch not present
/* v2 */
patch not present
/* v3 */
Link: https://lore.kernel.org/r/20200117002143.15559-4-christian.brauner@ubuntu.com
patch introduced
- Tejun Heo <tj@kernel.org>:
- split into separate commmit
/* v4 */
Link: https://lore.kernel.org/r/20200117181219.14542-4-christian.brauner@ubuntu.com
unchanged
/* v5 */
- Oleg Nesterov <oleg@redhat.com>:
- remove struct task_struct *parent argument from clone helpers in favor of
using current directly
- Christian Brauner <christian.brauner@ubuntu.com>:
- fix typo in commit message
---
kernel/cgroup/cgroup.c | 47 ++++++++++++++++++++++++++----------------
kernel/fork.c | 6 +-----
2 files changed, 30 insertions(+), 23 deletions(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 9b3241d67592..ce2d5b8aa19f 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5895,17 +5895,21 @@ static struct cgroup *cgroup_get_from_file(struct file *f)
/**
* cgroup_can_fork - called on a new task before the process is exposed
- * @child: the task in question.
+ * @child: the child process
+ * @kargs: the arguments passed to create the child process
*
- * This calls the subsystem can_fork() callbacks. If the can_fork() callback
- * returns an error, the fork aborts with that error code. This allows for
- * a cgroup subsystem to conditionally allow or deny new forks.
+ * This calls the subsystem can_fork() callbacks. If the cgroup_can_fork()
+ * callback returns an error, the fork aborts with that error code. This
+ * allows for a cgroup subsystem to conditionally allow or deny new forks.
*/
int cgroup_can_fork(struct task_struct *child)
+ __acquires(&cgroup_threadgroup_rwsem) __releases(&cgroup_threadgroup_rwsem)
{
struct cgroup_subsys *ss;
int i, j, ret;
+ cgroup_threadgroup_change_begin(current);
+
do_each_subsys_mask(ss, i, have_canfork_callback) {
ret = ss->can_fork(child);
if (ret)
@@ -5922,17 +5926,21 @@ int cgroup_can_fork(struct task_struct *child)
ss->cancel_fork(child);
}
+ cgroup_threadgroup_change_end(current);
+
return ret;
}
/**
- * cgroup_cancel_fork - called if a fork failed after cgroup_can_fork()
- * @child: the task in question
- *
- * This calls the cancel_fork() callbacks if a fork failed *after*
- * cgroup_can_fork() succeded.
- */
+ * cgroup_cancel_fork - called if a fork failed after cgroup_can_fork()
+ * @child: the child process
+ * @kargs: the arguments passed to create the child process
+ *
+ * This calls the cancel_fork() callbacks if a fork failed *after*
+ * cgroup_can_fork() succeded.
+ */
void cgroup_cancel_fork(struct task_struct *child)
+ __releases(&cgroup_threadgroup_rwsem)
{
struct cgroup_subsys *ss;
int i;
@@ -5940,19 +5948,20 @@ void cgroup_cancel_fork(struct task_struct *child)
for_each_subsys(ss, i)
if (ss->cancel_fork)
ss->cancel_fork(child);
+
+ cgroup_threadgroup_change_end(current);
}
/**
- * cgroup_post_fork - called on a new task after adding it to the task list
- * @child: the task in question
- *
- * Adds the task to the list running through its css_set if necessary and
- * call the subsystem fork() callbacks. Has to be after the task is
- * visible on the task list in case we race with the first call to
- * cgroup_task_iter_start() - to guarantee that the new task ends up on its
- * list.
+ * cgroup_post_fork - finalize cgroup setup for the child process
+ * @child: the child process
+ * @kargs: the arguments passed to create the child process
+ *
+ * Attach the child process to its css_set calling the subsystem fork()
+ * callbacks.
*/
void cgroup_post_fork(struct task_struct *child)
+ __releases(&cgroup_threadgroup_rwsem)
{
struct cgroup_subsys *ss;
struct css_set *cset;
@@ -5995,6 +6004,8 @@ void cgroup_post_fork(struct task_struct *child)
do_each_subsys_mask(ss, i, have_fork_callback) {
ss->fork(child);
} while_each_subsys_mask();
+
+ cgroup_threadgroup_change_end(current);
}
/**
diff --git a/kernel/fork.c b/kernel/fork.c
index 080809560072..ca5de25690c8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2165,7 +2165,6 @@ static __latent_entropy struct task_struct *copy_process(
INIT_LIST_HEAD(&p->thread_group);
p->task_works = NULL;
- cgroup_threadgroup_change_begin(current);
/*
* Ensure that the cgroup subsystem policies allow the new process to be
* forked. It should be noted the the new process's css_set can be changed
@@ -2174,7 +2173,7 @@ static __latent_entropy struct task_struct *copy_process(
*/
retval = cgroup_can_fork(p);
if (retval)
- goto bad_fork_cgroup_threadgroup_change_end;
+ goto bad_fork_put_pidfd;
/*
* From this point on we must avoid any synchronous user-space
@@ -2280,7 +2279,6 @@ static __latent_entropy struct task_struct *copy_process(
proc_fork_connector(p);
cgroup_post_fork(p);
- cgroup_threadgroup_change_end(current);
perf_event_fork(p);
trace_task_newtask(p, clone_flags);
@@ -2292,8 +2290,6 @@ static __latent_entropy struct task_struct *copy_process(
spin_unlock(¤t->sighand->siglock);
write_unlock_irq(&tasklist_lock);
cgroup_cancel_fork(p);
-bad_fork_cgroup_threadgroup_change_end:
- cgroup_threadgroup_change_end(current);
bad_fork_put_pidfd:
if (clone_flags & CLONE_PIDFD) {
fput(pidfile);
--
2.25.0
^ permalink raw reply related
* [PATCH v5 4/6] cgroup: add cgroup_may_write() helper
From: Christian Brauner @ 2020-01-21 15:48 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo
Cc: Oleg Nesterov, Christian Brauner, Johannes Weiner, Li Zefan,
cgroups
In-Reply-To: <20200121154844.411-1-christian.brauner@ubuntu.com>
Add a cgroup_may_write() helper which we can use in the
CLONE_INTO_CGROUP patch series to verify that we can write to the
destination cgroup.
Cc: Tejun Heo <tj@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: cgroups@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
patch not present
/* v2 */
patch not present
/* v3 */
patch not present
/* v4 */
Link: https://lore.kernel.org/r/20200117181219.14542-5-christian.brauner@ubuntu.com
patch introduced
/* v5 */
unchanged
---
kernel/cgroup/cgroup.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index ce2d5b8aa19f..636fe3d46d2d 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4649,13 +4649,28 @@ static int cgroup_procs_show(struct seq_file *s, void *v)
return 0;
}
+static int cgroup_may_write(const struct cgroup *cgrp, struct super_block *sb)
+{
+ int ret;
+ struct inode *inode;
+
+ lockdep_assert_held(&cgroup_mutex);
+
+ inode = kernfs_get_inode(sb, cgrp->procs_file.kn);
+ if (!inode)
+ return -ENOMEM;
+
+ ret = inode_permission(inode, MAY_WRITE);
+ iput(inode);
+ return ret;
+}
+
static int cgroup_procs_write_permission(struct cgroup *src_cgrp,
struct cgroup *dst_cgrp,
struct super_block *sb)
{
struct cgroup_namespace *ns = current->nsproxy->cgroup_ns;
struct cgroup *com_cgrp = src_cgrp;
- struct inode *inode;
int ret;
lockdep_assert_held(&cgroup_mutex);
@@ -4665,12 +4680,7 @@ static int cgroup_procs_write_permission(struct cgroup *src_cgrp,
com_cgrp = cgroup_parent(com_cgrp);
/* %current should be authorized to migrate to the common ancestor */
- inode = kernfs_get_inode(sb, com_cgrp->procs_file.kn);
- if (!inode)
- return -ENOMEM;
-
- ret = inode_permission(inode, MAY_WRITE);
- iput(inode);
+ ret = cgroup_may_write(com_cgrp, sb);
if (ret)
return ret;
--
2.25.0
^ permalink raw reply related
* [PATCH v5 5/6] clone3: allow spawning processes into cgroups
From: Christian Brauner @ 2020-01-21 15:48 UTC (permalink / raw)
To: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Tejun Heo
Cc: Oleg Nesterov, Christian Brauner, Ingo Molnar, Johannes Weiner,
Li Zefan, Peter Zijlstra, cgroups-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20200121154844.411-1-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
This adds support for creating a process in a different cgroup than its
parent. Callers can limit and account processes and threads right from
the moment they are spawned:
- A service manager can directly spawn new services into dedicated
cgroups.
- A process can be directly created in a frozen cgroup and will be
frozen as well.
- The initial accounting jitter experienced by process supervisors and
daemons is eliminated with this.
- Threaded applications or even thread implementations can choose to
create a specific cgroup layout where each thread is spawned
directly into a dedicated cgroup.
This feature is limited to the unified hierarchy. Callers need to pass
an directory file descriptor for the target cgroup. The caller can
choose to pass an O_PATH file descriptor. All usual migration
restrictions apply, i.e. there can be no processes in inner nodes. In
general, creating a process directly in a target cgroup adheres to all
migration restrictions.
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
/* v1 */
Link: https://lore.kernel.org/r/20191218173516.7875-3-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org
/* v2 */
Link: https://lore.kernel.org/r/20191223061504.28716-3-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org
- Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
- prevent deadlock from wrong locking order
- Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>:
- Rework locking. In the previous patch version we would have already
acquired the cgroup_threadgroup_rwsem before we grabbed cgroup mutex
we need to hold when CLONE_INTO_CGROUP is specified. This meant we
could deadlock with other codepaths that all require it to be done
the other way around. Fix this by first grabbing cgroup mutex when
CLONE_INTO_CGROUP is specified and then grabbing
cgroup_threadgroup_rwsem unconditionally after. This way we don't
require the cgroup mutex be held in codepaths that don't need it.
- Switch from mutex_lock() to mutex_lock_killable().
/* v3 */
Link: https://lore.kernel.org/r/20200117002143.15559-5-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org
- Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
- s/mutex_lock_killable()/mutex_lock()/ because it should only ever
be held for a short time:
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index a9fedcfeae4b..d68d3fb6af1d 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5927,11 +5927,8 @@ static int cgroup_css_set_fork(struct task_struct *parent,
struct super_block *sb;
struct file *f;
- if (kargs->flags & CLONE_INTO_CGROUP) {
- ret = mutex_lock_killable(&cgroup_mutex);
- if (ret)
- return ret;
- }
+ if (kargs->flags & CLONE_INTO_CGROUP)
+ mutex_lock(&cgroup_mutex);
cgroup_threadgroup_change_begin(parent);
- s/task_cgroup_from_root/cset->dfl_cgrp/:
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index d68d3fb6af1d..3ceef006d144 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5922,7 +5922,7 @@ static int cgroup_css_set_fork(struct task_struct *parent,
__acquires(&cgroup_mutex) __acquires(&cgroup_threadgroup_rwsem)
{
int ret;
- struct cgroup *dst_cgrp = NULL, *src_cgrp;
+ struct cgroup *dst_cgrp = NULL;
struct css_set *cset;
struct super_block *sb;
struct file *f;
@@ -5956,11 +5956,7 @@ static int cgroup_css_set_fork(struct task_struct *parent,
goto err;
}
- spin_lock_irq(&css_set_lock);
- src_cgrp = task_cgroup_from_root(parent, &cgrp_dfl_cgrp);
- spin_unlock_irq(&css_set_lock);
-
- ret = cgroup_attach_permissions(src_cgrp, dst_cgrp, sb,
+ ret = cgroup_attach_permissions(cset->dfl_cgrp, dst_cgrp, sb,
!!(kargs->flags & CLONE_THREAD));
if (ret)
goto err;
- pass struct css_set instead of struct kernel_clone_args into cgroup
fork subsystem callbacks:
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index cd848c6bac4a..058bb16d073f 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -630,9 +630,8 @@ struct cgroup_subsys {
void (*attach)(struct cgroup_taskset *tset);
void (*post_attach)(void);
int (*can_fork)(struct task_struct *parent, struct task_struct *child,
- struct kernel_clone_args *kargs);
- void (*cancel_fork)(struct task_struct *child,
- struct kernel_clone_args *kargs);
+ struct css_set *cset);
+ void (*cancel_fork)(struct task_struct *child, struct css_set *cset);
void (*fork)(struct task_struct *task);
void (*exit)(struct task_struct *task);
void (*release)(struct task_struct *task);
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 3ceef006d144..2ac1c37a3fcb 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6044,7 +6044,7 @@ int cgroup_can_fork(struct task_struct *parent, struct task_struct *child,
return ret;
do_each_subsys_mask(ss, i, have_canfork_callback) {
- ret = ss->can_fork(parent, child, kargs);
+ ret = ss->can_fork(parent, child, kargs->cset);
if (ret)
goto out_revert;
} while_each_subsys_mask();
@@ -6056,7 +6056,7 @@ int cgroup_can_fork(struct task_struct *parent, struct task_struct *child,
if (j >= i)
break;
if (ss->cancel_fork)
- ss->cancel_fork(child, kargs);
+ ss->cancel_fork(child, kargs->cset);
}
cgroup_css_set_put_fork(parent, kargs);
@@ -6082,7 +6082,7 @@ void cgroup_cancel_fork(struct task_struct *parent, struct task_struct *child,
for_each_subsys(ss, i)
if (ss->cancel_fork)
- ss->cancel_fork(child, kargs);
+ ss->cancel_fork(child, kargs->cset);
cgroup_css_set_put_fork(parent, kargs);
}
diff --git a/kernel/cgroup/pids.c b/kernel/cgroup/pids.c
index e5955bc1fb00..4e7c8819c8df 100644
--- a/kernel/cgroup/pids.c
+++ b/kernel/cgroup/pids.c
@@ -216,20 +216,16 @@ static void pids_cancel_attach(struct cgroup_taskset *tset)
* on cgroup_threadgroup_change_begin() held by the copy_process().
*/
static int pids_can_fork(struct task_struct *parent, struct task_struct *child,
- struct kernel_clone_args *args)
+ struct css_set *cset)
{
- struct css_set *new_cset = NULL;
struct cgroup_subsys_state *css;
struct pids_cgroup *pids;
int err;
- if (args)
- new_cset = args->cset;
-
- if (!new_cset)
- css = task_css_check(current, pids_cgrp_id, true);
+ if (cset)
+ css = cset->subsys[pids_cgrp_id];
else
- css = new_cset->subsys[pids_cgrp_id];
+ css = task_css_check(current, pids_cgrp_id, true);
pids = css_pids(css);
err = pids_try_charge(pids, 1);
if (err) {
@@ -244,20 +240,15 @@ static int pids_can_fork(struct task_struct *parent, struct task_struct *child,
return err;
}
-static void pids_cancel_fork(struct task_struct *task,
- struct kernel_clone_args *args)
+static void pids_cancel_fork(struct task_struct *task, struct css_set *cset)
{
- struct css_set *new_cset = NULL;
struct cgroup_subsys_state *css;
struct pids_cgroup *pids;
- if (args)
- new_cset = args->cset;
-
- if (!new_cset)
- css = task_css_check(current, pids_cgrp_id, true);
+ if (cset)
+ css = cset->subsys[pids_cgrp_id];
else
- css = new_cset->subsys[pids_cgrp_id];
+ css = task_css_check(current, pids_cgrp_id, true);
pids = css_pids(css);
pids_uncharge(pids, 1);
}
- Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>:
- update comment for cgroup_fork()
- if CLONE_NEWCGROUP and CLONE_INTO_CGROUP is requested, set the
root_cset of the new cgroup namespace to the child's cset
/* v4 */
Link: https://lore.kernel.org/r/20200117181219.14542-6-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org
- Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
- verify that we can write to the target cgroup since we're not going through
the vfs layer which would do it for us
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 61d1a6cd0059..6b38b2545667 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5966,6 +5966,15 @@ static int cgroup_css_set_fork(struct task_struct *parent,
goto err;
}
+ /*
+ * Verify that we can the target cgroup is writable for us. This is
+ * usally done by the vfs layer but since we're not going through the
+ * vfs layer here we need to do it.
+ */
+ ret = cgroup_may_write(dst_cgrp, sb);
+ if (ret)
+ goto err;
+
ret = cgroup_attach_permissions(cset->dfl_cgrp, dst_cgrp, sb,
!!(kargs->flags & CLONE_THREAD));
if (ret)
/* v5 */
- Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
- remove struct task_struct *parent argument from clone helpers in favor of
using current directly
- remove cgroup_same_domain_helper()
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index f4379401327a..4d36255ef25f 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4696,12 +4696,6 @@ static int cgroup_procs_write_permission(struct cgroup *src_cgrp,
return 0;
}
-static inline bool cgroup_same_domain(const struct cgroup *src_cgrp,
- const struct cgroup *dst_cgrp)
-{
- return src_cgrp->dom_cgrp == dst_cgrp->dom_cgrp;
-}
-
static int cgroup_attach_permissions(struct cgroup *src_cgrp,
struct cgroup *dst_cgrp,
struct super_block *sb, bool thread)
@@ -4716,8 +4710,7 @@ static int cgroup_attach_permissions(struct cgroup *src_cgrp,
if (ret)
return ret;
- if (thread &&
- !cgroup_same_domain(src_cgrp->dom_cgrp, dst_cgrp->dom_cgrp))
+ if (thread && (src_cgrp->dom_cgrp != dst_cgrp->dom_cgrp))
ret = -EOPNOTSUPP;
return ret;
- put kargs->cset on failure
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 4d36255ef25f..482055d1e64a 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5994,6 +5994,8 @@ static int cgroup_css_set_fork(struct kernel_clone_args *kargs)
if (dst_cgrp)
cgroup_put(dst_cgrp);
put_css_set(cset);
+ if (kargs->cset)
+ put_css_set(kargs->cset);
return ret;
}
---
include/linux/cgroup-defs.h | 6 +-
include/linux/cgroup.h | 20 ++--
include/linux/sched/task.h | 4 +
include/uapi/linux/sched.h | 5 +
kernel/cgroup/cgroup.c | 189 +++++++++++++++++++++++++++++++-----
kernel/cgroup/pids.c | 15 ++-
kernel/fork.c | 13 ++-
7 files changed, 212 insertions(+), 40 deletions(-)
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 63097cb243cb..89d627abcbd6 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -33,6 +33,7 @@ struct kernfs_ops;
struct kernfs_open_file;
struct seq_file;
struct poll_table_struct;
+struct kernel_clone_args;
#define MAX_CGROUP_TYPE_NAMELEN 32
#define MAX_CGROUP_ROOT_NAMELEN 64
@@ -628,8 +629,9 @@ struct cgroup_subsys {
void (*cancel_attach)(struct cgroup_taskset *tset);
void (*attach)(struct cgroup_taskset *tset);
void (*post_attach)(void);
- int (*can_fork)(struct task_struct *task);
- void (*cancel_fork)(struct task_struct *task);
+ int (*can_fork)(struct task_struct *task,
+ struct css_set *cset);
+ void (*cancel_fork)(struct task_struct *task, struct css_set *cset);
void (*fork)(struct task_struct *task);
void (*exit)(struct task_struct *task);
void (*release)(struct task_struct *task);
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index d7ddebd0cdec..fbbaeac9fe29 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -27,6 +27,8 @@
#include <linux/cgroup-defs.h>
+struct kernel_clone_args;
+
#ifdef CONFIG_CGROUPS
/*
@@ -121,9 +123,12 @@ int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
struct pid *pid, struct task_struct *tsk);
void cgroup_fork(struct task_struct *p);
-extern int cgroup_can_fork(struct task_struct *p);
-extern void cgroup_cancel_fork(struct task_struct *p);
-extern void cgroup_post_fork(struct task_struct *p);
+extern int cgroup_can_fork(struct task_struct *p,
+ struct kernel_clone_args *kargs);
+extern void cgroup_cancel_fork(struct task_struct *p,
+ struct kernel_clone_args *kargs);
+extern void cgroup_post_fork(struct task_struct *p,
+ struct kernel_clone_args *kargs);
void cgroup_exit(struct task_struct *p);
void cgroup_release(struct task_struct *p);
void cgroup_free(struct task_struct *p);
@@ -707,9 +712,12 @@ static inline int cgroupstats_build(struct cgroupstats *stats,
struct dentry *dentry) { return -EINVAL; }
static inline void cgroup_fork(struct task_struct *p) {}
-static inline int cgroup_can_fork(struct task_struct *p) { return 0; }
-static inline void cgroup_cancel_fork(struct task_struct *p) {}
-static inline void cgroup_post_fork(struct task_struct *p) {}
+static inline int cgroup_can_fork(struct task_struct *p,
+ struct kernel_clone_args *kargs) { return 0; }
+static inline void cgroup_cancel_fork(struct task_struct *p,
+ struct kernel_clone_args *kargs) {}
+static inline void cgroup_post_fork(struct task_struct *p,
+ struct kernel_clone_args *kargs) {}
static inline void cgroup_exit(struct task_struct *p) {}
static inline void cgroup_release(struct task_struct *p) {}
static inline void cgroup_free(struct task_struct *p) {}
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index f1879884238e..38359071236a 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -13,6 +13,7 @@
struct task_struct;
struct rusage;
union thread_union;
+struct css_set;
/* All the bits taken by the old clone syscall. */
#define CLONE_LEGACY_FLAGS 0xffffffffULL
@@ -29,6 +30,9 @@ struct kernel_clone_args {
pid_t *set_tid;
/* Number of elements in *set_tid */
size_t set_tid_size;
+ int cgroup;
+ struct cgroup *cgrp;
+ struct css_set *cset;
};
/*
diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 4a0217832464..08620c220f30 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -35,6 +35,7 @@
/* Flags for the clone3() syscall. */
#define CLONE_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and reset to SIG_DFL. */
+#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given the right permissions. */
#ifndef __ASSEMBLY__
/**
@@ -75,6 +76,8 @@
* @set_tid_size: This defines the size of the array referenced
* in @set_tid. This cannot be larger than the
* kernel's limit of nested PID namespaces.
+ * @cgroup: If CLONE_INTO_CGROUP is specified set this to
+ * a file descriptor for the cgroup.
*
* The structure is versioned by size and thus extensible.
* New struct members must go at the end of the struct and
@@ -91,11 +94,13 @@ struct clone_args {
__aligned_u64 tls;
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
+ __aligned_u64 cgroup;
};
#endif
#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
+#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
/*
* Scheduling policies
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 636fe3d46d2d..6a7f53fd8374 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5876,8 +5876,7 @@ int proc_cgroup_show(struct seq_file *m, struct pid_namespace *ns,
* @child: pointer to task_struct of forking parent process.
*
* A task is associated with the init_css_set until cgroup_post_fork()
- * attaches it to the parent's css_set. Empty cg_list indicates that
- * @child isn't holding reference to its css_set.
+ * attaches it to the target css_set.
*/
void cgroup_fork(struct task_struct *child)
{
@@ -5903,25 +5902,156 @@ static struct cgroup *cgroup_get_from_file(struct file *f)
return cgrp;
}
+/**
+ * cgroup_css_set_fork - find or create a css_set for a child process
+ * @kargs: the arguments passed to create the child process
+ *
+ * This functions finds or creates a new css_set which the child
+ * process will be attached to in cgroup_post_fork(). By default,
+ * the child process will be given the same css_set as its parent.
+ *
+ * If CLONE_INTO_CGROUP is specified this function will try to find an
+ * existing css_set which includes the requested cgroup and if not create
+ * a new css_set that the child will be attached to later. If this function
+ * succeeds it will hold cgroup_threadgroup_rwsem on return. If
+ * CLONE_INTO_CGROUP is requested this function will grab cgroup mutex
+ * before grabbing cgroup_threadgroup_rwsem and will hold a reference
+ * to the target cgroup.
+ */
+static int cgroup_css_set_fork(struct kernel_clone_args *kargs)
+ __acquires(&cgroup_mutex) __acquires(&cgroup_threadgroup_rwsem)
+{
+ int ret;
+ struct cgroup *dst_cgrp = NULL;
+ struct css_set *cset;
+ struct super_block *sb;
+ struct file *f;
+
+ if (kargs->flags & CLONE_INTO_CGROUP)
+ mutex_lock(&cgroup_mutex);
+
+ cgroup_threadgroup_change_begin(current);
+
+ spin_lock_irq(&css_set_lock);
+ cset = task_css_set(current);
+ get_css_set(cset);
+ spin_unlock_irq(&css_set_lock);
+
+ if (!(kargs->flags & CLONE_INTO_CGROUP)) {
+ kargs->cset = cset;
+ return 0;
+ }
+
+ f = fget_raw(kargs->cgroup);
+ if (!f) {
+ ret = -EBADF;
+ goto err;
+ }
+ sb = f->f_path.dentry->d_sb;
+
+ dst_cgrp = cgroup_get_from_file(f);
+ if (IS_ERR(dst_cgrp)) {
+ ret = PTR_ERR(dst_cgrp);
+ dst_cgrp = NULL;
+ goto err;
+ }
+
+ /*
+ * Verify that we the target cgroup is writable for us. This is
+ * usually done by the vfs layer but since we're not going through
+ * the vfs layer here we need to do it "manually".
+ */
+ ret = cgroup_may_write(dst_cgrp, sb);
+ if (ret)
+ goto err;
+
+ ret = cgroup_attach_permissions(cset->dfl_cgrp, dst_cgrp, sb,
+ !!(kargs->flags & CLONE_THREAD));
+ if (ret)
+ goto err;
+
+ kargs->cset = find_css_set(cset, dst_cgrp);
+ if (!kargs->cset) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ if (cgroup_is_dead(dst_cgrp)) {
+ ret = -ENODEV;
+ goto err;
+ }
+
+ put_css_set(cset);
+ fput(f);
+ kargs->cgrp = dst_cgrp;
+ return ret;
+
+err:
+ cgroup_threadgroup_change_end(current);
+ mutex_unlock(&cgroup_mutex);
+ if (f)
+ fput(f);
+ if (dst_cgrp)
+ cgroup_put(dst_cgrp);
+ put_css_set(cset);
+ if (kargs->cset)
+ put_css_set(kargs->cset);
+ return ret;
+}
+
+/**
+ * cgroup_css_set_put_fork - drop references we took during fork
+ * @kargs: the arguments passed to create the child process
+ *
+ * Drop references to the prepared css_set and target cgroup if
+ * CLONE_INTO_CGROUP was requested. This function can only be
+ * called before fork()'s point of no return.
+ */
+static void cgroup_css_set_put_fork(struct kernel_clone_args *kargs)
+ __releases(&cgroup_threadgroup_rwsem) __releases(&cgroup_mutex)
+{
+ cgroup_threadgroup_change_end(current);
+
+ if (kargs->flags & CLONE_INTO_CGROUP) {
+ struct cgroup *cgrp = kargs->cgrp;
+ struct css_set *cset = kargs->cset;
+
+ mutex_unlock(&cgroup_mutex);
+
+ if (cset) {
+ put_css_set(cset);
+ kargs->cset = NULL;
+ }
+
+ if (cgrp) {
+ cgroup_put(cgrp);
+ kargs->cgrp = NULL;
+ }
+ }
+}
+
/**
* cgroup_can_fork - called on a new task before the process is exposed
* @child: the child process
* @kargs: the arguments passed to create the child process
*
+ * This prepares a new css_set for the child process which the child will
+ * be attached to in cgroup_post_fork().
* This calls the subsystem can_fork() callbacks. If the cgroup_can_fork()
* callback returns an error, the fork aborts with that error code. This
* allows for a cgroup subsystem to conditionally allow or deny new forks.
*/
-int cgroup_can_fork(struct task_struct *child)
- __acquires(&cgroup_threadgroup_rwsem) __releases(&cgroup_threadgroup_rwsem)
+int cgroup_can_fork(struct task_struct *child, struct kernel_clone_args *kargs)
{
struct cgroup_subsys *ss;
int i, j, ret;
- cgroup_threadgroup_change_begin(current);
+ ret = cgroup_css_set_fork(kargs);
+ if (ret)
+ return ret;
do_each_subsys_mask(ss, i, have_canfork_callback) {
- ret = ss->can_fork(child);
+ ret = ss->can_fork(child, kargs->cset);
if (ret)
goto out_revert;
} while_each_subsys_mask();
@@ -5933,33 +6063,34 @@ int cgroup_can_fork(struct task_struct *child)
if (j >= i)
break;
if (ss->cancel_fork)
- ss->cancel_fork(child);
+ ss->cancel_fork(child, kargs->cset);
}
- cgroup_threadgroup_change_end(current);
+ cgroup_css_set_put_fork(kargs);
return ret;
}
/**
- * cgroup_cancel_fork - called if a fork failed after cgroup_can_fork()
- * @child: the child process
- * @kargs: the arguments passed to create the child process
- *
- * This calls the cancel_fork() callbacks if a fork failed *after*
- * cgroup_can_fork() succeded.
- */
-void cgroup_cancel_fork(struct task_struct *child)
- __releases(&cgroup_threadgroup_rwsem)
+ * cgroup_cancel_fork - called if a fork failed after cgroup_can_fork()
+ * @child: the child process
+ * @kargs: the arguments passed to create the child process
+ *
+ * This calls the cancel_fork() callbacks if a fork failed *after*
+ * cgroup_can_fork() succeded and cleans up references we took to
+ * prepare a new css_set for the child process in cgroup_can_fork().
+ */
+void cgroup_cancel_fork(struct task_struct *child,
+ struct kernel_clone_args *kargs)
{
struct cgroup_subsys *ss;
int i;
for_each_subsys(ss, i)
if (ss->cancel_fork)
- ss->cancel_fork(child);
+ ss->cancel_fork(child, kargs->cset);
- cgroup_threadgroup_change_end(current);
+ cgroup_css_set_put_fork(kargs);
}
/**
@@ -5970,18 +6101,17 @@ void cgroup_cancel_fork(struct task_struct *child)
* Attach the child process to its css_set calling the subsystem fork()
* callbacks.
*/
-void cgroup_post_fork(struct task_struct *child)
- __releases(&cgroup_threadgroup_rwsem)
+void cgroup_post_fork(struct task_struct *child,
+ struct kernel_clone_args *kargs)
+ __releases(&cgroup_threadgroup_rwsem) __releases(&cgroup_mutex)
{
struct cgroup_subsys *ss;
- struct css_set *cset;
+ struct css_set *cset = kargs->cset;
int i;
spin_lock_irq(&css_set_lock);
WARN_ON_ONCE(!list_empty(&child->cg_list));
- cset = task_css_set(current); /* current is @child's parent */
- get_css_set(cset);
cset->nr_tasks++;
css_set_move_task(child, NULL, cset, false);
@@ -6016,6 +6146,17 @@ void cgroup_post_fork(struct task_struct *child)
} while_each_subsys_mask();
cgroup_threadgroup_change_end(current);
+
+ if (kargs->flags & CLONE_INTO_CGROUP) {
+ mutex_unlock(&cgroup_mutex);
+
+ cgroup_put(kargs->cgrp);
+ kargs->cgrp = NULL;
+ }
+
+ /* Make the new cset the root_cset of the new cgroup namespace. */
+ if (kargs->flags & CLONE_NEWCGROUP)
+ child->nsproxy->cgroup_ns->root_cset = cset;
}
/**
diff --git a/kernel/cgroup/pids.c b/kernel/cgroup/pids.c
index 138059eb730d..511af87f685e 100644
--- a/kernel/cgroup/pids.c
+++ b/kernel/cgroup/pids.c
@@ -33,6 +33,7 @@
#include <linux/atomic.h>
#include <linux/cgroup.h>
#include <linux/slab.h>
+#include <linux/sched/task.h>
#define PIDS_MAX (PID_MAX_LIMIT + 1ULL)
#define PIDS_MAX_STR "max"
@@ -214,13 +215,16 @@ static void pids_cancel_attach(struct cgroup_taskset *tset)
* task_css_check(true) in pids_can_fork() and pids_cancel_fork() relies
* on cgroup_threadgroup_change_begin() held by the copy_process().
*/
-static int pids_can_fork(struct task_struct *task)
+static int pids_can_fork(struct task_struct *task, struct css_set *cset)
{
struct cgroup_subsys_state *css;
struct pids_cgroup *pids;
int err;
- css = task_css_check(current, pids_cgrp_id, true);
+ if (cset)
+ css = cset->subsys[pids_cgrp_id];
+ else
+ css = task_css_check(current, pids_cgrp_id, true);
pids = css_pids(css);
err = pids_try_charge(pids, 1);
if (err) {
@@ -235,12 +239,15 @@ static int pids_can_fork(struct task_struct *task)
return err;
}
-static void pids_cancel_fork(struct task_struct *task)
+static void pids_cancel_fork(struct task_struct *task, struct css_set *cset)
{
struct cgroup_subsys_state *css;
struct pids_cgroup *pids;
- css = task_css_check(current, pids_cgrp_id, true);
+ if (cset)
+ css = cset->subsys[pids_cgrp_id];
+ else
+ css = task_css_check(current, pids_cgrp_id, true);
pids = css_pids(css);
pids_uncharge(pids, 1);
}
diff --git a/kernel/fork.c b/kernel/fork.c
index ca5de25690c8..2853e258fe1f 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2171,7 +2171,7 @@ static __latent_entropy struct task_struct *copy_process(
* between here and cgroup_post_fork() if an organisation operation is in
* progress.
*/
- retval = cgroup_can_fork(p);
+ retval = cgroup_can_fork(p, args);
if (retval)
goto bad_fork_put_pidfd;
@@ -2278,7 +2278,7 @@ static __latent_entropy struct task_struct *copy_process(
write_unlock_irq(&tasklist_lock);
proc_fork_connector(p);
- cgroup_post_fork(p);
+ cgroup_post_fork(p, args);
perf_event_fork(p);
trace_task_newtask(p, clone_flags);
@@ -2289,7 +2289,7 @@ static __latent_entropy struct task_struct *copy_process(
bad_fork_cancel_cgroup:
spin_unlock(¤t->sighand->siglock);
write_unlock_irq(&tasklist_lock);
- cgroup_cancel_fork(p);
+ cgroup_cancel_fork(p, args);
bad_fork_put_pidfd:
if (clone_flags & CLONE_PIDFD) {
fput(pidfile);
@@ -2618,6 +2618,9 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
!valid_signal(args.exit_signal)))
return -EINVAL;
+ if ((args.flags & CLONE_INTO_CGROUP) && args.cgroup < 0)
+ return -EINVAL;
+
*kargs = (struct kernel_clone_args){
.flags = args.flags,
.pidfd = u64_to_user_ptr(args.pidfd),
@@ -2628,6 +2631,7 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
.stack_size = args.stack_size,
.tls = args.tls,
.set_tid_size = args.set_tid_size,
+ .cgroup = args.cgroup,
};
if (args.set_tid &&
@@ -2671,7 +2675,8 @@ static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
static bool clone3_args_valid(struct kernel_clone_args *kargs)
{
/* Verify that no unknown flags are passed along. */
- if (kargs->flags & ~(CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND))
+ if (kargs->flags &
+ ~(CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND | CLONE_INTO_CGROUP))
return false;
/*
--
2.25.0
^ permalink raw reply related
* [PATCH v5 6/6] selftests/cgroup: add tests for cloning into cgroups
From: Christian Brauner @ 2020-01-21 15:48 UTC (permalink / raw)
To: linux-api, linux-kernel, Tejun Heo
Cc: Oleg Nesterov, Christian Brauner, Shuah Khan, cgroups,
linux-kselftest, Roman Gushchin
In-Reply-To: <20200121154844.411-1-christian.brauner@ubuntu.com>
Expand the cgroup test-suite to include tests for CLONE_INTO_CGROUP.
This adds the following tests:
- CLONE_INTO_CGROUP manages to clone a process directly into a correctly
delegated cgroup
- CLONE_INTO_CGROUP fails to clone a process into a cgroup that has been
removed after we've opened an fd to it
- CLONE_INTO_CGROUP fails to clone a process into an invalid domain
cgroup
- CLONE_INTO_CGROUP adheres to the no internal process constraint
- CLONE_INTO_CGROUP works with the freezer feature
Cc: Tejun Heo <tj@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: cgroups@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
Acked-by: Roman Gushchin <guro@fb.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lore.kernel.org/r/20191218173516.7875-4-christian.brauner@ubuntu.com
/* v2 */
Link: https://lore.kernel.org/r/20191223061504.28716-4-christian.brauner@ubuntu.com
unchanged
/* v3 */
Link: https://lore.kernel.org/r/20200117002143.15559-6-christian.brauner@ubuntu.com
unchanged
/* v4 */
Link: https://lore.kernel.org/r/20200117181219.14542-7-christian.brauner@ubuntu.com
unchanged
/* v5 */
unchanged
- Christian Brauner <christian.brauner@ubuntu.com>:
- add Acked-by: Roman Gushchin <guro@fb.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
tools/testing/selftests/cgroup/Makefile | 6 +-
tools/testing/selftests/cgroup/cgroup_util.c | 126 ++++++++++++++++++
tools/testing/selftests/cgroup/cgroup_util.h | 4 +
tools/testing/selftests/cgroup/test_core.c | 64 +++++++++
.../selftests/clone3/clone3_selftests.h | 19 ++-
5 files changed, 214 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/cgroup/Makefile b/tools/testing/selftests/cgroup/Makefile
index 66aafe1f5746..967f268fde74 100644
--- a/tools/testing/selftests/cgroup/Makefile
+++ b/tools/testing/selftests/cgroup/Makefile
@@ -11,6 +11,6 @@ TEST_GEN_PROGS += test_freezer
include ../lib.mk
-$(OUTPUT)/test_memcontrol: cgroup_util.c
-$(OUTPUT)/test_core: cgroup_util.c
-$(OUTPUT)/test_freezer: cgroup_util.c
+$(OUTPUT)/test_memcontrol: cgroup_util.c ../clone3/clone3_selftests.h
+$(OUTPUT)/test_core: cgroup_util.c ../clone3/clone3_selftests.h
+$(OUTPUT)/test_freezer: cgroup_util.c ../clone3/clone3_selftests.h
diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c
index 8f7131dcf1ff..8a637ca7d73a 100644
--- a/tools/testing/selftests/cgroup/cgroup_util.c
+++ b/tools/testing/selftests/cgroup/cgroup_util.c
@@ -15,6 +15,7 @@
#include <unistd.h>
#include "cgroup_util.h"
+#include "../clone3/clone3_selftests.h"
static ssize_t read_text(const char *path, char *buf, size_t max_len)
{
@@ -331,12 +332,112 @@ int cg_run(const char *cgroup,
}
}
+pid_t clone_into_cgroup(int cgroup_fd)
+{
+#ifdef CLONE_ARGS_SIZE_VER2
+ pid_t pid;
+
+ struct clone_args args = {
+ .flags = CLONE_INTO_CGROUP,
+ .exit_signal = SIGCHLD,
+ .cgroup = cgroup_fd,
+ };
+
+ pid = sys_clone3(&args, sizeof(struct clone_args));
+ /*
+ * Verify that this is a genuine test failure:
+ * ENOSYS -> clone3() not available
+ * E2BIG -> CLONE_INTO_CGROUP not available
+ */
+ if (pid < 0 && (errno == ENOSYS || errno == E2BIG))
+ goto pretend_enosys;
+
+ return pid;
+
+pretend_enosys:
+#endif
+ errno = ENOSYS;
+ return -ENOSYS;
+}
+
+int clone_reap(pid_t pid, int options)
+{
+ int ret;
+ siginfo_t info = {
+ .si_signo = 0,
+ };
+
+again:
+ ret = waitid(P_PID, pid, &info, options | __WALL | __WNOTHREAD);
+ if (ret < 0) {
+ if (errno == EINTR)
+ goto again;
+ return -1;
+ }
+
+ if (options & WEXITED) {
+ if (WIFEXITED(info.si_status))
+ return WEXITSTATUS(info.si_status);
+ }
+
+ if (options & WSTOPPED) {
+ if (WIFSTOPPED(info.si_status))
+ return WSTOPSIG(info.si_status);
+ }
+
+ if (options & WCONTINUED) {
+ if (WIFCONTINUED(info.si_status))
+ return 0;
+ }
+
+ return -1;
+}
+
+int dirfd_open_opath(const char *dir)
+{
+ return open(dir, O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW | O_PATH);
+}
+
+#define close_prot_errno(fd) \
+ if (fd >= 0) { \
+ int _e_ = errno; \
+ close(fd); \
+ errno = _e_; \
+ }
+
+static int clone_into_cgroup_run_nowait(const char *cgroup,
+ int (*fn)(const char *cgroup, void *arg),
+ void *arg)
+{
+ int cgroup_fd;
+ pid_t pid;
+
+ cgroup_fd = dirfd_open_opath(cgroup);
+ if (cgroup_fd < 0)
+ return -1;
+
+ pid = clone_into_cgroup(cgroup_fd);
+ close_prot_errno(cgroup_fd);
+ if (pid == 0)
+ exit(fn(cgroup, arg));
+
+ return pid;
+}
+
int cg_run_nowait(const char *cgroup,
int (*fn)(const char *cgroup, void *arg),
void *arg)
{
int pid;
+ pid = clone_into_cgroup_run_nowait(cgroup, fn, arg);
+ if (pid > 0)
+ return pid;
+
+ /* Genuine test failure. */
+ if (pid < 0 && errno != ENOSYS)
+ return -1;
+
pid = fork();
if (pid == 0) {
char buf[64];
@@ -450,3 +551,28 @@ int proc_read_strstr(int pid, bool thread, const char *item, const char *needle)
return strstr(buf, needle) ? 0 : -1;
}
+
+int clone_into_cgroup_run_wait(const char *cgroup)
+{
+ int cgroup_fd;
+ pid_t pid;
+
+ cgroup_fd = dirfd_open_opath(cgroup);
+ if (cgroup_fd < 0)
+ return -1;
+
+ pid = clone_into_cgroup(cgroup_fd);
+ close_prot_errno(cgroup_fd);
+ if (pid < 0)
+ return -1;
+
+ if (pid == 0)
+ exit(EXIT_SUCCESS);
+
+ /*
+ * We don't care whether this fails. We only care whether the initial
+ * clone succeeded.
+ */
+ (void)clone_reap(pid, WEXITED);
+ return 0;
+}
diff --git a/tools/testing/selftests/cgroup/cgroup_util.h b/tools/testing/selftests/cgroup/cgroup_util.h
index 49c54fbdb229..5a1305dd1f0b 100644
--- a/tools/testing/selftests/cgroup/cgroup_util.h
+++ b/tools/testing/selftests/cgroup/cgroup_util.h
@@ -50,3 +50,7 @@ extern int cg_wait_for_proc_count(const char *cgroup, int count);
extern int cg_killall(const char *cgroup);
extern ssize_t proc_read_text(int pid, bool thread, const char *item, char *buf, size_t size);
extern int proc_read_strstr(int pid, bool thread, const char *item, const char *needle);
+extern pid_t clone_into_cgroup(int cgroup_fd);
+extern int clone_reap(pid_t pid, int options);
+extern int clone_into_cgroup_run_wait(const char *cgroup);
+extern int dirfd_open_opath(const char *dir);
diff --git a/tools/testing/selftests/cgroup/test_core.c b/tools/testing/selftests/cgroup/test_core.c
index c5ca669feb2b..96e016ccafe0 100644
--- a/tools/testing/selftests/cgroup/test_core.c
+++ b/tools/testing/selftests/cgroup/test_core.c
@@ -25,8 +25,11 @@
static int test_cgcore_populated(const char *root)
{
int ret = KSFT_FAIL;
+ int err;
char *cg_test_a = NULL, *cg_test_b = NULL;
char *cg_test_c = NULL, *cg_test_d = NULL;
+ int cgroup_fd = -EBADF;
+ pid_t pid;
cg_test_a = cg_name(root, "cg_test_a");
cg_test_b = cg_name(root, "cg_test_a/cg_test_b");
@@ -78,6 +81,52 @@ static int test_cgcore_populated(const char *root)
if (cg_read_strcmp(cg_test_d, "cgroup.events", "populated 0\n"))
goto cleanup;
+ /* Test that we can directly clone into a new cgroup. */
+ cgroup_fd = dirfd_open_opath(cg_test_d);
+ if (cgroup_fd < 0)
+ goto cleanup;
+
+ pid = clone_into_cgroup(cgroup_fd);
+ if (pid < 0) {
+ if (errno == ENOSYS)
+ goto cleanup_pass;
+ goto cleanup;
+ }
+
+ if (pid == 0) {
+ if (raise(SIGSTOP))
+ exit(EXIT_FAILURE);
+ exit(EXIT_SUCCESS);
+ }
+
+ err = cg_read_strcmp(cg_test_d, "cgroup.events", "populated 1\n");
+
+ (void)clone_reap(pid, WSTOPPED);
+ (void)kill(pid, SIGCONT);
+ (void)clone_reap(pid, WEXITED);
+
+ if (err)
+ goto cleanup;
+
+ if (cg_read_strcmp(cg_test_d, "cgroup.events", "populated 0\n"))
+ goto cleanup;
+
+ /* Remove cgroup. */
+ if (cg_test_d) {
+ cg_destroy(cg_test_d);
+ free(cg_test_d);
+ cg_test_d = NULL;
+ }
+
+ pid = clone_into_cgroup(cgroup_fd);
+ if (pid < 0)
+ goto cleanup_pass;
+ if (pid == 0)
+ exit(EXIT_SUCCESS);
+ (void)clone_reap(pid, WEXITED);
+ goto cleanup;
+
+cleanup_pass:
ret = KSFT_PASS;
cleanup:
@@ -93,6 +142,8 @@ static int test_cgcore_populated(const char *root)
free(cg_test_c);
free(cg_test_b);
free(cg_test_a);
+ if (cgroup_fd >= 0)
+ close(cgroup_fd);
return ret;
}
@@ -136,6 +187,16 @@ static int test_cgcore_invalid_domain(const char *root)
if (errno != EOPNOTSUPP)
goto cleanup;
+ if (!clone_into_cgroup_run_wait(child))
+ goto cleanup;
+
+ if (errno == ENOSYS)
+ goto cleanup_pass;
+
+ if (errno != EOPNOTSUPP)
+ goto cleanup;
+
+cleanup_pass:
ret = KSFT_PASS;
cleanup:
@@ -345,6 +406,9 @@ static int test_cgcore_internal_process_constraint(const char *root)
if (!cg_enter_current(parent))
goto cleanup;
+ if (!clone_into_cgroup_run_wait(parent))
+ goto cleanup;
+
ret = KSFT_PASS;
cleanup:
diff --git a/tools/testing/selftests/clone3/clone3_selftests.h b/tools/testing/selftests/clone3/clone3_selftests.h
index a3f2c8ad8bcc..91c1a78ddb39 100644
--- a/tools/testing/selftests/clone3/clone3_selftests.h
+++ b/tools/testing/selftests/clone3/clone3_selftests.h
@@ -5,12 +5,24 @@
#define _GNU_SOURCE
#include <sched.h>
+#include <linux/sched.h>
+#include <linux/types.h>
#include <stdint.h>
#include <syscall.h>
-#include <linux/types.h>
+#include <sys/wait.h>
+
+#include "../kselftest.h"
#define ptr_to_u64(ptr) ((__u64)((uintptr_t)(ptr)))
+#ifndef CLONE_INTO_CGROUP
+#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given the right permissions. */
+#endif
+
+#ifndef CLONE_ARGS_SIZE_VER0
+#define CLONE_ARGS_SIZE_VER0 64
+#endif
+
#ifndef __NR_clone3
#define __NR_clone3 -1
struct clone_args {
@@ -22,10 +34,13 @@ struct clone_args {
__aligned_u64 stack;
__aligned_u64 stack_size;
__aligned_u64 tls;
+#define CLONE_ARGS_SIZE_VER1 80
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
+#define CLONE_ARGS_SIZE_VER2 88
+ __aligned_u64 cgroup;
};
-#endif
+#endif /* __NR_clone3 */
static pid_t sys_clone3(struct clone_args *args, size_t size)
{
--
2.25.0
^ permalink raw reply related
* [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Mathieu Desnoyers @ 2020-01-21 16:03 UTC (permalink / raw)
To: Peter Zijlstra, Thomas Gleixner
Cc: linux-kernel, Mathieu Desnoyers, Joel Fernandes, Ingo Molnar,
Catalin Marinas, Dave Watson, Will Deacon, Shuah Khan, Andi Kleen,
linux-kselftest, H . Peter Anvin, Chris Lameter, Russell King,
Michael Kerrisk, Paul E . McKenney, Paul Turner, Boqun Feng,
Josh Triplett, Steven Rostedt, Ben Maurer, linux-api
There is an important use-case which is not possible with the
"rseq" (Restartable Sequences) system call, which was left as
future work.
That use-case is to modify user-space per-cpu data structures
belonging to specific CPUs which may be brought offline and
online again by CPU hotplug. This can be used by memory
allocators to migrate free memory pools when CPUs are brought
offline, or by ring buffer consumers to target specific per-CPU
buffers, even when CPUs are brought offline.
A few rather complex prior attempts were made to solve this.
Those were based on in-kernel interpreters (cpu_opv, do_on_cpu).
That complexity was generally frowned upon, even by their author.
This patch fulfills this use-case in a refreshingly simple way:
it introduces a "pin_on_cpu" system call, which allows user-space
threads to pin themselves on a specific CPU (which needs to be
present in the thread's allowed cpu mask), and then clear this
pinned state.
"But this can already be done with sched_setaffinity", some
would rightfully reply. However, there is a significant twist
in the way pin_on_cpu deals with CPU hotplug compared to the
allowed cpu mask.
When all CPUs part of the thread's allowed cpu mask are offlined,
this mask is effectively reset to include all CPUs. This behavior
is completely incompatible with modifying per-cpu data structures:
the updates then become racy between concurrent CPUs trying to
update the given per-cpu data.
Conversely, all threads pinned on a given CPU with pin_on_cpu are
guaranteed to be scheduled on the same runqueue when that CPU is
offline. If that CPU is brought back online, the CPU hotplug
scheduler hooks are responsible for migrating back the tasks to
their pinned CPU.
For instance, this allows implementing this userspace library API
for incrementing a per-cpu counter for a specific cpu number
received as parameter:
static inline __attribute__((always_inline))
int percpu_addv(intptr_t *v, intptr_t count, int cpu)
{
int ret;
ret = rseq_addv(v, count, cpu);
check:
if (rseq_unlikely(ret)) {
pin_on_cpu_set(cpu);
ret = rseq_addv(v, count, percpu_current_cpu());
pin_on_cpu_clear();
goto check;
}
return 0;
}
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Joel Fernandes <joelaf@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Watson <davejwatson@fb.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: linux-kselftest@vger.kernel.org
Cc: "H . Peter Anvin" <hpa@zytor.com>
Cc: Chris Lameter <cl@linux.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: "Paul E . McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Paul Turner <pjt@google.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Maurer <bmaurer@fb.com>
Cc: linux-api@vger.kernel.org
Cc: Andy Lutomirski <luto@amacapital.net>
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
fs/exec.c | 1 +
include/linux/sched.h | 1 +
include/linux/syscalls.h | 1 +
include/uapi/asm-generic/unistd.h | 5 +-
include/uapi/linux/sched.h | 6 +
init/init_task.c | 1 +
kernel/sched/core.c | 321 +++++++++++++++++++++++--
kernel/sched/deadline.c | 54 +++--
kernel/sched/fair.c | 19 ++
kernel/sched/rt.c | 15 +-
kernel/sched/sched.h | 28 +++
kernel/sys_ni.c | 1 +
14 files changed, 413 insertions(+), 42 deletions(-)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 15908eb9b17e..0b1081a9b872 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -440,3 +440,4 @@
433 i386 fspick sys_fspick __ia32_sys_fspick
434 i386 pidfd_open sys_pidfd_open __ia32_sys_pidfd_open
435 i386 clone3 sys_clone3 __ia32_sys_clone3
+436 i386 pin_on_cpu sys_pin_on_cpu __ia32_sys_pin_on_cpu
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index c29976eca4a8..90f9b3cab88d 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -357,6 +357,7 @@
433 common fspick __x64_sys_fspick
434 common pidfd_open __x64_sys_pidfd_open
435 common clone3 __x64_sys_clone3/ptregs
+436 common pin_on_cpu __x64_sys_pin_on_cpu
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/exec.c b/fs/exec.c
index c27231234764..6d882dbdd1e3 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1827,6 +1827,7 @@ static int __do_execve_file(int fd, struct filename *filename,
current->fs->in_exec = 0;
current->in_execve = 0;
rseq_execve(current);
+ current->pinned_cpu = -1;
acct_update_integrals(current);
task_numa_free(current, false);
free_bprm(bprm);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7f0bb6fff27c..ac0cac7b8d1d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -651,6 +651,7 @@ struct task_struct {
/* Current CPU: */
unsigned int cpu;
#endif
+ int pinned_cpu;
unsigned int wakee_flips;
unsigned long wakee_flip_decay_ts;
struct task_struct *last_wakee;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index be0d0cf788ba..46fee5af99e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1000,6 +1000,7 @@ asmlinkage long sys_fspick(int dfd, const char __user *path, unsigned int flags)
asmlinkage long sys_pidfd_send_signal(int pidfd, int sig,
siginfo_t __user *info,
unsigned int flags);
+asmlinkage long sys_pin_on_cpu(int cmd, int flags, int cpu);
/*
* Architecture-specific system calls
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 1fc8faa6e973..43b0c956cc3c 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -851,8 +851,11 @@ __SYSCALL(__NR_pidfd_open, sys_pidfd_open)
__SYSCALL(__NR_clone3, sys_clone3)
#endif
+#define __NR_pin_on_cpu 436
+__SYSCALL(__NR_pin_on_cpu, sys_pin_on_cpu)
+
#undef __NR_syscalls
-#define __NR_syscalls 436
+#define __NR_syscalls 437
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 25b4fa00bad1..590cdc613698 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -114,4 +114,10 @@ struct clone_args {
SCHED_FLAG_KEEP_ALL | \
SCHED_FLAG_UTIL_CLAMP)
+enum pin_on_cpu_cmd {
+ PIN_ON_CPU_CMD_QUERY = 0,
+ PIN_ON_CPU_CMD_SET = (1 << 0),
+ PIN_ON_CPU_CMD_CLEAR = (1 << 1),
+};
+
#endif /* _UAPI_LINUX_SCHED_H */
diff --git a/init/init_task.c b/init/init_task.c
index 9e5cbe5eab7b..9aabce589cc7 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -88,6 +88,7 @@ struct task_struct init_task
.tasks = LIST_HEAD_INIT(init_task.tasks),
#ifdef CONFIG_SMP
.pushable_tasks = PLIST_NODE_INIT(init_task.pushable_tasks, MAX_PRIO),
+ .pinned_cpu = -1,
#endif
#ifdef CONFIG_CGROUP_SCHED
.sched_task_group = &root_task_group,
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8dacda4b0362..6ca904d6e0ef 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -52,6 +52,8 @@ const_debug unsigned int sysctl_sched_features =
#undef SCHED_FEAT
#endif
+#define PIN_ON_CPU_CMD_BITMASK (PIN_ON_CPU_CMD_SET | PIN_ON_CPU_CMD_CLEAR)
+
/*
* Number of tasks to iterate in a single balance run.
* Limited because this is done with IRQs disabled.
@@ -1457,8 +1459,13 @@ static inline bool is_per_cpu_kthread(struct task_struct *p)
*/
static inline bool is_cpu_allowed(struct task_struct *p, int cpu)
{
- if (!cpumask_test_cpu(cpu, p->cpus_ptr))
- return false;
+ if (is_pinned_task(p)) {
+ if (!allowed_pinned_cpu(p, cpu))
+ return false;
+ } else {
+ if (!cpumask_test_cpu(cpu, p->cpus_ptr))
+ return false;
+ }
if (is_per_cpu_kthread(p))
return cpu_online(cpu);
@@ -1662,6 +1669,12 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
goto out;
}
+ /* Prevent removing the currently pinned CPU from the allowed cpu mask. */
+ if (is_pinned_task(p) && !cpumask_test_cpu(p->pinned_cpu, new_mask)) {
+ ret = -EINVAL;
+ goto out;
+ }
+
do_set_cpus_allowed(p, new_mask);
if (p->flags & PF_KTHREAD) {
@@ -1674,6 +1687,10 @@ static int __set_cpus_allowed_ptr(struct task_struct *p,
p->nr_cpus_allowed != 1);
}
+ /* Task pinned to a CPU overrides allowed cpu mask. */
+ if (is_pinned_task(p))
+ goto out;
+
/* Can the task run on the task's current CPU? If so, we're done */
if (cpumask_test_cpu(task_cpu(p), new_mask))
goto out;
@@ -1813,11 +1830,20 @@ static int migrate_swap_stop(void *data)
if (task_cpu(arg->src_task) != arg->src_cpu)
goto unlock;
- if (!cpumask_test_cpu(arg->dst_cpu, arg->src_task->cpus_ptr))
- goto unlock;
-
- if (!cpumask_test_cpu(arg->src_cpu, arg->dst_task->cpus_ptr))
- goto unlock;
+ if (is_pinned_task(arg->src_task)) {
+ if (!allowed_pinned_cpu(arg->src_task, arg->dst_cpu))
+ goto unlock;
+ } else {
+ if (!cpumask_test_cpu(arg->dst_cpu, arg->src_task->cpus_ptr))
+ goto unlock;
+ }
+ if (is_pinned_task(arg->dst_task)) {
+ if (!allowed_pinned_cpu(arg->dst_task, arg->src_cpu))
+ goto unlock;
+ } else {
+ if (!cpumask_test_cpu(arg->src_cpu, arg->dst_task->cpus_ptr))
+ goto unlock;
+ }
__migrate_swap_task(arg->src_task, arg->dst_cpu);
__migrate_swap_task(arg->dst_task, arg->src_cpu);
@@ -1858,11 +1884,21 @@ int migrate_swap(struct task_struct *cur, struct task_struct *p,
if (!cpu_active(arg.src_cpu) || !cpu_active(arg.dst_cpu))
goto out;
- if (!cpumask_test_cpu(arg.dst_cpu, arg.src_task->cpus_ptr))
- goto out;
+ if (is_pinned_task(arg.src_task)) {
+ if (!allowed_pinned_cpu(arg.src_task, arg.dst_cpu))
+ goto out;
+ } else {
+ if (!cpumask_test_cpu(arg.dst_cpu, arg.src_task->cpus_ptr))
+ goto out;
+ }
- if (!cpumask_test_cpu(arg.src_cpu, arg.dst_task->cpus_ptr))
- goto out;
+ if (is_pinned_task(arg.dst_task)) {
+ if (!allowed_pinned_cpu(arg.dst_task, arg.src_cpu))
+ goto out;
+ } else {
+ if (!cpumask_test_cpu(arg.src_cpu, arg.dst_task->cpus_ptr))
+ goto out;
+ }
trace_sched_swap_numa(cur, arg.src_cpu, p, arg.dst_cpu);
ret = stop_two_cpus(arg.dst_cpu, arg.src_cpu, migrate_swap_stop, &arg);
@@ -2034,6 +2070,18 @@ static int select_fallback_rq(int cpu, struct task_struct *p)
enum { cpuset, possible, fail } state = cpuset;
int dest_cpu;
+ /*
+ * If the task is pinned to a CPU which is online, pick that pinned CPU
+ * number.
+ * If the task is pinned to a CPU which is offline, pick a CPU which is
+ * guaranteed to be the same for all tasks pinned to that offlined CPU.
+ */
+ if (is_pinned_task(p)) {
+ if (cpu_online(p->pinned_cpu))
+ return p->pinned_cpu;
+ else
+ return pinned_cpu_offline_offload(p);
+ }
/*
* If the node that the CPU is on has been offlined, cpu_to_node()
* will return -1. There is no CPU on the node, and we should
@@ -2104,10 +2152,15 @@ int select_task_rq(struct task_struct *p, int cpu, int sd_flags, int wake_flags)
{
lockdep_assert_held(&p->pi_lock);
- if (p->nr_cpus_allowed > 1)
- cpu = p->sched_class->select_task_rq(p, cpu, sd_flags, wake_flags);
- else
- cpu = cpumask_any(p->cpus_ptr);
+ if (is_pinned_task(p))
+ cpu = p->pinned_cpu;
+ else {
+ if (p->nr_cpus_allowed > 1)
+ cpu = p->sched_class->select_task_rq(p, cpu, sd_flags,
+ wake_flags);
+ else
+ cpu = cpumask_any(p->cpus_ptr);
+ }
/*
* In order not to call set_task_cpu() on a blocking task we need
@@ -6130,8 +6183,13 @@ int migrate_task_to(struct task_struct *p, int target_cpu)
if (curr_cpu == target_cpu)
return 0;
- if (!cpumask_test_cpu(target_cpu, p->cpus_ptr))
- return -EINVAL;
+ if (is_pinned_task(p)) {
+ if (!allowed_pinned_cpu(p, target_cpu))
+ return -EINVAL;
+ } else {
+ if (!cpumask_test_cpu(target_cpu, p->cpus_ptr))
+ return -EINVAL;
+ }
/* TODO: This is not properly updating schedstats */
@@ -6300,6 +6358,7 @@ static void migrate_tasks(struct rq *dead_rq, struct rq_flags *rf)
rq->stop = stop;
}
+
#endif /* CONFIG_HOTPLUG_CPU */
void set_rq_online(struct rq *rq)
@@ -6380,11 +6439,100 @@ static int cpuset_cpu_inactive(unsigned int cpu)
return 0;
}
+static bool skip_pinned_task(int pinned_cpu, int cpu,
+ bool first_online)
+{
+ if (pinned_cpu < 0)
+ return true;
+ if (first_online) {
+ if (cpu_online(pinned_cpu) && pinned_cpu != cpu)
+ return true;
+ } else {
+ if (pinned_cpu != cpu)
+ return true;
+ }
+ return false;
+}
+
+static void sched_cpu_migrate_pinned_tasks(unsigned int cpu)
+{
+ struct rq *rq = cpu_rq(cpu);
+ struct task_struct *p, *t;
+ bool first_online = false;
+
+ if (cpu == cpumask_first(cpu_online_mask))
+ first_online = true;
+
+ /*
+ * This state transition (online && !active) when going online
+ * only allow bound kthreads to be scheduled.
+ * At this point, the CPU is completely online and running,
+ * but no userspace tasks are scheduled yet.
+ */
+ read_lock(&tasklist_lock);
+ for_each_process_thread(p, t) {
+ struct rq *target_rq;
+ struct rq_flags rf;
+ int pinned_cpu;
+
+ /*
+ * Migrate t to cpu if pinned to this cpu.
+ *
+ * Migrate t to cpu if its pinned cpu is offline
+ * and cpu becomes the new first online cpu.
+ *
+ * Transition of t->pinned_cpu to cpu can only
+ * happen if the thread is scheduled on cpu, which
+ * is impossible at this point because the cpu is
+ * not active.
+ *
+ * Transition of t->pinned_cpu from cpu to -1 or some
+ * other cpu number may happen concurrently. Therefore,
+ * skip early (without rq lock), and check again with
+ * the rq lock held to eliminate concurrent transitions
+ * from cpu to -1 or some other cpu number.
+ */
+ pinned_cpu = READ_ONCE(t->pinned_cpu);
+ if (skip_pinned_task(pinned_cpu, cpu, first_online))
+ continue;
+ if (pinned_cpu == cpu)
+ printk("pin_on_cpu migrate to owner: online cpu %d\n",
+ cpu);
+ if (first_online && !cpu_online(pinned_cpu))
+ printk("pin_on_cpu migrate to new offload cpu %d\n",
+ cpu);
+ target_rq = task_rq_lock(t, &rf);
+ pinned_cpu = t->pinned_cpu;
+ if (skip_pinned_task(pinned_cpu, cpu, first_online))
+ goto unlock;
+ WARN_ON_ONCE(target_rq == rq);
+ update_rq_clock(target_rq);
+ if (task_running(target_rq, t) || t->state == TASK_WAKING) {
+ struct migration_arg arg = { t, cpu };
+ /* Need help from migration thread: drop lock and wait. */
+ task_rq_unlock(target_rq, t, &rf);
+ stop_one_cpu(cpu_of(target_rq), migration_cpu_stop, &arg);
+ continue;
+ } else if (task_on_rq_queued(t)) {
+ /*
+ * OK, since we're going to drop the lock immediately
+ * afterwards anyway.
+ */
+ rq = move_queued_task(target_rq, &rf, t, cpu);
+ }
+ unlock:
+ task_rq_unlock(target_rq, t, &rf);
+ }
+ read_unlock(&tasklist_lock);
+}
+
int sched_cpu_activate(unsigned int cpu)
{
struct rq *rq = cpu_rq(cpu);
struct rq_flags rf;
+ sched_cpu_migrate_pinned_tasks(cpu);
+
#ifdef CONFIG_SCHED_SMT
/*
* When going up, increment the number of cores with SMT present.
@@ -7899,6 +8047,145 @@ struct cgroup_subsys cpu_cgrp_subsys = {
#endif /* CONFIG_CGROUP_SCHED */
+static void do_set_pinned_cpu(struct task_struct *p, int cpu)
+{
+ struct rq *rq = task_rq(p);
+ bool queued, running;
+
+ lockdep_assert_held(&p->pi_lock);
+
+ queued = task_on_rq_queued(p);
+ running = task_current(rq, p);
+
+ if (queued) {
+ /*
+ * Because __kthread_bind() calls this on blocked tasks without
+ * holding rq->lock.
+ */
+ lockdep_assert_held(&rq->lock);
+ dequeue_task(rq, p, DEQUEUE_SAVE | DEQUEUE_NOCLOCK);
+ }
+ if (running)
+ put_prev_task(rq, p);
+
+ WRITE_ONCE(p->pinned_cpu, cpu);
+
+ if (queued)
+ enqueue_task(rq, p, ENQUEUE_RESTORE | ENQUEUE_NOCLOCK);
+ if (running)
+ set_next_task(rq, p);
+}
+
+static int __do_pin_on_cpu(int cpu)
+{
+ struct task_struct *p = current;
+ struct rq_flags rf;
+ struct rq *rq;
+ int ret = 0, dest_cpu;
+ struct migration_arg arg = { p };
+
+ cpus_read_lock();
+ rq = task_rq_lock(p, &rf);
+ update_rq_clock(rq);
+ if (cpu >= 0 && !cpumask_test_cpu(cpu, current->cpus_ptr)) {
+ ret = -EINVAL;
+ goto out;
+ }
+#ifdef CONFIG_SMP
+ do_set_pinned_cpu(p, cpu);
+ if (cpu >= 0) {
+ if (cpu_online(cpu))
+ dest_cpu = cpu;
+ else
+ dest_cpu = pinned_cpu_offline_offload(p);
+ if (task_cpu(p) == dest_cpu) {
+ /*
+ * If the task already runs on the pinned cpu, we're
+ * done.
+ */
+ goto out;
+ }
+ } else {
+ /*
+ * When clearing the pinned cpu, we may need to migrate the
+ * current task if it is currently sitting on a runqueue which
+ * does not belong to the allowed mask.
+ */
+ dest_cpu = cpumask_any(p->cpus_ptr);
+ }
+ arg.dest_cpu = dest_cpu;
+ /* Need help from migration thread: drop lock and wait. */
+ task_rq_unlock(rq, p, &rf);
+ stop_one_cpu(cpu_of(rq), migration_cpu_stop, &arg);
+
+ /* Preempt disable prevents hotplug on current cpu. */
+ preempt_disable();
+ WARN_ON_ONCE(cpu >= 0 && cpu_online(cpu) &&
+ smp_processor_id() != cpu);
+ preempt_enable();
+ cpus_read_unlock();
+ return 0;
+#endif
+out:
+ task_rq_unlock(rq, p, &rf);
+ cpus_read_unlock();
+ return ret;
+}
+
+static int pin_on_cpu_set(int cpu)
+{
+ if (cpu < 0 || !cpu_possible(cpu)) {
+ return -EINVAL;
+ }
+ return __do_pin_on_cpu(cpu);
+}
+
+static int pin_on_cpu_clear(void)
+{
+ return __do_pin_on_cpu(-1);
+}
+
+/*
+ * sys_pin_on_cpu - pin current task to a specific cpu.
+ * @cmd: command to issue (enum pin_on_cpu_cmd)
+ * @flags: system call flags
+ * @cpu: cpu where the task should run.
+ *
+ * Returns -EINVAL if cmd is unknown.
+ * Returns -EINVAL if flags are unknown.
+ * Returns -EINVAL if the CPU is not part of the possible CPUs.
+ * Returns -EINVAL if the CPU is not part of the allowed cpu mask
+ * for the current task.
+ *
+ * PIN_ON_CPU_CMD_QUERY: Return the mask of supported commands.
+ * PIN_ON_CPU_CMD_SET: Pin the current task to a specific CPU.
+ * PIN_ON_CPU_CMD_CLEAR: Clear cpu pinning for current task.
+ *
+ * If the pinned CPU is online, the current task will run on that CPU.
+ *
+ * If the pinned CPU is offline, the scheduler guarantees that
+ * all tasks pinned to that CPU number are moved to the same
+ * runqueue.
+ *
+ * Removing the pinned CPU from the task's allowed cpu mask is
+ * forbidden.
+ */
+SYSCALL_DEFINE3(pin_on_cpu, int, cmd, int, flags, int, cpu)
+{
+ if (unlikely(flags))
+ return -EINVAL;
+ switch (cmd) {
+ case PIN_ON_CPU_CMD_QUERY:
+ return PIN_ON_CPU_CMD_BITMASK;
+ case PIN_ON_CPU_CMD_SET:
+ return pin_on_cpu_set(cpu);
+ case PIN_ON_CPU_CMD_CLEAR:
+ return pin_on_cpu_clear();
+ default:
+ return -EINVAL;
+ }
+}
+
void dump_cpu_task(int cpu)
{
pr_info("Task dump for CPU %d:\n", cpu);
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index a8a08030a8f7..8a1581e8509e 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -535,24 +535,31 @@ static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p
if (!later_rq) {
int cpu;
- /*
- * If we cannot preempt any rq, fall back to pick any
- * online CPU:
- */
- cpu = cpumask_any_and(cpu_active_mask, p->cpus_ptr);
- if (cpu >= nr_cpu_ids) {
- /*
- * Failed to find any suitable CPU.
- * The task will never come back!
- */
- BUG_ON(dl_bandwidth_enabled());
-
+ if (is_pinned_task(p)) {
+ if (cpu_online(p->pinned_cpu))
+ cpu = p->pinned_cpu;
+ else
+ cpu = pinned_cpu_offline_offload(p);
+ } else {
/*
- * If admission control is disabled we
- * try a little harder to let the task
- * run.
+ * If we cannot preempt any rq, fall back to pick any
+ * online CPU:
*/
- cpu = cpumask_any(cpu_active_mask);
+ cpu = cpumask_any_and(cpu_active_mask, p->cpus_ptr);
+ if (cpu >= nr_cpu_ids) {
+ /*
+ * Failed to find any suitable CPU.
+ * The task will never come back!
+ */
+ BUG_ON(dl_bandwidth_enabled());
+
+ /*
+ * If admission control is disabled we
+ * try a little harder to let the task
+ * run.
+ */
+ cpu = cpumask_any(cpu_active_mask);
+ }
}
later_rq = cpu_rq(cpu);
double_lock_balance(rq, later_rq);
@@ -1836,9 +1843,15 @@ static void task_fork_dl(struct task_struct *p)
static int pick_dl_task(struct rq *rq, struct task_struct *p, int cpu)
{
- if (!task_running(rq, p) &&
- cpumask_test_cpu(cpu, p->cpus_ptr))
- return 1;
+ if (!task_running(rq, p)) {
+ if (is_pinned_task(p)) {
+ if (allowed_pinned_cpu(p, cpu))
+ return 1;
+ } else {
+ if (cpumask_test_cpu(cpu, p->cpus_ptr))
+ return 1;
+ }
+ }
return 0;
}
@@ -1987,7 +2000,8 @@ static struct rq *find_lock_later_rq(struct task_struct *task, struct rq *rq)
/* Retry if something changed. */
if (double_lock_balance(rq, later_rq)) {
if (unlikely(task_rq(task) != rq ||
- !cpumask_test_cpu(later_rq->cpu, task->cpus_ptr) ||
+ (is_pinned_task(task) && !allowed_pinned_cpu(task, later_rq->cpu)) ||
+ (!is_pinned_task(task) && !cpumask_test_cpu(later_rq->cpu, task->cpus_ptr)) ||
task_running(rq, task) ||
!dl_task(task) ||
!task_on_rq_queued(task))) {
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 69a81a5709ff..e96ae1ce9829 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7223,6 +7223,25 @@ int can_migrate_task(struct task_struct *p, struct lb_env *env)
lockdep_assert_held(&env->src_rq->lock);
+ if (is_pinned_task(p)) {
+ if (task_running(env->src_rq, p)) {
+ schedstat_inc(p->se.statistics.nr_failed_migrations_running);
+ return 0;
+ }
+ if (cpu_online(p->pinned_cpu)) {
+ if (env->dst_cpu == p->pinned_cpu)
+ return 1;
+ else
+ return 0;
+ } else {
+ if (env->dst_cpu ==
+ pinned_cpu_offline_offload(p))
+ return 1;
+ else
+ return 0;
+ }
+ }
+
/*
* We do not migrate tasks that are:
* 1) throttled_lb_pair, or
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 9b8adc01be3d..2774311e5750 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1600,9 +1600,15 @@ static void put_prev_task_rt(struct rq *rq, struct task_struct *p)
static int pick_rt_task(struct rq *rq, struct task_struct *p, int cpu)
{
- if (!task_running(rq, p) &&
- cpumask_test_cpu(cpu, p->cpus_ptr))
- return 1;
+ if (!task_running(rq, p)) {
+ if (is_pinned_task(p)) {
+ if (allowed_pinned_cpu(p, cpu))
+ return 1;
+ } else {
+ if (cpumask_test_cpu(cpu, p->cpus_ptr))
+ return 1;
+ }
+ }
return 0;
}
@@ -1738,7 +1744,8 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
* Also make sure that it wasn't scheduled on its rq.
*/
if (unlikely(task_rq(task) != rq ||
- !cpumask_test_cpu(lowest_rq->cpu, task->cpus_ptr) ||
+ (is_pinned_task(task) && !allowed_pinned_cpu(task, lowest_rq->cpu)) ||
+ (!is_pinned_task(task) && !cpumask_test_cpu(lowest_rq->cpu, task->cpus_ptr)) ||
task_running(rq, task) ||
!rt_task(task) ||
!task_on_rq_queued(task))) {
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 49ed949f850c..922bc618cc87 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -187,6 +187,34 @@ static inline int task_has_dl_policy(struct task_struct *p)
return dl_policy(p->policy);
}
+/*
+ * All tasks which require to be pinned on offlined CPUs are sent
+ * to runqueue of the first online CPU.
+ */
+static inline bool is_pinned_task(struct task_struct *p)
+{
+ return p->pinned_cpu >= 0;
+}
+
+static inline int pinned_cpu_offline_offload(struct task_struct *p)
+{
+ return cpumask_first(cpu_online_mask);
+}
+
+static inline bool allowed_pinned_cpu(struct task_struct *p, int cpu)
+{
+ if (!cpu_possible(cpu))
+ return false;
+ if (cpu_online(p->pinned_cpu)) {
+ if (p->pinned_cpu == cpu)
+ return true;
+ } else {
+ if (cpu == pinned_cpu_offline_offload(p))
+ return true;
+ }
+ return false;
+}
+
#define cap_scale(v, s) ((v)*(s) >> SCHED_CAPACITY_SHIFT)
/*
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 34b76895b81e..7e5192cd8d9d 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -449,3 +449,4 @@ COND_SYSCALL(setuid16);
/* restartable sequence */
COND_SYSCALL(rseq);
+COND_SYSCALL(pin_on_cpu);
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Jann Horn @ 2020-01-21 17:20 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Thomas Gleixner, kernel list, Joel Fernandes,
Ingo Molnar, Catalin Marinas, Dave Watson, Will Deacon,
Shuah Khan, Andi Kleen, open list:KERNEL SELFTEST FRAMEWORK,
H . Peter Anvin, Chris Lameter, Russell King, Michael Kerrisk,
Paul E . McKenney, Paul Turner, Boqun Feng, Josh Triplett, Stev
In-Reply-To: <20200121160312.26545-1-mathieu.desnoyers@efficios.com>
On Tue, Jan 21, 2020 at 5:13 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> There is an important use-case which is not possible with the
> "rseq" (Restartable Sequences) system call, which was left as
> future work.
>
> That use-case is to modify user-space per-cpu data structures
> belonging to specific CPUs which may be brought offline and
> online again by CPU hotplug. This can be used by memory
> allocators to migrate free memory pools when CPUs are brought
> offline, or by ring buffer consumers to target specific per-CPU
> buffers, even when CPUs are brought offline.
>
> A few rather complex prior attempts were made to solve this.
> Those were based on in-kernel interpreters (cpu_opv, do_on_cpu).
> That complexity was generally frowned upon, even by their author.
>
> This patch fulfills this use-case in a refreshingly simple way:
> it introduces a "pin_on_cpu" system call, which allows user-space
> threads to pin themselves on a specific CPU (which needs to be
> present in the thread's allowed cpu mask), and then clear this
> pinned state.
[...]
> For instance, this allows implementing this userspace library API
> for incrementing a per-cpu counter for a specific cpu number
> received as parameter:
>
> static inline __attribute__((always_inline))
> int percpu_addv(intptr_t *v, intptr_t count, int cpu)
> {
> int ret;
>
> ret = rseq_addv(v, count, cpu);
> check:
> if (rseq_unlikely(ret)) {
> pin_on_cpu_set(cpu);
> ret = rseq_addv(v, count, percpu_current_cpu());
> pin_on_cpu_clear();
> goto check;
> }
> return 0;
> }
What does userspace have to do if the set of allowed CPUs switches all
the time? For example, on Android, if you first open Chrome and then
look at its allowed CPUs, Chrome is allowed to use all CPU cores
because it's running in the foreground:
walleye:/ # ps -AZ | grep 'android.chrome$'
u:r:untrusted_app:s0:c145,c256,c512,c768 u0_a145 7845 805 1474472
197868 SyS_epoll_wait f09c0194 S com.android.chrome
walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
/proc/7845/status
3:cpuset:/top-app
Cpus_allowed_list: 0-7
But if you then switch to the home screen, the application is moved
into a different cgroup, and is restricted to two CPU cores:
walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
/proc/7845/status
3:cpuset:/background
Cpus_allowed_list: 0-1
At the same time, I also wonder whether it is a good idea to allow
userspace to stay active on a CPU even after the task has been told to
move to another CPU core - that's probably not exactly a big deal, but
seems suboptimal to me.
I'm wondering whether it might be possible to rework this mechanism
such that, instead of moving the current task onto a target CPU, it
prevents all *other* threads of the current process from running on
that CPU (either entirely or in user mode). That might be the easiest
way to take care of issues like CPU hotplugging and changing cpusets
all at once? The only potential issue I see with that approach would
be that you wouldn't be able to use it for inter-process
communication; and I have no idea whether this would be good or bad
performance-wise.
^ permalink raw reply
* Re: [PATCH v2 4/5] mm/madvise: allow KSM hints for remote API
From: Minchan Kim @ 2020-01-21 17:45 UTC (permalink / raw)
To: Oleksandr Natalenko
Cc: Kirill Tkhai, Andrew Morton, LKML, linux-mm,
linux-api-u79uwXL29TY76Z2rM5mHXA, Suren Baghdasaryan, Tim Murray,
Daniel Colascione, Sandeep Patil, Sonny Rao, Brian Geffon,
Michal Hocko, Johannes Weiner, Shakeel Butt, John Dias,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200117123400.o3ne6kazkovq4okd-1pX4wqAwPg9Bd2nDH4ldLLp2dZbC/Bob@public.gmane.org>
On Fri, Jan 17, 2020 at 01:34:00PM +0100, Oleksandr Natalenko wrote:
> Hi.
>
> On Fri, Jan 17, 2020 at 01:13:14PM +0300, Kirill Tkhai wrote:
> > On 17.01.2020 02:59, Minchan Kim wrote:
> > > From: Oleksandr Natalenko <oleksandr-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >
> > > It all began with the fact that KSM works only on memory that is marked
> > > by madvise(). And the only way to get around that is to either:
> > >
> > > * use LD_PRELOAD; or
> > > * patch the kernel with something like UKSM or PKSM.
> > >
> > > (i skip ptrace can of worms here intentionally)
> > >
> > > To overcome this restriction, lets employ a new remote madvise API. This
> > > can be used by some small userspace helper daemon that will do auto-KSM
> > > job for us.
> > >
> > > I think of two major consumers of remote KSM hints:
> > >
> > > * hosts, that run containers, especially similar ones and especially in
> > > a trusted environment, sharing the same runtime like Node.js;
> > >
> > > * heavy applications, that can be run in multiple instances, not
> > > limited to opensource ones like Firefox, but also those that cannot be
> > > modified since they are binary-only and, maybe, statically linked.
> > >
> > > Speaking of statistics, more numbers can be found in the very first
> > > submission, that is related to this one [1]. For my current setup with
> > > two Firefox instances I get 100 to 200 MiB saved for the second instance
> > > depending on the amount of tabs.
> > >
> > > 1 FF instance with 15 tabs:
> > >
> > > $ echo "$(cat /sys/kernel/mm/ksm/pages_sharing) * 4 / 1024" | bc
> > > 410
> > >
> > > 2 FF instances, second one has 12 tabs (all the tabs are different):
> > >
> > > $ echo "$(cat /sys/kernel/mm/ksm/pages_sharing) * 4 / 1024" | bc
> > > 592
> > >
> > > At the very moment I do not have specific numbers for containerised
> > > workload, but those should be comparable in case the containers share
> > > similar/same runtime.
> > >
> > > [1] https://lore.kernel.org/patchwork/patch/1012142/
> > >
> > > Signed-off-by: Oleksandr Natalenko <oleksandr-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Minchan Kim <minchan-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > ---
> > > mm/madvise.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 84cffd0900f1..89557998d287 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -1000,6 +1000,8 @@ process_madvise_behavior_valid(int behavior)
> > > switch (behavior) {
> > > case MADV_COLD:
> > > case MADV_PAGEOUT:
> > > + case MADV_MERGEABLE:
> > > + case MADV_UNMERGEABLE:
> > > return true;
> > > default:
> > > return false;
> >
> > Remote madvise on KSM parameters should be OK.
> >
> > One thing is madvise_behavior_valid() places MADV_MERGEABLE/UNMERGEABLE
> > in #ifdef brackes, so -EINVAL is returned by madvise() syscall if KSM
> > is not enabled. Here we should follow the same way for symmetry.
> >
>
> Thanks for the suggestion.
>
> Minchan, shall you adopt it directly, or I should send a separate patch?
I will handle it in next spin.
^ permalink raw reply
* Dear beloved,
From: Rita Johnson @ 2020-01-21 17:56 UTC (permalink / raw)
--
Hello
My name is Mrs Rita Johnson,I want to donate my fund USd $8.6m to you
on charity name to help the poor people.As soon as i read from you i
will give more details on how to achieve this goal and get this fund
transfer to your bank account.
Regards
Mrs Rita Johnson
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Minchan Kim @ 2020-01-21 18:11 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Michal Hocko, Andrew Morton, LKML, linux-mm,
linux-api-u79uwXL29TY76Z2rM5mHXA,
oleksandr-H+wXaHxf7aLQT0dZR+AlfA, Suren Baghdasaryan, Tim Murray,
Daniel Colascione, Sandeep Patil, Sonny Rao, Brian Geffon,
Johannes Weiner, Shakeel Butt, John Dias,
ktkhai-5HdwGun5lf+gSpxsJD1C4w,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200117212653.7uftw3lk35oykkmb@box>
On Sat, Jan 18, 2020 at 12:26:53AM +0300, Kirill A. Shutemov wrote:
> On Fri, Jan 17, 2020 at 09:32:39AM -0800, Minchan Kim wrote:
> > On Fri, Jan 17, 2020 at 06:58:37PM +0300, Kirill A. Shutemov wrote:
> > > On Fri, Jan 17, 2020 at 12:52:25PM +0100, Michal Hocko wrote:
> > > > On Thu 16-01-20 15:59:50, Minchan Kim wrote:
> > > > > There is usecase that System Management Software(SMS) want to give
> > > > > a memory hint like MADV_[COLD|PAGEEOUT] to other processes and
> > > > > in the case of Android, it is the ActivityManagerService.
> > > > >
> > > > > It's similar in spirit to madvise(MADV_WONTNEED), but the information
> > > > > required to make the reclaim decision is not known to the app. Instead,
> > > > > it is known to the centralized userspace daemon(ActivityManagerService),
> > > > > and that daemon must be able to initiate reclaim on its own without
> > > > > any app involvement.
> > > > >
> > > > > To solve the issue, this patch introduces new syscall process_madvise(2).
> > > > > It uses pidfd of an external processs to give the hint.
> > > > >
> > > > > int process_madvise(int pidfd, void *addr, size_t length, int advise,
> > > > > unsigned long flag);
> > > > >
> > > > > Since it could affect other process's address range, only privileged
> > > > > process(CAP_SYS_PTRACE) or something else(e.g., being the same UID)
> > > > > gives it the right to ptrace the process could use it successfully.
> > > > > The flag argument is reserved for future use if we need to extend the
> > > > > API.
> > > > >
> > > > > I think supporting all hints madvise has/will supported/support to
> > > > > process_madvise is rather risky. Because we are not sure all hints make
> > > > > sense from external process and implementation for the hint may rely on
> > > > > the caller being in the current context so it could be error-prone.
> > > > > Thus, I just limited hints as MADV_[COLD|PAGEOUT] in this patch.
> > > > >
> > > > > If someone want to add other hints, we could hear hear the usecase and
> > > > > review it for each hint. It's more safe for maintainace rather than
> > > > > introducing a buggy syscall but hard to fix it later.
> > > >
> > > > I have brought this up when we discussed this in the past but there is
> > > > no reflection on that here so let me bring that up again.
> > > >
> > > > I believe that the interface has an inherent problem that it is racy.
> > > > The external entity needs to know the address space layout of the target
> > > > process to do anyhing useful on it. The address space is however under
> > > > the full control of the target process though and the external entity
> > > > has no means to find out that the layout has changed. So
> > > > time-to-check-time-to-act is an inherent problem.
> > > >
> > > > This is a serious design flaw and it should be explained why it doesn't
> > > > matter or how to use the interface properly to prevent that problem.
> > >
> > > I agree, it looks flawed.
> > >
> > > Also I don't see what System Management Software can generically do on
> > > sub-process level. I mean how can it decide which part of address space is
> > > less important than other.
> > >
> > > I see how a manager can indicate that this process (or a group of
> > > processes) is less important than other, but on per-addres-range basis?
> >
> > For example, memory ranges shared by several processes or critical for the
> > latency, we could avoid those ranges to be cold/pageout to prevent
> > unncecessary CPU burning/paging.
>
> Hmm.. I still don't see why any external entity has a better (or any)
> knowledge about the matter. The process has to do this, no?
I think Sandeep already gave enough information in other thread.
>
> > I also think people don't want to give an KSM hint to non-mergeable area.
>
> And how the manager knows which data is mergable?
Oleksandr, could you say your thought why you need address range based
API?
>
> If you are intimate enough with the process' internal state feel free to
> inject syscall into the process with ptrace. Why bother with half-measures?
Concern is we want to act the hint in caller's context, not calle because
calle is usually very limited in cpuset/cgroups or even freezed state so
they couldn't act by themselves quick enough, which makes many problems.
One of efforts to solve the issue was "Expedited memory reclaim"
https://lwn.net/Articles/785709/
That could be also a good candidate for process_madvise API.
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Minchan Kim @ 2020-01-21 18:32 UTC (permalink / raw)
To: Michal Hocko
Cc: sspatil, kirill, akpm, linux-kernel, linux-mm, linux-api,
oleksandr, surenb, timmurray, dancol, sonnyrao, bgeffon, hannes,
shakeelb, joaodias, ktkhai, christian.brauner, sjpark
In-Reply-To: <20200120075825.GH18451@dhcp22.suse.cz>
On Mon, Jan 20, 2020 at 08:58:25AM +0100, Michal Hocko wrote:
> On Sun 19-01-20 08:14:31, sspatil@google.com wrote:
> > On Sat, Jan 18, 2020 at 12:26:53AM +0300, Kirill A. Shutemov wrote:
> > > On Fri, Jan 17, 2020 at 09:32:39AM -0800, Minchan Kim wrote:
> > > > On Fri, Jan 17, 2020 at 06:58:37PM +0300, Kirill A. Shutemov wrote:
> > > > > On Fri, Jan 17, 2020 at 12:52:25PM +0100, Michal Hocko wrote:
> > > > > > On Thu 16-01-20 15:59:50, Minchan Kim wrote:
> > > > > > > There is usecase that System Management Software(SMS) want to give
> > > > > > > a memory hint like MADV_[COLD|PAGEEOUT] to other processes and
> > > > > > > in the case of Android, it is the ActivityManagerService.
> > > > > > >
> > > > > > > It's similar in spirit to madvise(MADV_WONTNEED), but the information
> > > > > > > required to make the reclaim decision is not known to the app. Instead,
> > > > > > > it is known to the centralized userspace daemon(ActivityManagerService),
> > > > > > > and that daemon must be able to initiate reclaim on its own without
> > > > > > > any app involvement.
> > > > > > >
> > > > > > > To solve the issue, this patch introduces new syscall process_madvise(2).
> > > > > > > It uses pidfd of an external processs to give the hint.
> > > > > > >
> > > > > > > int process_madvise(int pidfd, void *addr, size_t length, int advise,
> > > > > > > unsigned long flag);
> > > > > > >
> > > > > > > Since it could affect other process's address range, only privileged
> > > > > > > process(CAP_SYS_PTRACE) or something else(e.g., being the same UID)
> > > > > > > gives it the right to ptrace the process could use it successfully.
> > > > > > > The flag argument is reserved for future use if we need to extend the
> > > > > > > API.
> > > > > > >
> > > > > > > I think supporting all hints madvise has/will supported/support to
> > > > > > > process_madvise is rather risky. Because we are not sure all hints make
> > > > > > > sense from external process and implementation for the hint may rely on
> > > > > > > the caller being in the current context so it could be error-prone.
> > > > > > > Thus, I just limited hints as MADV_[COLD|PAGEOUT] in this patch.
> > > > > > >
> > > > > > > If someone want to add other hints, we could hear hear the usecase and
> > > > > > > review it for each hint. It's more safe for maintainace rather than
> > > > > > > introducing a buggy syscall but hard to fix it later.
> > > > > >
> > > > > > I have brought this up when we discussed this in the past but there is
> > > > > > no reflection on that here so let me bring that up again.
> > > > > >
> > > > > > I believe that the interface has an inherent problem that it is racy.
> > > > > > The external entity needs to know the address space layout of the target
> > > > > > process to do anyhing useful on it. The address space is however under
> > > > > > the full control of the target process though and the external entity
> > > > > > has no means to find out that the layout has changed. So
> > > > > > time-to-check-time-to-act is an inherent problem.
> > > > > >
> > > > > > This is a serious design flaw and it should be explained why it doesn't
> > > > > > matter or how to use the interface properly to prevent that problem.
> > > > >
> > > > > I agree, it looks flawed.
> > > > >
> > > > > Also I don't see what System Management Software can generically do on
> > > > > sub-process level. I mean how can it decide which part of address space is
> > > > > less important than other.
> > > > >
> > > > > I see how a manager can indicate that this process (or a group of
> > > > > processes) is less important than other, but on per-addres-range basis?
> > > >
> > > > For example, memory ranges shared by several processes or critical for the
> > > > latency, we could avoid those ranges to be cold/pageout to prevent
> > > > unncecessary CPU burning/paging.
> > >
> > > Hmm.. I still don't see why any external entity has a better (or any)
> > > knowledge about the matter. The process has to do this, no?
> >
> > FWIW, I totally agree with the time-to-check-time-to-react problem. However,
> > I'd like to clarify the ActivityManager/SystemServer case (I'll call it
> > SystemServer from now on)
> >
> > For Android, every application (including the special SystemServer) are forked
> > from Zygote. The reason ofcourse is to share as many libraries and classes between
> > the two as possible to benefit from the preloading during boot.
> >
> > After applications start, (almost) all of the APIs end up calling into this
> > SystemServer process over IPC (binder) and back to the application.
> >
> > In a fully running system, the SystemServer monitors every single process
> > periodically to calculate their PSS / RSS and also decides which process is
> > "important" to the user for interactivity.
> >
> > So, because of how these processes start _and_ the fact that the SystemServer
> > is looping to monitor each process, it does tend to *know* which address
> > range of the application is not used / useful.
> >
> > Besides, we can never rely on applications to clean things up themselves.
> > We've had the "hey app1, the system is low on memory, please trim your
> > memory usage down" notifications for a long time[1]. They rely on
> > applications honoring the broadcasts and very few do.
> >
> > So, if we want to avoid the inevitable killing of the application and
> > restarting it, some way to be able to tell the OS about unimportant memory in
> > these applications will be useful.
Thanks for adding more useful description, Sandeep.
>
> This is a useful information that should be a part of the changelog. I
I will incldue it in next respin.
> do see how the current form of the API might fit into Android model
> without many problems. But we are not designing an API for a single
> usecase, right? In a highly cooperative environments you can use ptrace
> code injection as mentioned by Kirill. Or is there any fundamental
> problem about that?
I replied it at Kirill's thread so let's discuss there.
>
> The interface really has to be robust to future potential usecases.
I do understand your concern but for me, it's chicken and egg problem.
We usually do best effort to make something perfect as far as possible
but we also don't do over-engineering without real usecase from the
beginning.
I already told you how we could synchronize among processes and potential
way to be extended Daniel suggested(That's why current API has extra field
for the cookie) even though we don't need it right now.
If you want to suggest the other way, please explain why your idea is
better and why we need it at this moment. I don't think that is a
blocker for the progress of this API since we already have several ways
to synchronize processes.
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Minchan Kim @ 2020-01-21 18:43 UTC (permalink / raw)
To: Michal Hocko
Cc: Kirill A. Shutemov, Kirill Tkhai, Andrew Morton, LKML, linux-mm,
linux-api-u79uwXL29TY76Z2rM5mHXA,
oleksandr-H+wXaHxf7aLQT0dZR+AlfA, Suren Baghdasaryan, Tim Murray,
Daniel Colascione, Sandeep Patil, Sonny Rao, Brian Geffon,
Johannes Weiner, Shakeel Butt, John Dias,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200120132405.GF18451-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org>
On Mon, Jan 20, 2020 at 02:24:05PM +0100, Michal Hocko wrote:
> On Mon 20-01-20 15:39:35, Kirill A. Shutemov wrote:
> > On Mon, Jan 20, 2020 at 12:27:22PM +0100, Michal Hocko wrote:
> > > On Mon 20-01-20 13:24:35, Kirill Tkhai wrote:
> [...]
> > > > Even two threads on common memory need a synchronization
> > > > to manage mappings in a sane way. Managing memory from two processes
> > > > is the same in principle, and the only difference is that another level
> > > > of synchronization is required.
> > >
> > > Well, not really. The operation might simply attempt to perform an
> > > operation on a specific memory area and get a failure if it doesn't
> > > reference the same object anymore. What I think we need is some form of
> > > a handle to operate on. In the past we have discussed several
> > > directions. I was proposing /proc/self/map_anon/ (analogous to
> > > map_files) where you could inspect anonymous memory and get a file
> > > handle for it. madvise would then operate on the fd and then there
> > > shouldn't be a real problem to revalidate that the object is still
> > > valid. But there was no general enthusiasm about that approach. There
> > > are likely some land mines on the way.
> >
> > Converting anon memory to file-backed is bad idea and going to backfire.
>
> I didn't mean to convert. I meant to expose that information via proc
> the same way we do for file backed mappings. That shouldn't really
> require to re-design the way how anonymous vma work IMO. But I haven't
> tried that so there might be many gotchas there.
>
> There are obvious things to think about though. Such fd cannot be sent
> to other processes (SCM stuff), mmap of the file would have to be
> disallowed and many others I am not aware of. I am not even pushing this
> direction because I am not convinced about how viable it is myself. But
> it would sound like a nice extension of the existing mechanism we have
> and a file based madvise sounds attractive to me as well because we
> already have that.
I am not a fan of fd based approach but I already reserved last argument
of the API as extendable field so we could use the field as "fd" when we
really need that kinds of fine-grained synchronization model if it's not
enough with SGISTOP, freezer and so.
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Mathieu Desnoyers @ 2020-01-21 19:47 UTC (permalink / raw)
To: Jann Horn
Cc: Peter Zijlstra, Thomas Gleixner, linux-kernel, Joel Fernandes,
Ingo Molnar, Catalin Marinas, Dave Watson, Will Deacon, shuah,
Andi Kleen, linux-kselftest, H. Peter Anvin, Chris Lameter,
Russell King, Michael Kerrisk, Paul E . McKenney, Paul Turner,
Boqun Feng, Josh Triplett, rostedt, Ben Maurer <bmaur>
In-Reply-To: <CAG48ez2bQdoT9y7HkyU06DTazysUDdPdJe+gyV-NxgQA7JWQVQ@mail.gmail.com>
----- On Jan 21, 2020, at 12:20 PM, Jann Horn jannh@google.com wrote:
> On Tue, Jan 21, 2020 at 5:13 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> There is an important use-case which is not possible with the
>> "rseq" (Restartable Sequences) system call, which was left as
>> future work.
>>
>> That use-case is to modify user-space per-cpu data structures
>> belonging to specific CPUs which may be brought offline and
>> online again by CPU hotplug. This can be used by memory
>> allocators to migrate free memory pools when CPUs are brought
>> offline, or by ring buffer consumers to target specific per-CPU
>> buffers, even when CPUs are brought offline.
>>
>> A few rather complex prior attempts were made to solve this.
>> Those were based on in-kernel interpreters (cpu_opv, do_on_cpu).
>> That complexity was generally frowned upon, even by their author.
>>
>> This patch fulfills this use-case in a refreshingly simple way:
>> it introduces a "pin_on_cpu" system call, which allows user-space
>> threads to pin themselves on a specific CPU (which needs to be
>> present in the thread's allowed cpu mask), and then clear this
>> pinned state.
> [...]
>> For instance, this allows implementing this userspace library API
>> for incrementing a per-cpu counter for a specific cpu number
>> received as parameter:
>>
>> static inline __attribute__((always_inline))
>> int percpu_addv(intptr_t *v, intptr_t count, int cpu)
>> {
>> int ret;
>>
>> ret = rseq_addv(v, count, cpu);
>> check:
>> if (rseq_unlikely(ret)) {
>> pin_on_cpu_set(cpu);
>> ret = rseq_addv(v, count, percpu_current_cpu());
>> pin_on_cpu_clear();
>> goto check;
>> }
>> return 0;
>> }
>
> What does userspace have to do if the set of allowed CPUs switches all
> the time? For example, on Android, if you first open Chrome and then
> look at its allowed CPUs, Chrome is allowed to use all CPU cores
> because it's running in the foreground:
>
> walleye:/ # ps -AZ | grep 'android.chrome$'
> u:r:untrusted_app:s0:c145,c256,c512,c768 u0_a145 7845 805 1474472
> 197868 SyS_epoll_wait f09c0194 S com.android.chrome
> walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
> /proc/7845/status
> 3:cpuset:/top-app
> Cpus_allowed_list: 0-7
>
> But if you then switch to the home screen, the application is moved
> into a different cgroup, and is restricted to two CPU cores:
>
> walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
> /proc/7845/status
> 3:cpuset:/background
> Cpus_allowed_list: 0-1
Then at that point, pin_on_cpu() would only be allowed to pin on
CPUs 0 and 1.
> At the same time, I also wonder whether it is a good idea to allow
> userspace to stay active on a CPU even after the task has been told to
> move to another CPU core - that's probably not exactly a big deal, but
> seems suboptimal to me.
Do you mean the following scenario for a given task ?
1) set affinity to CPU 0,1,2
2) pin on CPU 2
3) set affinity to CPU 0,1
In the patch I propose here, step (3) is forbidden by this added check in
__set_cpus_allowed_ptr, which is used by sched_setaffinity(2):
+ /* Prevent removing the currently pinned CPU from the allowed cpu mask. */
+ if (is_pinned_task(p) && !cpumask_test_cpu(p->pinned_cpu, new_mask)) {
+ ret = -EINVAL;
+ goto out;
+ }
> I'm wondering whether it might be possible to rework this mechanism
> such that, instead of moving the current task onto a target CPU, it
> prevents all *other* threads of the current process from running on
> that CPU (either entirely or in user mode). That might be the easiest
> way to take care of issues like CPU hotplugging and changing cpusets
> all at once? The only potential issue I see with that approach would
> be that you wouldn't be able to use it for inter-process
> communication; and I have no idea whether this would be good or bad
> performance-wise.
Firstly, inter-process communication over shared memory is one of my use-cases
(for lttng-ust's ring buffer).
I'm not convinced that applying constraints on all other threads belonging to
the current process would be easier or faster than migrating the current thread
over to the target CPU. I'm unsure how those additional constraints would
fit with other threads already having their own cpu affinity masks (which
could generate an empty cpumask by adding an extra constraint).
Thanks for the feedback!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Jann Horn @ 2020-01-21 20:35 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Thomas Gleixner, linux-kernel, Joel Fernandes,
Ingo Molnar, Catalin Marinas, Dave Watson, Will Deacon, shuah,
Andi Kleen, linux-kselftest, H. Peter Anvin, Chris Lameter,
Russell King, Michael Kerrisk, Paul E . McKenney, Paul Turner,
Boqun Feng, Josh Triplett, rostedt, Ben Maurer <bmaur>
In-Reply-To: <430172781.596271.1579636021412.JavaMail.zimbra@efficios.com>
On Tue, Jan 21, 2020 at 8:47 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Jan 21, 2020, at 12:20 PM, Jann Horn jannh@google.com wrote:
>
> > On Tue, Jan 21, 2020 at 5:13 PM Mathieu Desnoyers
> > <mathieu.desnoyers@efficios.com> wrote:
> >> There is an important use-case which is not possible with the
> >> "rseq" (Restartable Sequences) system call, which was left as
> >> future work.
> >>
> >> That use-case is to modify user-space per-cpu data structures
> >> belonging to specific CPUs which may be brought offline and
> >> online again by CPU hotplug. This can be used by memory
> >> allocators to migrate free memory pools when CPUs are brought
> >> offline, or by ring buffer consumers to target specific per-CPU
> >> buffers, even when CPUs are brought offline.
> >>
> >> A few rather complex prior attempts were made to solve this.
> >> Those were based on in-kernel interpreters (cpu_opv, do_on_cpu).
> >> That complexity was generally frowned upon, even by their author.
> >>
> >> This patch fulfills this use-case in a refreshingly simple way:
> >> it introduces a "pin_on_cpu" system call, which allows user-space
> >> threads to pin themselves on a specific CPU (which needs to be
> >> present in the thread's allowed cpu mask), and then clear this
> >> pinned state.
> > [...]
> >> For instance, this allows implementing this userspace library API
> >> for incrementing a per-cpu counter for a specific cpu number
> >> received as parameter:
> >>
> >> static inline __attribute__((always_inline))
> >> int percpu_addv(intptr_t *v, intptr_t count, int cpu)
> >> {
> >> int ret;
> >>
> >> ret = rseq_addv(v, count, cpu);
> >> check:
> >> if (rseq_unlikely(ret)) {
> >> pin_on_cpu_set(cpu);
> >> ret = rseq_addv(v, count, percpu_current_cpu());
> >> pin_on_cpu_clear();
> >> goto check;
> >> }
> >> return 0;
> >> }
> >
> > What does userspace have to do if the set of allowed CPUs switches all
> > the time? For example, on Android, if you first open Chrome and then
> > look at its allowed CPUs, Chrome is allowed to use all CPU cores
> > because it's running in the foreground:
> >
> > walleye:/ # ps -AZ | grep 'android.chrome$'
> > u:r:untrusted_app:s0:c145,c256,c512,c768 u0_a145 7845 805 1474472
> > 197868 SyS_epoll_wait f09c0194 S com.android.chrome
> > walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
> > /proc/7845/status
> > 3:cpuset:/top-app
> > Cpus_allowed_list: 0-7
> >
> > But if you then switch to the home screen, the application is moved
> > into a different cgroup, and is restricted to two CPU cores:
> >
> > walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
> > /proc/7845/status
> > 3:cpuset:/background
> > Cpus_allowed_list: 0-1
>
> Then at that point, pin_on_cpu() would only be allowed to pin on
> CPUs 0 and 1.
Which means that you can't actually reliably use pin_on_cpu_set() to
manipulate percpu data structures since you have to call it with the
assumption that it might randomly fail at any time, right? And then
userspace needs to code a fallback path that somehow halts all the
threads with thread-directed signals or something?
Especially if the task trying to use pin_on_cpu_set() isn't allowed to
pin to the target CPU, but all the other tasks using the shared data
structure are allowed to do that. Or if the CPU you want to pin to is
always removed from your affinity mask immediately before
pin_on_cpu_set() and added back immediately afterwards.
> > At the same time, I also wonder whether it is a good idea to allow
> > userspace to stay active on a CPU even after the task has been told to
> > move to another CPU core - that's probably not exactly a big deal, but
> > seems suboptimal to me.
>
> Do you mean the following scenario for a given task ?
>
> 1) set affinity to CPU 0,1,2
> 2) pin on CPU 2
> 3) set affinity to CPU 0,1
>
> In the patch I propose here, step (3) is forbidden by this added check in
> __set_cpus_allowed_ptr, which is used by sched_setaffinity(2):
>
> + /* Prevent removing the currently pinned CPU from the allowed cpu mask. */
> + if (is_pinned_task(p) && !cpumask_test_cpu(p->pinned_cpu, new_mask)) {
> + ret = -EINVAL;
> + goto out;
> + }
How does that interact with things like cgroups? What happens when
root tries to move a process from the cpuset:/top-app cgroup to the
cpuset:/background cgroup? Does that also just throw an error code?
> > I'm wondering whether it might be possible to rework this mechanism
> > such that, instead of moving the current task onto a target CPU, it
> > prevents all *other* threads of the current process from running on
> > that CPU (either entirely or in user mode). That might be the easiest
> > way to take care of issues like CPU hotplugging and changing cpusets
> > all at once? The only potential issue I see with that approach would
> > be that you wouldn't be able to use it for inter-process
> > communication; and I have no idea whether this would be good or bad
> > performance-wise.
>
> Firstly, inter-process communication over shared memory is one of my use-cases
> (for lttng-ust's ring buffer).
>
> I'm not convinced that applying constraints on all other threads belonging to
> the current process would be easier or faster than migrating the current thread
> over to the target CPU. I'm unsure how those additional constraints would
> fit with other threads already having their own cpu affinity masks (which
> could generate an empty cpumask by adding an extra constraint).
Hm - is an empty cpumask a problem here? If one task is in the middle
of a critical section for performing maintenance on a percpu data
structure, isn't it a nice design property to exclude concurrent
access from other tasks to that data structure automatically (by
preventing those tasks by running on that CPU)? That way the
(presumably rarely-executed) update path doesn't have to be
rseq-reentrancy-safe.
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Mathieu Desnoyers @ 2020-01-21 21:18 UTC (permalink / raw)
To: Jann Horn
Cc: Peter Zijlstra, Thomas Gleixner, linux-kernel, Joel Fernandes,
Ingo Molnar, Catalin Marinas, Dave Watson, Will Deacon, shuah,
Andi Kleen, linux-kselftest, H. Peter Anvin, Chris Lameter,
Russell King, Michael Kerrisk, Paul, Paul Turner, Boqun Feng,
Josh Triplett, rostedt, Ben Maurer
In-Reply-To: <CAG48ez2Z5CesMfandNK+S32Rrgp_QGQHqQ1Fpd5-YTsCWGfHeg@mail.gmail.com>
----- On Jan 21, 2020, at 3:35 PM, Jann Horn jannh@google.com wrote:
> On Tue, Jan 21, 2020 at 8:47 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> ----- On Jan 21, 2020, at 12:20 PM, Jann Horn jannh@google.com wrote:
>>
>> > On Tue, Jan 21, 2020 at 5:13 PM Mathieu Desnoyers
>> > <mathieu.desnoyers@efficios.com> wrote:
>> >> There is an important use-case which is not possible with the
>> >> "rseq" (Restartable Sequences) system call, which was left as
>> >> future work.
>> >>
>> >> That use-case is to modify user-space per-cpu data structures
>> >> belonging to specific CPUs which may be brought offline and
>> >> online again by CPU hotplug. This can be used by memory
>> >> allocators to migrate free memory pools when CPUs are brought
>> >> offline, or by ring buffer consumers to target specific per-CPU
>> >> buffers, even when CPUs are brought offline.
>> >>
>> >> A few rather complex prior attempts were made to solve this.
>> >> Those were based on in-kernel interpreters (cpu_opv, do_on_cpu).
>> >> That complexity was generally frowned upon, even by their author.
>> >>
>> >> This patch fulfills this use-case in a refreshingly simple way:
>> >> it introduces a "pin_on_cpu" system call, which allows user-space
>> >> threads to pin themselves on a specific CPU (which needs to be
>> >> present in the thread's allowed cpu mask), and then clear this
>> >> pinned state.
>> > [...]
>> >> For instance, this allows implementing this userspace library API
>> >> for incrementing a per-cpu counter for a specific cpu number
>> >> received as parameter:
>> >>
>> >> static inline __attribute__((always_inline))
>> >> int percpu_addv(intptr_t *v, intptr_t count, int cpu)
>> >> {
>> >> int ret;
>> >>
>> >> ret = rseq_addv(v, count, cpu);
>> >> check:
>> >> if (rseq_unlikely(ret)) {
>> >> pin_on_cpu_set(cpu);
>> >> ret = rseq_addv(v, count, percpu_current_cpu());
>> >> pin_on_cpu_clear();
>> >> goto check;
>> >> }
>> >> return 0;
>> >> }
>> >
>> > What does userspace have to do if the set of allowed CPUs switches all
>> > the time? For example, on Android, if you first open Chrome and then
>> > look at its allowed CPUs, Chrome is allowed to use all CPU cores
>> > because it's running in the foreground:
>> >
>> > walleye:/ # ps -AZ | grep 'android.chrome$'
>> > u:r:untrusted_app:s0:c145,c256,c512,c768 u0_a145 7845 805 1474472
>> > 197868 SyS_epoll_wait f09c0194 S com.android.chrome
>> > walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
>> > /proc/7845/status
>> > 3:cpuset:/top-app
>> > Cpus_allowed_list: 0-7
>> >
>> > But if you then switch to the home screen, the application is moved
>> > into a different cgroup, and is restricted to two CPU cores:
>> >
>> > walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
>> > /proc/7845/status
>> > 3:cpuset:/background
>> > Cpus_allowed_list: 0-1
>>
>> Then at that point, pin_on_cpu() would only be allowed to pin on
>> CPUs 0 and 1.
>
> Which means that you can't actually reliably use pin_on_cpu_set() to
> manipulate percpu data structures since you have to call it with the
> assumption that it might randomly fail at any time, right?
Only if the cpu affinity of the thread is being changed concurrently
by another thread which is a possibility in some applications, indeed.
> And then
> userspace needs to code a fallback path that somehow halts all the
> threads with thread-directed signals or something?
The example use of pin_on_cpu() did not include handling of the return
value in that case (-1, errno=EINVAL) for conciseness. But yes, the
application would have to handle this.
It's not so different from error handling which is required when using
sched_setaffinity(), which can fail with -1, errno=EINVAL in the following
scenario:
EINVAL The affinity bit mask mask contains no processors that are cur‐
rently physically on the system and permitted to the thread
according to any restrictions that may be imposed by the
"cpuset" mechanism described in cpuset(7).
> Especially if the task trying to use pin_on_cpu_set() isn't allowed to
> pin to the target CPU, but all the other tasks using the shared data
> structure are allowed to do that. Or if the CPU you want to pin to is
> always removed from your affinity mask immediately before
> pin_on_cpu_set() and added back immediately afterwards.
I am tempted to state that using pin_on_cpu() targeting a disallowed cpu
should be considered a programming error and handled accordingly by the
application.
>
>> > At the same time, I also wonder whether it is a good idea to allow
>> > userspace to stay active on a CPU even after the task has been told to
>> > move to another CPU core - that's probably not exactly a big deal, but
>> > seems suboptimal to me.
>>
>> Do you mean the following scenario for a given task ?
>>
>> 1) set affinity to CPU 0,1,2
>> 2) pin on CPU 2
>> 3) set affinity to CPU 0,1
>>
>> In the patch I propose here, step (3) is forbidden by this added check in
>> __set_cpus_allowed_ptr, which is used by sched_setaffinity(2):
>>
>> + /* Prevent removing the currently pinned CPU from the allowed cpu mask.
>> */
>> + if (is_pinned_task(p) && !cpumask_test_cpu(p->pinned_cpu, new_mask)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>
> How does that interact with things like cgroups? What happens when
> root tries to move a process from the cpuset:/top-app cgroup to the
> cpuset:/background cgroup? Does that also just throw an error code?
Looking at kernel/cgroup/cpuset.c use of set_cpus_allowed_ptr(), either
it:
A) silently ignores the error (3 instances),
or
B) triggers a WARN_ON_ONCE() (1 instance).
So yeah, that cgroup code seems rather optimistic that this operation
should always succeed (?!?)
I can say up front I am not entirely sure what's the best way to proceed
here. Either sched_setaffinity and cgroup can fail if trying to remove a
CPU from affinity mask while the task is pinned, or we let them succeed
and leave the task running on the pinned CPU anyway, or any better idea ?
>
>> > I'm wondering whether it might be possible to rework this mechanism
>> > such that, instead of moving the current task onto a target CPU, it
>> > prevents all *other* threads of the current process from running on
>> > that CPU (either entirely or in user mode). That might be the easiest
>> > way to take care of issues like CPU hotplugging and changing cpusets
>> > all at once? The only potential issue I see with that approach would
>> > be that you wouldn't be able to use it for inter-process
>> > communication; and I have no idea whether this would be good or bad
>> > performance-wise.
>>
>> Firstly, inter-process communication over shared memory is one of my use-cases
>> (for lttng-ust's ring buffer).
>>
>> I'm not convinced that applying constraints on all other threads belonging to
>> the current process would be easier or faster than migrating the current thread
>> over to the target CPU. I'm unsure how those additional constraints would
>> fit with other threads already having their own cpu affinity masks (which
>> could generate an empty cpumask by adding an extra constraint).
>
> Hm - is an empty cpumask a problem here? If one task is in the middle
> of a critical section for performing maintenance on a percpu data
> structure, isn't it a nice design property to exclude concurrent
> access from other tasks to that data structure automatically (by
> preventing those tasks by running on that CPU)? That way the
> (presumably rarely-executed) update path doesn't have to be
> rseq-reentrancy-safe.
Given we already have rseq, simply using it to protect against other
threads trying to touch the same per-cpu data seems rather more lightweight
than to try to exclude all other threads from that CPU for a possibly
unbounded amount of time.
Allowing a completely empty cpumask could effectively allow those
critical sections to prevent execution of possibly higher priority
threads on the system, which ends up being the definition of a priority
inversion, which I'd like to avoid.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Christopher Lameter @ 2020-01-21 21:44 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Jann Horn, Peter Zijlstra, Thomas Gleixner, linux-kernel,
Joel Fernandes, Ingo Molnar, Catalin Marinas, Dave Watson,
Will Deacon, shuah, Andi Kleen, linux-kselftest, H. Peter Anvin,
Russell King, Michael Kerrisk, Paul, Paul Turner, Boqun Feng,
Josh Triplett, rostedt, Ben Maurer
In-Reply-To: <2049164886.596497.1579641536619.JavaMail.zimbra-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org>
These scenarios are all pretty complex and will be difficult to understand
for the user of these APIs.
I think the easiest solution (and most comprehensible) is for the user
space process that does per cpu operations to get some sort of signal. If
its not able to handle that then terminate it. The code makes a basic
assumption after all that the process is running on a specific cpu. If
this is no longer the case then its better to abort if the process cannot
handle moving to a different processor.
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Mathieu Desnoyers @ 2020-01-22 1:11 UTC (permalink / raw)
To: Chris Lameter
Cc: Jann Horn, Peter Zijlstra, Thomas Gleixner, linux-kernel,
Joel Fernandes, Ingo Molnar, Catalin Marinas, Dave Watson,
Will Deacon, shuah, Andi Kleen, linux-kselftest, H. Peter Anvin,
Russell King, Michael Kerrisk, Paul, Paul Turner, Boqun Feng,
Josh Triplett, rostedt, Ben Maurer
In-Reply-To: <alpine.DEB.2.21.2001212141590.1231-FiTwH0KJEoYIjDr1QQGPvw@public.gmane.org>
----- On Jan 21, 2020, at 4:44 PM, Chris Lameter cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org wrote:
> These scenarios are all pretty complex and will be difficult to understand
> for the user of these APIs.
>
> I think the easiest solution (and most comprehensible) is for the user
> space process that does per cpu operations to get some sort of signal. If
> its not able to handle that then terminate it. The code makes a basic
> assumption after all that the process is running on a specific cpu. If
> this is no longer the case then its better to abort if the process cannot
> handle moving to a different processor.
The point of pin_on_cpu() is to allow threads to access per-cpu data
structures belonging to a given CPU even if they cannot run on that
CPU (because it is offline).
I am not sure what scenario your signal delivery proposal aims to cover.
Just to try to put this into the context of a specific scenario to see
if I understand your point, is the following what you have in mind ?
1. Thread A issues pin_on_cpu(5),
2. Thread B issues sched_setaffinity removing cpu 5 from thread A's
affinity mask,
3. Noticing that it would generate an invalid combination, rather than
failing sched_setaffinity, it would send a SIGSEGV (or other) signal
to thread A.
Or so you have something entirely different in mind ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Jann Horn @ 2020-01-22 8:23 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Thomas Gleixner, linux-kernel, Joel Fernandes,
Ingo Molnar, Catalin Marinas, Dave Watson, Will Deacon, shuah,
Andi Kleen, linux-kselftest, H. Peter Anvin, Chris Lameter,
Russell King, Michael Kerrisk, Paul, Paul Turner, Boqun Feng,
Josh Triplett, rostedt, Ben Maurer
In-Reply-To: <2049164886.596497.1579641536619.JavaMail.zimbra@efficios.com>
On Tue, Jan 21, 2020 at 10:18 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> ----- On Jan 21, 2020, at 3:35 PM, Jann Horn jannh@google.com wrote:
>
> > On Tue, Jan 21, 2020 at 8:47 PM Mathieu Desnoyers
> > <mathieu.desnoyers@efficios.com> wrote:
> >>
> >> ----- On Jan 21, 2020, at 12:20 PM, Jann Horn jannh@google.com wrote:
> >>
> >> > On Tue, Jan 21, 2020 at 5:13 PM Mathieu Desnoyers
> >> > <mathieu.desnoyers@efficios.com> wrote:
> >> >> There is an important use-case which is not possible with the
> >> >> "rseq" (Restartable Sequences) system call, which was left as
> >> >> future work.
> >> >>
> >> >> That use-case is to modify user-space per-cpu data structures
> >> >> belonging to specific CPUs which may be brought offline and
> >> >> online again by CPU hotplug. This can be used by memory
> >> >> allocators to migrate free memory pools when CPUs are brought
> >> >> offline, or by ring buffer consumers to target specific per-CPU
> >> >> buffers, even when CPUs are brought offline.
> >> >>
> >> >> A few rather complex prior attempts were made to solve this.
> >> >> Those were based on in-kernel interpreters (cpu_opv, do_on_cpu).
> >> >> That complexity was generally frowned upon, even by their author.
> >> >>
> >> >> This patch fulfills this use-case in a refreshingly simple way:
> >> >> it introduces a "pin_on_cpu" system call, which allows user-space
> >> >> threads to pin themselves on a specific CPU (which needs to be
> >> >> present in the thread's allowed cpu mask), and then clear this
> >> >> pinned state.
> >> > [...]
> >> >> For instance, this allows implementing this userspace library API
> >> >> for incrementing a per-cpu counter for a specific cpu number
> >> >> received as parameter:
> >> >>
> >> >> static inline __attribute__((always_inline))
> >> >> int percpu_addv(intptr_t *v, intptr_t count, int cpu)
> >> >> {
> >> >> int ret;
> >> >>
> >> >> ret = rseq_addv(v, count, cpu);
> >> >> check:
> >> >> if (rseq_unlikely(ret)) {
> >> >> pin_on_cpu_set(cpu);
> >> >> ret = rseq_addv(v, count, percpu_current_cpu());
> >> >> pin_on_cpu_clear();
> >> >> goto check;
> >> >> }
> >> >> return 0;
> >> >> }
> >> >
> >> > What does userspace have to do if the set of allowed CPUs switches all
> >> > the time? For example, on Android, if you first open Chrome and then
> >> > look at its allowed CPUs, Chrome is allowed to use all CPU cores
> >> > because it's running in the foreground:
> >> >
> >> > walleye:/ # ps -AZ | grep 'android.chrome$'
> >> > u:r:untrusted_app:s0:c145,c256,c512,c768 u0_a145 7845 805 1474472
> >> > 197868 SyS_epoll_wait f09c0194 S com.android.chrome
> >> > walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
> >> > /proc/7845/status
> >> > 3:cpuset:/top-app
> >> > Cpus_allowed_list: 0-7
> >> >
> >> > But if you then switch to the home screen, the application is moved
> >> > into a different cgroup, and is restricted to two CPU cores:
> >> >
> >> > walleye:/ # grep cpuset /proc/7845/cgroup; grep Cpus_allowed_list
> >> > /proc/7845/status
> >> > 3:cpuset:/background
> >> > Cpus_allowed_list: 0-1
> >>
> >> Then at that point, pin_on_cpu() would only be allowed to pin on
> >> CPUs 0 and 1.
> >
> > Which means that you can't actually reliably use pin_on_cpu_set() to
> > manipulate percpu data structures since you have to call it with the
> > assumption that it might randomly fail at any time, right?
>
> Only if the cpu affinity of the thread is being changed concurrently
> by another thread which is a possibility in some applications, indeed.
Not just some applications, but also some environments, right? See the
Android example - the set of permitted CPUs is changed not by the
application itself, but by a management process that uses cgroup
modifications to indirectly change the set of permitted CPUs. I
wouldn't be surprised if the same could happen in e.g. container
environments.
> > And then
> > userspace needs to code a fallback path that somehow halts all the
> > threads with thread-directed signals or something?
>
> The example use of pin_on_cpu() did not include handling of the return
> value in that case (-1, errno=EINVAL) for conciseness. But yes, the
> application would have to handle this.
>
> It's not so different from error handling which is required when using
> sched_setaffinity(), which can fail with -1, errno=EINVAL in the following
> scenario:
>
> EINVAL The affinity bit mask mask contains no processors that are cur‐
> rently physically on the system and permitted to the thread
> according to any restrictions that may be imposed by the
> "cpuset" mechanism described in cpuset(7).
Except that sched_setaffinity() is normally just a performance
optimization, right? Whereas pin_to_cpu() is more of a correctness
thing?
> > Especially if the task trying to use pin_on_cpu_set() isn't allowed to
> > pin to the target CPU, but all the other tasks using the shared data
> > structure are allowed to do that. Or if the CPU you want to pin to is
> > always removed from your affinity mask immediately before
> > pin_on_cpu_set() and added back immediately afterwards.
>
> I am tempted to state that using pin_on_cpu() targeting a disallowed cpu
> should be considered a programming error and handled accordingly by the
> application.
How can it be a programming error if that situation can be triggered
by legitimate external modifications to CPU affinity?
[...]
> >> > I'm wondering whether it might be possible to rework this mechanism
> >> > such that, instead of moving the current task onto a target CPU, it
> >> > prevents all *other* threads of the current process from running on
> >> > that CPU (either entirely or in user mode). That might be the easiest
> >> > way to take care of issues like CPU hotplugging and changing cpusets
> >> > all at once? The only potential issue I see with that approach would
> >> > be that you wouldn't be able to use it for inter-process
> >> > communication; and I have no idea whether this would be good or bad
> >> > performance-wise.
> >>
> >> Firstly, inter-process communication over shared memory is one of my use-cases
> >> (for lttng-ust's ring buffer).
> >>
> >> I'm not convinced that applying constraints on all other threads belonging to
> >> the current process would be easier or faster than migrating the current thread
> >> over to the target CPU. I'm unsure how those additional constraints would
> >> fit with other threads already having their own cpu affinity masks (which
> >> could generate an empty cpumask by adding an extra constraint).
> >
> > Hm - is an empty cpumask a problem here? If one task is in the middle
> > of a critical section for performing maintenance on a percpu data
> > structure, isn't it a nice design property to exclude concurrent
> > access from other tasks to that data structure automatically (by
> > preventing those tasks by running on that CPU)? That way the
> > (presumably rarely-executed) update path doesn't have to be
> > rseq-reentrancy-safe.
>
> Given we already have rseq, simply using it to protect against other
> threads trying to touch the same per-cpu data seems rather more lightweight
> than to try to exclude all other threads from that CPU for a possibly
> unbounded amount of time.
That only works if the cpu-targeted maintenance operation is something
that can be implemented in rseq, right? I was thinking that it might
be nice to avoid that limitation - but I don't know much about the
kinds of data structures that one might want to build on top of rseq,
so maybe that's a silly idea.
> Allowing a completely empty cpumask could effectively allow those
> critical sections to prevent execution of possibly higher priority
> threads on the system, which ends up being the definition of a priority
> inversion, which I'd like to avoid.
Linux does have the infrastructure for RT futexes, right? Maybe that
could be useful here.
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Michal Hocko @ 2020-01-22 8:28 UTC (permalink / raw)
To: Minchan Kim
Cc: sspatil-hpIqsD4AKlfQT0dZR+AlfA, kirill-oKw7cIdHH8eLwutG50LtGA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, linux-api-u79uwXL29TY76Z2rM5mHXA,
oleksandr-H+wXaHxf7aLQT0dZR+AlfA, surenb-hpIqsD4AKlfQT0dZR+AlfA,
timmurray-hpIqsD4AKlfQT0dZR+AlfA, dancol-hpIqsD4AKlfQT0dZR+AlfA,
sonnyrao-hpIqsD4AKlfQT0dZR+AlfA, bgeffon-hpIqsD4AKlfQT0dZR+AlfA,
hannes-druUgvl0LCNAfugRpC6u6w, shakeelb-hpIqsD4AKlfQT0dZR+AlfA,
joaodias-hpIqsD4AKlfQT0dZR+AlfA, ktkhai-5HdwGun5lf+gSpxsJD1C4w,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200121183212.GF140922-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On Tue 21-01-20 10:32:12, Minchan Kim wrote:
> On Mon, Jan 20, 2020 at 08:58:25AM +0100, Michal Hocko wrote:
[...]
> > The interface really has to be robust to future potential usecases.
>
> I do understand your concern but for me, it's chicken and egg problem.
> We usually do best effort to make something perfect as far as possible
> but we also don't do over-engineering without real usecase from the
> beginning.
>
> I already told you how we could synchronize among processes and potential
> way to be extended Daniel suggested(That's why current API has extra field
> for the cookie) even though we don't need it right now.
If you can synchronize with the target task then you do not need a
remote interface. Just use ptrace and you are done with it.
> If you want to suggest the other way, please explain why your idea is
> better and why we need it at this moment.
I believe I have explained my concerns and why they matter. All you are
saying is that you do not care because your particular usecase doesn't
care. And that is a first signal of a future disaster when we end up
with a broken and unfixable interface we have to maintain for ever.
I will not go as far as to nack this but you should seriously think
about other potential usecases and how they would work and what we are
going to do when a first non-cooperative userspace memory management
usecase materializes.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: SeongJae Park @ 2020-01-22 9:36 UTC (permalink / raw)
To: Michal Hocko
Cc: Minchan Kim, sspatil-hpIqsD4AKlfQT0dZR+AlfA,
kirill-oKw7cIdHH8eLwutG50LtGA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, linux-api-u79uwXL29TY76Z2rM5mHXA,
oleksandr-H+wXaHxf7aLQT0dZR+AlfA, surenb-hpIqsD4AKlfQT0dZR+AlfA,
timmurray-hpIqsD4AKlfQT0dZR+AlfA, dancol-hpIqsD4AKlfQT0dZR+AlfA,
sonnyrao-hpIqsD4AKlfQT0dZR+AlfA, bgeffon-hpIqsD4AKlfQT0dZR+AlfA,
hannes-druUgvl0LCNAfugRpC6u6w, shakeelb-hpIqsD4AKlfQT0dZR+AlfA,
joaodias-hpIqsD4AKlfQT0dZR+AlfA, ktkhai-5HdwGun5lf+gSpxsJD1C4w,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200122082853.GS29276-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org>
On Wed, 22 Jan 2020 09:28:53 +0100 Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue 21-01-20 10:32:12, Minchan Kim wrote:
> > On Mon, Jan 20, 2020 at 08:58:25AM +0100, Michal Hocko wrote:
> [...]
> > > The interface really has to be robust to future potential usecases.
> >
> > I do understand your concern but for me, it's chicken and egg problem.
> > We usually do best effort to make something perfect as far as possible
> > but we also don't do over-engineering without real usecase from the
> > beginning.
> >
> > I already told you how we could synchronize among processes and potential
> > way to be extended Daniel suggested(That's why current API has extra field
> > for the cookie) even though we don't need it right now.
>
> If you can synchronize with the target task then you do not need a
> remote interface. Just use ptrace and you are done with it.
>
> > If you want to suggest the other way, please explain why your idea is
> > better and why we need it at this moment.
>
> I believe I have explained my concerns and why they matter. All you are
> saying is that you do not care because your particular usecase doesn't
> care. And that is a first signal of a future disaster when we end up
> with a broken and unfixable interface we have to maintain for ever.
>
> I will not go as far as to nack this but you should seriously think
> about other potential usecases and how they would work and what we are
> going to do when a first non-cooperative userspace memory management
> usecase materializes.
Beside of the specific environment of Android, I think there are many ways to
know the address space layout and access patterns of other processes. The
idle_page_tracking might be an example that widelay available.
Of course, the information might not strictly correct due to the timing issue,
but could be still worth to be used under some extreme situations, such as
memory pressure or fragmentation. For the same reason, ptrace() would not be
sufficient, as we have no perfect control, but only some level of control that
would be useful under specific situations.
I assume the users of this systemcall would understand the tradeoff and make
decisions. Also, as the users already have the right to do the tradeoff, I
think it's fair. In other words, I think the caller has both the power and the
responsibility to deal with the time-to-check-time-to-react problem.
Nonetheless, I also agree this is important concern and the patch would be
better if it adds more detailed documentation regarding this issue.
If I'm missing some points or you have different opinions, please let me know.
Thanks,
SeongJae Park
> --
> Michal Hocko
> SUSE Labs
>
^ permalink raw reply
* Re: Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Michal Hocko @ 2020-01-22 10:02 UTC (permalink / raw)
To: SeongJae Park
Cc: Minchan Kim, sspatil-hpIqsD4AKlfQT0dZR+AlfA,
kirill-oKw7cIdHH8eLwutG50LtGA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, linux-api-u79uwXL29TY76Z2rM5mHXA,
oleksandr-H+wXaHxf7aLQT0dZR+AlfA, surenb-hpIqsD4AKlfQT0dZR+AlfA,
timmurray-hpIqsD4AKlfQT0dZR+AlfA, dancol-hpIqsD4AKlfQT0dZR+AlfA,
sonnyrao-hpIqsD4AKlfQT0dZR+AlfA, bgeffon-hpIqsD4AKlfQT0dZR+AlfA,
hannes-druUgvl0LCNAfugRpC6u6w, shakeelb-hpIqsD4AKlfQT0dZR+AlfA,
joaodias-hpIqsD4AKlfQT0dZR+AlfA, ktkhai-5HdwGun5lf+gSpxsJD1C4w,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200122093624.14799-1-sjpark-vV1OtcyAfmbQT0dZR+AlfA@public.gmane.org>
On Wed 22-01-20 10:36:24, SeongJae Park wrote:
> On Wed, 22 Jan 2020 09:28:53 +0100 Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> > On Tue 21-01-20 10:32:12, Minchan Kim wrote:
> > > On Mon, Jan 20, 2020 at 08:58:25AM +0100, Michal Hocko wrote:
> > [...]
> > > > The interface really has to be robust to future potential usecases.
> > >
> > > I do understand your concern but for me, it's chicken and egg problem.
> > > We usually do best effort to make something perfect as far as possible
> > > but we also don't do over-engineering without real usecase from the
> > > beginning.
> > >
> > > I already told you how we could synchronize among processes and potential
> > > way to be extended Daniel suggested(That's why current API has extra field
> > > for the cookie) even though we don't need it right now.
> >
> > If you can synchronize with the target task then you do not need a
> > remote interface. Just use ptrace and you are done with it.
> >
> > > If you want to suggest the other way, please explain why your idea is
> > > better and why we need it at this moment.
> >
> > I believe I have explained my concerns and why they matter. All you are
> > saying is that you do not care because your particular usecase doesn't
> > care. And that is a first signal of a future disaster when we end up
> > with a broken and unfixable interface we have to maintain for ever.
> >
> > I will not go as far as to nack this but you should seriously think
> > about other potential usecases and how they would work and what we are
> > going to do when a first non-cooperative userspace memory management
> > usecase materializes.
>
> Beside of the specific environment of Android, I think there are many ways to
> know the address space layout and access patterns of other processes. The
> idle_page_tracking might be an example that widelay available.
>
> Of course, the information might not strictly correct due to the timing issue,
> but could be still worth to be used under some extreme situations, such as
> memory pressure or fragmentation. For the same reason, ptrace() would not be
> sufficient, as we have no perfect control, but only some level of control that
> would be useful under specific situations.
I am not sure I see your point. I am talking about races where a remote
task is operating on a completely different object because the one it
checked for has been unmapped and new one mapped over it. Memory
pressure or a fragmentation will not change the object itself. Sure the
memory might be reclaimed but that should be completely OK unless I am
missing something.
> I assume the users of this systemcall would understand the tradeoff and make
> decisions.
I disagree. My experience tells me that users tend to squeeze the
maximum and beyond and hope they get what they want.
> Also, as the users already have the right to do the tradeoff, I
> think it's fair. In other words, I think the caller has both the power and the
> responsibility to deal with the time-to-check-time-to-react problem.
>
> Nonetheless, I also agree this is important concern and the patch would be
> better if it adds more detailed documentation regarding this issue.
If there is _really_ a strong consensus that the racy interface is
reasonable then it absolutely has to be described with a clearly state
that those races might result in hard to predict behavior unless all
tasks sharing the address space are blocked between the check and the
madvise call.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Oleksandr Natalenko @ 2020-01-22 10:44 UTC (permalink / raw)
To: Minchan Kim
Cc: Kirill A. Shutemov, Michal Hocko, Andrew Morton, LKML, linux-mm,
linux-api, Suren Baghdasaryan, Tim Murray, Daniel Colascione,
Sandeep Patil, Sonny Rao, Brian Geffon, Johannes Weiner,
Shakeel Butt, John Dias, ktkhai, christian.brauner, sjpark
In-Reply-To: <20200121181113.GE140922@google.com>
Hello.
On Tue, Jan 21, 2020 at 10:11:13AM -0800, Minchan Kim wrote:
> > > I also think people don't want to give an KSM hint to non-mergeable area.
> >
> > And how the manager knows which data is mergable?
>
> Oleksandr, could you say your thought why you need address range based
> API?
It seems I've overlooked an important piece of this submission: one
cannot apply the hint to all the anonymous mapping regardless of address
range. For KSM I'd rather either have a possibility to hint all the
anonymous mappings, or, as it was suggested previously, be able to iterate
over existing mappings using some (fd-based?) API.
--
Best regards,
Oleksandr Natalenko (post-factum)
Senior Software Maintenance Engineer
^ permalink raw reply
* Re: Re: Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: SeongJae Park @ 2020-01-22 13:28 UTC (permalink / raw)
To: Michal Hocko
Cc: SeongJae Park, Minchan Kim, sspatil-hpIqsD4AKlfQT0dZR+AlfA,
kirill-oKw7cIdHH8eLwutG50LtGA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, linux-api-u79uwXL29TY76Z2rM5mHXA,
oleksandr-H+wXaHxf7aLQT0dZR+AlfA, surenb-hpIqsD4AKlfQT0dZR+AlfA,
timmurray-hpIqsD4AKlfQT0dZR+AlfA, dancol-hpIqsD4AKlfQT0dZR+AlfA,
sonnyrao-hpIqsD4AKlfQT0dZR+AlfA, bgeffon-hpIqsD4AKlfQT0dZR+AlfA,
hannes-druUgvl0LCNAfugRpC6u6w, shakeelb-hpIqsD4AKlfQT0dZR+AlfA,
joaodias-hpIqsD4AKlfQT0dZR+AlfA, ktkhai-5HdwGun5lf+gSpxsJD1C4w,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200122100233.GT29276-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org>
On Wed, 22 Jan 2020 11:02:33 +0100 Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed 22-01-20 10:36:24, SeongJae Park wrote:
> > On Wed, 22 Jan 2020 09:28:53 +0100 Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > > On Tue 21-01-20 10:32:12, Minchan Kim wrote:
> > > > On Mon, Jan 20, 2020 at 08:58:25AM +0100, Michal Hocko wrote:
> > > [...]
> > > > > The interface really has to be robust to future potential usecases.
> > > >
> > > > I do understand your concern but for me, it's chicken and egg problem.
> > > > We usually do best effort to make something perfect as far as possible
> > > > but we also don't do over-engineering without real usecase from the
> > > > beginning.
> > > >
> > > > I already told you how we could synchronize among processes and potential
> > > > way to be extended Daniel suggested(That's why current API has extra field
> > > > for the cookie) even though we don't need it right now.
> > >
> > > If you can synchronize with the target task then you do not need a
> > > remote interface. Just use ptrace and you are done with it.
> > >
> > > > If you want to suggest the other way, please explain why your idea is
> > > > better and why we need it at this moment.
> > >
> > > I believe I have explained my concerns and why they matter. All you are
> > > saying is that you do not care because your particular usecase doesn't
> > > care. And that is a first signal of a future disaster when we end up
> > > with a broken and unfixable interface we have to maintain for ever.
> > >
> > > I will not go as far as to nack this but you should seriously think
> > > about other potential usecases and how they would work and what we are
> > > going to do when a first non-cooperative userspace memory management
> > > usecase materializes.
> >
> > Beside of the specific environment of Android, I think there are many ways to
> > know the address space layout and access patterns of other processes. The
> > idle_page_tracking might be an example that widelay available.
> >
> > Of course, the information might not strictly correct due to the timing issue,
> > but could be still worth to be used under some extreme situations, such as
> > memory pressure or fragmentation. For the same reason, ptrace() would not be
> > sufficient, as we have no perfect control, but only some level of control that
> > would be useful under specific situations.
>
> I am not sure I see your point. I am talking about races where a remote
> task is operating on a completely different object because the one it
> checked for has been unmapped and new one mapped over it. Memory
> pressure or a fragmentation will not change the object itself. Sure the
> memory might be reclaimed but that should be completely OK unless I am
> missing something.
Thank you for pointing out your concerns in more detail. I was assuming a case
using MADV_PAGEOUT or MADV_HUGEPAGE like hints under access frequency
monitoring for better performance under memory pressure or fragmentation,
respectively. Under the race, such hints might incur some performance
degradation, but no critical problem such as SEGV. I previously implemented
such optimization for research purpose and it was worthy. Nonetheless, it was
just a research purpose hack.
MADV_FREE like hints might result in SEGV and thus of course should be avoided.
But, to my perspective, the 4 hints madvise_process() is currently supporting
(COLD, PAGEOUT, MERGEABLE, UNMERGEABLE) are not too risky even under the race.
That's why I said the incorrect information could be worth to be used under
some extreme situations.
>
> > I assume the users of this systemcall would understand the tradeoff and make
> > decisions.
>
> I disagree. My experience tells me that users tend to squeeze the
> maximum and beyond and hope they get what they want.
>
> > Also, as the users already have the right to do the tradeoff, I
> > think it's fair. In other words, I think the caller has both the power and the
> > responsibility to deal with the time-to-check-time-to-react problem.
> >
> > Nonetheless, I also agree this is important concern and the patch would be
> > better if it adds more detailed documentation regarding this issue.
>
> If there is _really_ a strong consensus that the racy interface is
> reasonable then it absolutely has to be described with a clearly state
> that those races might result in hard to predict behavior unless all
> tasks sharing the address space are blocked between the check and the
> madvise call.
So, it's still too risky to simply believe users to do the things well on their
responsibility, but a strong real consensus on needs and clear description
might justify this. I also agreed.
Thanks,
SeongJae Park
> --
> Michal Hocko
> SUSE Labs
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox