[Devel] [PATCH RHEL9 COMMIT] ve/cgroup: Move release_agent from system_wq to per-ve workqueues

Konstantin Khorenko khorenko at virtuozzo.com
Wed Oct 20 11:43:56 MSK 2021


The commit is pushed to "branch-rh9-5.14.vz9.1.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh9-5.14.0-4.vz9.10.12
------>
commit 94b2fa5aa580f377e3ccb9014d04988940bc6a58
Author: Valeriy Vdovin <valeriy.vdovin at virtuozzo.com>
Date:   Wed Oct 20 11:43:55 2021 +0300

    ve/cgroup: Move release_agent from system_wq to per-ve workqueues
    
    Each VE should execute release agent notifications within it's own
    workqueue. This way we achieve a more fine-grained control over
    release_agent work flushing at VE destruction.
    
    Short history of the patch:
    
    https://jira.sw.ru/browse/PSBM-83887
    Signed-off-by: Valeriy Vdovin <valeriy.vdovin at virtuozzo.com>
    
    Reviewed-by: Kirill Tkhai <ktkhai at virtuozzo.com>
    (cherry picked from vz7 commit 9fbfb5b4cfb87ba7c9dd63eec5e5e27946a38d3c)
    https://jira.sw.ru/browse/PSBM-108270
    (cherry picked from vz8 commit 0eda8e4d0a9f6660a72f7584e8504a363c25a392)
    +++
    cgroup/ve: Do not run release_agent on non-running ve
    +++
    cgroup/ve: Move ve workqueue stop to ve_stop_ns()
    (cherry picked from vz8 commit 1481d130afa32eecffe28b34fd00b57e11d2666f)
    
    vz9 changes:
    - merge fixup hunks from 1481d130afa3 related to workqueue locking
    - do less code movements around cgroup->release_list and
      ve->release_list init
    - add comment to ve_rm_from_release_list about why we always have the
      right ve->release_list_lock there
    - rework cgroup1_release_agent, on vz8 release-agent patches are badly
      broken with conterintuitive reordering, e.g. cgroup_path_ve_relative
      used in this patch was defined in later patches...
    - fix missing cgroup->release_list initialization
    
    https://jira.sw.ru/browse/PSBM-134002
    Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
    
    Reviewed-by: Alexander Mikhalitsyn <alexander.mikhalitsyn at virtuozzo.com>
    Reviewed-by: Kirill Tkhai <ktkhai at virtuozzo.com>
---
 include/linux/cgroup-defs.h     |  8 +++--
 include/linux/cgroup.h          |  2 ++
 include/linux/ve.h              | 10 ++++++
 kernel/cgroup/cgroup-internal.h |  1 +
 kernel/cgroup/cgroup-v1.c       | 78 ++++++++++++++++++++++++++++-------------
 kernel/cgroup/cgroup.c          |  4 +--
 kernel/ve/ve.c                  | 73 ++++++++++++++++++++++++++++++++++++++
 7 files changed, 148 insertions(+), 28 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 315c399c5d3b..2035909f7a0a 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -482,8 +482,12 @@ struct cgroup {
 	/* used to wait for offlining of csses */
 	wait_queue_head_t offline_waitq;
 
-	/* used to schedule release agent */
-	struct work_struct release_agent_work;
+	/*
+	 * Linked list running through all cgroups that can
+	 * potentially be reaped by the release agent. Protected by
+	 * release_list_lock
+	 */
+	struct list_head release_list;
 
 	/* used to track pressure stalls */
 	struct psi_group psi;
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index f4d9b0f44a43..d2586a8397f3 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -886,6 +886,8 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags,
 int cgroup_path_ns(struct cgroup *cgrp, char *buf, size_t buflen,
 		   struct cgroup_namespace *ns);
 
+void cgroup1_release_agent(struct work_struct *work);
+
 #ifdef CONFIG_VE
 extern int cgroup_mark_ve_roots(struct ve_struct *ve);
 void cgroup_unmark_ve_roots(struct ve_struct *ve);
diff --git a/include/linux/ve.h b/include/linux/ve.h
index eb223c9db6e5..25dee1b4d4e9 100644
--- a/include/linux/ve.h
+++ b/include/linux/ve.h
@@ -106,8 +106,16 @@ struct ve_struct {
 	unsigned long		aio_nr;
 	unsigned long		aio_max_nr;
 #endif
+	/*
+	 * cgroups, that want to notify about becoming
+	 * empty, are linked to this release_list.
+	 */
+	struct list_head	release_list;
+	spinlock_t		release_list_lock;
+
 	/* Should take rcu_read_lock and check ve->is_running before queue */
 	struct workqueue_struct	*wq;
+	struct work_struct	release_agent_work;
 
 	struct vfsmount		*devtmpfs_mnt;
 };
@@ -133,6 +141,8 @@ extern int nr_ve;
 extern unsigned int sysctl_ve_mount_nr;
 
 #ifdef CONFIG_VE
+void ve_add_to_release_list(struct cgroup *cgrp);
+void ve_rm_from_release_list(struct cgroup *cgrp);
 extern struct ve_struct *get_ve(struct ve_struct *ve);
 extern void put_ve(struct ve_struct *ve);
 
diff --git a/kernel/cgroup/cgroup-internal.h b/kernel/cgroup/cgroup-internal.h
index bfbeabc17a9d..81bf97510198 100644
--- a/kernel/cgroup/cgroup-internal.h
+++ b/kernel/cgroup/cgroup-internal.h
@@ -151,6 +151,7 @@ extern spinlock_t css_set_lock;
 extern struct cgroup_subsys *cgroup_subsys[];
 extern struct list_head cgroup_roots;
 extern struct file_system_type cgroup_fs_type;
+struct cgroup *cgroup_get_local_root(struct cgroup *cgrp);
 
 /* iterate across the hierarchies */
 #define for_each_root(root)						\
diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 494b4bdd3265..5b3e8e79cc32 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -774,7 +774,7 @@ void cgroup1_check_for_release(struct cgroup *cgrp)
 {
 	if (notify_on_release(cgrp) && !cgroup_is_populated(cgrp) &&
 	    !css_has_online_children(&cgrp->self) && !cgroup_is_dead(cgrp))
-		schedule_work(&cgrp->release_agent_work);
+		ve_add_to_release_list(cgrp);
 }
 
 /*
@@ -802,42 +802,72 @@ void cgroup1_check_for_release(struct cgroup *cgrp)
  */
 void cgroup1_release_agent(struct work_struct *work)
 {
-	struct cgroup *cgrp =
-		container_of(work, struct cgroup, release_agent_work);
+	struct ve_struct *ve =
+		container_of(work, struct ve_struct, release_agent_work);
+	/*
+	 * Release agent work can be queued only to running ve (see
+	 * ve_add_to_release_list) and ve waits for all queued works to finish
+	 * (see ve_workqueue_stop) before stopping, so we can safely access
+	 * ve_ns->cgroup_ns here without rcu_read_lock - it won't disappear
+	 * under us.
+	 */
+	struct cgroup_namespace *ve_cgroup_ns =
+		rcu_dereference_protected(ve->ve_ns, 1)->cgroup_ns;
 	char *pathbuf, *agentbuf;
 	char *argv[3], *envp[3];
+	unsigned long flags;
 	int ret;
 
-	/* snoop agent path and exit early if empty */
-	if (!cgrp->root->release_agent_path[0])
-		return;
-
 	/* prepare argument buffers */
 	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
 	agentbuf = kmalloc(PATH_MAX, GFP_KERNEL);
 	if (!pathbuf || !agentbuf)
 		goto out_free;
 
-	spin_lock(&release_agent_path_lock);
-	strlcpy(agentbuf, cgrp->root->release_agent_path, PATH_MAX);
-	spin_unlock(&release_agent_path_lock);
-	if (!agentbuf[0])
-		goto out_free;
+	spin_lock_irqsave(&ve->release_list_lock, flags);
+	while (!list_empty(&ve->release_list)) {
+		struct cgroup *cgrp;
 
-	ret = cgroup_path_ns(cgrp, pathbuf, PATH_MAX, &init_cgroup_ns);
-	if (ret < 0 || ret >= PATH_MAX)
-		goto out_free;
-
-	argv[0] = agentbuf;
-	argv[1] = pathbuf;
-	argv[2] = NULL;
+		cgrp = list_entry(ve->release_list.next,
+				  struct cgroup,
+				  release_list);
+		list_del_init(&cgrp->release_list);
+		spin_unlock_irqrestore(&ve->release_list_lock, flags);
 
-	/* minimal command environment */
-	envp[0] = "HOME=/";
-	envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
-	envp[2] = NULL;
+		/* snoop agent path and exit early if empty */
+		if (!cgrp->root->release_agent_path[0])
+			goto continue_locked;
 
-	call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
+		spin_lock(&release_agent_path_lock);
+		strlcpy(agentbuf, cgrp->root->release_agent_path, PATH_MAX);
+		spin_unlock(&release_agent_path_lock);
+		if (!agentbuf[0])
+			goto continue_locked;
+
+		ret = cgroup_path_ns(cgrp, pathbuf, PATH_MAX, ve_cgroup_ns);
+		if (ret < 0 || ret >= PATH_MAX)
+			goto continue_locked;
+
+		argv[0] = agentbuf;
+		argv[1] = pathbuf;
+		argv[2] = NULL;
+
+		/* minimal command environment */
+		envp[0] = "HOME=/";
+		envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
+		envp[2] = NULL;
+
+		ret = call_usermodehelper_ve(ve, argv[0], argv, envp,
+					     UMH_WAIT_EXEC);
+
+		if (ret < 0 && ve == &ve0)
+			pr_warn_ratelimited("cgroup1_release_agent "
+					    "%s %s failed: %d\n",
+					    agentbuf, pathbuf, ret);
+continue_locked:
+		spin_lock_irqsave(&ve->release_list_lock, flags);
+	}
+	spin_unlock_irqrestore(&ve->release_list_lock, flags);
 out_free:
 	kfree(agentbuf);
 	kfree(pathbuf);
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index ff9f5b9dc219..c0c92a4cf791 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -2167,6 +2167,7 @@ static void init_cgroup_housekeeping(struct cgroup *cgrp)
 	INIT_LIST_HEAD(&cgrp->self.children);
 	INIT_LIST_HEAD(&cgrp->cset_links);
 	INIT_LIST_HEAD(&cgrp->pidlists);
+	INIT_LIST_HEAD(&cgrp->release_list);
 	mutex_init(&cgrp->pidlist_mutex);
 	cgrp->self.cgroup = cgrp;
 	cgrp->self.flags |= CSS_ONLINE;
@@ -2180,7 +2181,6 @@ static void init_cgroup_housekeeping(struct cgroup *cgrp)
 		INIT_LIST_HEAD(&cgrp->e_csets[ssid]);
 
 	init_waitqueue_head(&cgrp->offline_waitq);
-	INIT_WORK(&cgrp->release_agent_work, cgroup1_release_agent);
 }
 
 void init_cgroup_root(struct cgroup_fs_context *ctx)
@@ -5327,7 +5327,6 @@ static void css_free_rwork_fn(struct work_struct *work)
 		/* cgroup free path */
 		atomic_dec(&cgrp->root->nr_cgrps);
 		cgroup1_pidlist_destroy_all(cgrp);
-		cancel_work_sync(&cgrp->release_agent_work);
 
 		if (cgroup_parent(cgrp)) {
 			/*
@@ -5879,6 +5878,7 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)
 	 * the migration path.
 	 */
 	cgrp->self.flags &= ~CSS_ONLINE;
+	ve_rm_from_release_list(cgrp);
 
 	spin_lock_irq(&css_set_lock);
 	list_for_each_entry(link, &cgrp->cset_links, cset_link)
diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
index d8fd86568533..9c593e93a600 100644
--- a/kernel/ve/ve.c
+++ b/kernel/ve/ve.c
@@ -74,6 +74,11 @@ struct ve_struct ve0 = {
 	.meminfo_val		= VE_MEMINFO_SYSTEM,
 	.vdso_64		= (struct vdso_image*)&vdso_image_64,
 	.vdso_32		= (struct vdso_image*)&vdso_image_32,
+	.release_list_lock	= __SPIN_LOCK_UNLOCKED(
+					ve0.release_list_lock),
+	.release_list		= LIST_HEAD_INIT(ve0.release_list),
+	.release_agent_work	= __WORK_INITIALIZER(ve0.release_agent_work,
+					cgroup1_release_agent),
 };
 EXPORT_SYMBOL(ve0);
 
@@ -478,6 +483,69 @@ static void ve_workqueue_stop(struct ve_struct *ve)
 	ve->wq = NULL;
 }
 
+/*
+ * ve_add_to_release_list - called from cgroup1_check_for_release to put a
+ * cgroup into a release workqueue. There are two codepaths that lead to this
+ * function. One starts from cgroup_exit() which holds css_set_lock, another
+ * one from cgroup_destroy_locked which does not hold css_set_lock. So we
+ * should not use any reschedulable
+ *
+ */
+void ve_add_to_release_list(struct cgroup *cgrp)
+{
+	struct ve_struct *ve;
+	unsigned long flags;
+	int need_schedule_work = 0;
+
+	rcu_read_lock();
+	ve = cgroup_ve_owner(cgrp);
+	if (!ve)
+		ve = &ve0;
+
+	if (!ve->is_running) {
+		rcu_read_unlock();
+		return;
+	}
+
+	spin_lock_irqsave(&ve->release_list_lock, flags);
+	if (!cgroup_is_dead(cgrp) &&
+	    list_empty(&cgrp->release_list)) {
+		list_add(&cgrp->release_list, &ve->release_list);
+		need_schedule_work = 1;
+	}
+	spin_unlock_irqrestore(&ve->release_list_lock, flags);
+
+	if (need_schedule_work)
+		queue_work(ve->wq, &ve->release_agent_work);
+
+	rcu_read_unlock();
+}
+
+/*
+ * As workqueue destroy happens before we unset CGRP_VE_ROOT and
+ * cgroup->ve_owner and also as destroy waits for all current works
+ * to finish, we can rely that if cgroup is in release_list of some ve
+ * then cgroup_ve_owner would return exactly the same ve, and we
+ * would take the right ve->release_list_lock for operating on
+ * our cgroup->release_list.
+ */
+void ve_rm_from_release_list(struct cgroup *cgrp)
+{
+	struct ve_struct *ve;
+	unsigned long flags;
+
+	rcu_read_lock();
+	ve = cgroup_ve_owner(cgrp);
+	if (!ve)
+		ve = &ve0;
+
+	spin_lock_irqsave(&ve->release_list_lock, flags);
+	if (!list_empty(&cgrp->release_list))
+		list_del_init(&cgrp->release_list);
+	spin_unlock_irqrestore(&ve->release_list_lock, flags);
+	rcu_read_unlock();
+}
+
 /* under ve->op_sem write-lock */
 static int ve_start_container(struct ve_struct *ve)
 {
@@ -720,6 +788,11 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
 		goto err_lat;
 
 	ve->features = VE_FEATURES_DEF;
+
+	INIT_WORK(&ve->release_agent_work, cgroup1_release_agent);
+	spin_lock_init(&ve->release_list_lock);
+	INIT_LIST_HEAD(&ve->release_list);
+
 	ve->_randomize_va_space = ve0._randomize_va_space;
 
 	ve->meminfo_val = VE_MEMINFO_DEFAULT;


More information about the Devel mailing list