* [PATCH v1 0/3] cgroup: Add lock guard support
@ 2025-06-06 16:18 Jemmy Wong
2025-06-06 16:18 ` [PATCH v1 1/3] cgroup: add lock guard support for cgroup_muetx Jemmy Wong
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Jemmy Wong @ 2025-06-06 16:18 UTC (permalink / raw)
To: Michal Koutný
Cc: Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel
v1 changes:
- remove guard support for BPF
- split patch into parts
v0 link:
https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/
Jemmy Wong (3):
cgroup: add lock guard support for cgroup_muetx
cgroup: add lock guard support for css_set_lock and rcu
cgroup: add lock guard support for others
include/linux/cgroup.h | 7 +
kernel/cgroup/cgroup-internal.h | 8 +-
kernel/cgroup/cgroup-v1.c | 56 ++-
kernel/cgroup/cgroup.c | 720 +++++++++++++++-----------------
kernel/cgroup/cpuset-v1.c | 16 +-
kernel/cgroup/debug.c | 185 ++++----
kernel/cgroup/freezer.c | 28 +-
kernel/cgroup/namespace.c | 8 +-
8 files changed, 483 insertions(+), 545 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v1 1/3] cgroup: add lock guard support for cgroup_muetx 2025-06-06 16:18 [PATCH v1 0/3] cgroup: Add lock guard support Jemmy Wong @ 2025-06-06 16:18 ` Jemmy Wong 2025-06-17 9:09 ` Michal Koutný 2025-06-06 16:18 ` [PATCH v1 2/3] cgroup: add lock guard support for css_set_lock and rcu Jemmy Wong ` (3 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Jemmy Wong @ 2025-06-06 16:18 UTC (permalink / raw) To: Michal Koutný Cc: Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel Signed-off-by: Jemmy Wong <jemmywong512@gmail.com> --- include/linux/cgroup.h | 7 ++++ kernel/cgroup/cgroup-v1.c | 16 ++++---- kernel/cgroup/cgroup.c | 81 +++++++++++++++++++-------------------- 3 files changed, 54 insertions(+), 50 deletions(-) diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index b18fb5fcb38e..979f827452ad 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -382,6 +382,10 @@ static inline void cgroup_put(struct cgroup *cgrp) extern struct mutex cgroup_mutex; +DEFINE_LOCK_GUARD_0(cgroup_mutex, + mutex_lock(&cgroup_mutex), + mutex_unlock(&cgroup_mutex)) + static inline void cgroup_lock(void) { mutex_lock(&cgroup_mutex); @@ -656,6 +660,9 @@ struct cgroup *cgroup_get_from_id(u64 id); struct cgroup_subsys_state; struct cgroup; +extern struct mutex cgroup_mutex; +DEFINE_LOCK_GUARD_0(cgroup_mutex, , ,) + static inline u64 cgroup_id(const struct cgroup *cgrp) { return 1; } static inline void css_get(struct cgroup_subsys_state *css) {} static inline void css_put(struct cgroup_subsys_state *css) {} diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c index fa24c032ed6f..f4658eda4445 100644 --- a/kernel/cgroup/cgroup-v1.c +++ b/kernel/cgroup/cgroup-v1.c @@ -64,7 +64,8 @@ int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk) struct cgroup_root *root; int retval = 0; - cgroup_lock(); + guard(cgroup_mutex)(); + cgroup_attach_lock(true); for_each_root(root) { struct cgroup *from_cgrp; @@ -78,7 +79,6 @@ int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk) break; } cgroup_attach_unlock(true); - cgroup_unlock(); return retval; } @@ -862,13 +862,11 @@ static int cgroup1_rename(struct kernfs_node *kn, struct kernfs_node *new_parent kernfs_break_active_protection(new_parent); kernfs_break_active_protection(kn); - cgroup_lock(); - - ret = kernfs_rename(kn, new_parent, new_name_str); - if (!ret) - TRACE_CGROUP_PATH(rename, cgrp); - - cgroup_unlock(); + scoped_guard(cgroup_mutex) { + ret = kernfs_rename(kn, new_parent, new_name_str); + if (!ret) + TRACE_CGROUP_PATH(rename, cgrp); + } kernfs_unbreak_active_protection(kn); kernfs_unbreak_active_protection(new_parent); diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index a723b7dc6e4e..54f80afe4f65 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -2224,13 +2224,13 @@ int cgroup_do_get_tree(struct fs_context *fc) struct super_block *sb = fc->root->d_sb; struct cgroup *cgrp; - cgroup_lock(); - spin_lock_irq(&css_set_lock); + scoped_guard(cgroup_mutex) { + spin_lock_irq(&css_set_lock); - cgrp = cset_cgroup_from_root(ctx->ns->root_cset, ctx->root); + cgrp = cset_cgroup_from_root(ctx->ns->root_cset, ctx->root); - spin_unlock_irq(&css_set_lock); - cgroup_unlock(); + spin_unlock_irq(&css_set_lock); + } nsdentry = kernfs_node_dentry(cgrp->kn, sb); dput(fc->root); @@ -2440,13 +2440,12 @@ int cgroup_path_ns(struct cgroup *cgrp, char *buf, size_t buflen, { int ret; - cgroup_lock(); + guard(cgroup_mutex)(); spin_lock_irq(&css_set_lock); ret = cgroup_path_ns_locked(cgrp, buf, buflen, ns); spin_unlock_irq(&css_set_lock); - cgroup_unlock(); return ret; } @@ -4472,9 +4471,10 @@ int cgroup_rm_cftypes(struct cftype *cfts) if (!(cfts[0].flags & __CFTYPE_ADDED)) return -ENOENT; - cgroup_lock(); - cgroup_rm_cftypes_locked(cfts); - cgroup_unlock(); + scoped_guard(cgroup_mutex) { + cgroup_rm_cftypes_locked(cfts); + } + return 0; } @@ -4506,14 +4506,13 @@ int cgroup_add_cftypes(struct cgroup_subsys *ss, struct cftype *cfts) if (ret) return ret; - cgroup_lock(); - - list_add_tail(&cfts->node, &ss->cfts); - ret = cgroup_apply_cftypes(cfts, true); - if (ret) - cgroup_rm_cftypes_locked(cfts); + scoped_guard(cgroup_mutex) { + list_add_tail(&cfts->node, &ss->cfts); + ret = cgroup_apply_cftypes(cfts, true); + if (ret) + cgroup_rm_cftypes_locked(cfts); + } - cgroup_unlock(); return ret; } @@ -5489,14 +5488,14 @@ static void css_free_rwork_fn(struct work_struct *work) } } -static void css_release_work_fn(struct work_struct *work) +static inline void css_release_work_fn_locked(struct work_struct *work) { struct cgroup_subsys_state *css = container_of(work, struct cgroup_subsys_state, destroy_work); struct cgroup_subsys *ss = css->ss; struct cgroup *cgrp = css->cgroup; - cgroup_lock(); + guard(cgroup_mutex)(); css->flags |= CSS_RELEASED; list_del_rcu(&css->sibling); @@ -5550,8 +5549,14 @@ static void css_release_work_fn(struct work_struct *work) NULL); } - cgroup_unlock(); +} + +static void css_release_work_fn(struct work_struct *work) +{ + struct cgroup_subsys_state *css = + container_of(work, struct cgroup_subsys_state, destroy_work); + css_release_work_fn_locked(work); INIT_RCU_WORK(&css->destroy_rwork, css_free_rwork_fn); queue_rcu_work(cgroup_destroy_wq, &css->destroy_rwork); } @@ -5914,7 +5919,7 @@ static void css_killed_work_fn(struct work_struct *work) struct cgroup_subsys_state *css = container_of(work, struct cgroup_subsys_state, destroy_work); - cgroup_lock(); + guard(cgroup_mutex)(); do { offline_css(css); @@ -5922,8 +5927,6 @@ static void css_killed_work_fn(struct work_struct *work) /* @css can't go away while we're holding cgroup_mutex */ css = css->parent; } while (css && atomic_dec_and_test(&css->online_cnt)); - - cgroup_unlock(); } /* css kill confirmation processing requires process context, bounce */ @@ -6115,7 +6118,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early) pr_debug("Initializing cgroup subsys %s\n", ss->name); - cgroup_lock(); + guard(cgroup_mutex)(); idr_init(&ss->css_idr); INIT_LIST_HEAD(&ss->cfts); @@ -6161,8 +6164,6 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss, bool early) BUG_ON(!list_empty(&init_task.tasks)); BUG_ON(online_css(css)); - - cgroup_unlock(); } /** @@ -6224,20 +6225,18 @@ int __init cgroup_init(void) get_user_ns(init_cgroup_ns.user_ns); - cgroup_lock(); - - /* - * Add init_css_set to the hash table so that dfl_root can link to - * it during init. - */ - hash_add(css_set_table, &init_css_set.hlist, - css_set_hash(init_css_set.subsys)); - - cgroup_bpf_lifetime_notifier_init(); + scoped_guard(cgroup_mutex) { + /* + * Add init_css_set to the hash table so that dfl_root can link to + * it during init. + */ + hash_add(css_set_table, &init_css_set.hlist, + css_set_hash(init_css_set.subsys)); - BUG_ON(cgroup_setup_root(&cgrp_dfl_root, 0)); + cgroup_bpf_lifetime_notifier_init(); - cgroup_unlock(); + BUG_ON(cgroup_setup_root(&cgrp_dfl_root, 0)); + } for_each_subsys(ss, ssid) { if (ss->early_init) { @@ -6289,9 +6288,9 @@ int __init cgroup_init(void) if (ss->bind) ss->bind(init_css_set.subsys[ssid]); - cgroup_lock(); - css_populate_dir(init_css_set.subsys[ssid]); - cgroup_unlock(); + scoped_guard(cgroup_mutex) { + css_populate_dir(init_css_set.subsys[ssid]); + } } /* init_css_set.subsys[] has been updated, re-hash */ -- 2.43.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/3] cgroup: add lock guard support for cgroup_muetx 2025-06-06 16:18 ` [PATCH v1 1/3] cgroup: add lock guard support for cgroup_muetx Jemmy Wong @ 2025-06-17 9:09 ` Michal Koutný 0 siblings, 0 replies; 15+ messages in thread From: Michal Koutný @ 2025-06-17 9:09 UTC (permalink / raw) To: Jemmy Wong; +Cc: Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel [-- Attachment #1: Type: text/plain, Size: 801 bytes --] (typo in subject) On Sat, Jun 07, 2025 at 12:18:39AM +0800, Jemmy Wong <jemmywong512@gmail.com> wrote: ... > @@ -5489,14 +5488,14 @@ static void css_free_rwork_fn(struct work_struct *work) > } > } > > -static void css_release_work_fn(struct work_struct *work) > +static inline void css_release_work_fn_locked(struct work_struct *work) > { > struct cgroup_subsys_state *css = > container_of(work, struct cgroup_subsys_state, destroy_work); > struct cgroup_subsys *ss = css->ss; > struct cgroup *cgrp = css->cgroup; > > - cgroup_lock(); > + guard(cgroup_mutex)(); I think this should use different name suffix than _locked to distinguish it from traditional _locked functions that expect a lock being held. E.g. *_locking? or __* (like __cgroup_task_count()). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 2/3] cgroup: add lock guard support for css_set_lock and rcu 2025-06-06 16:18 [PATCH v1 0/3] cgroup: Add lock guard support Jemmy Wong 2025-06-06 16:18 ` [PATCH v1 1/3] cgroup: add lock guard support for cgroup_muetx Jemmy Wong @ 2025-06-06 16:18 ` Jemmy Wong 2025-06-17 9:09 ` Michal Koutný 2025-06-06 16:18 ` [PATCH v1 3/3] cgroup: add lock guard support for others Jemmy Wong ` (2 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Jemmy Wong @ 2025-06-06 16:18 UTC (permalink / raw) To: Michal Koutný Cc: Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel Signed-off-by: Jemmy Wong <jemmywong512@gmail.com> --- kernel/cgroup/cgroup-v1.c | 29 +- kernel/cgroup/cgroup.c | 580 ++++++++++++++++++-------------------- kernel/cgroup/cpuset-v1.c | 16 +- kernel/cgroup/debug.c | 185 ++++++------ kernel/cgroup/freezer.c | 28 +- kernel/cgroup/namespace.c | 8 +- 6 files changed, 401 insertions(+), 445 deletions(-) diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c index f4658eda4445..fcc2d474b470 100644 --- a/kernel/cgroup/cgroup-v1.c +++ b/kernel/cgroup/cgroup-v1.c @@ -70,9 +70,9 @@ int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk) for_each_root(root) { struct cgroup *from_cgrp; - spin_lock_irq(&css_set_lock); - from_cgrp = task_cgroup_from_root(from, root); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + from_cgrp = task_cgroup_from_root(from, root); + } retval = cgroup_attach_task(from_cgrp, tsk, false); if (retval) @@ -117,10 +117,10 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from) cgroup_attach_lock(true); /* all tasks in @from are being moved, all csets are source */ - spin_lock_irq(&css_set_lock); - list_for_each_entry(link, &from->cset_links, cset_link) - cgroup_migrate_add_src(link->cset, to, &mgctx); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + list_for_each_entry(link, &from->cset_links, cset_link) + cgroup_migrate_add_src(link->cset, to, &mgctx); + } ret = cgroup_migrate_prepare_dst(&mgctx); if (ret) @@ -728,13 +728,12 @@ int cgroupstats_build(struct cgroupstats *stats, struct dentry *dentry) * @kn->priv's validity. For this and css_tryget_online_from_dir(), * @kn->priv is RCU safe. Let's do the RCU dancing. */ - rcu_read_lock(); - cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); - if (!cgrp || !cgroup_tryget(cgrp)) { - rcu_read_unlock(); - return -ENOENT; + scoped_guard(rcu) { + cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); + if (!cgrp || !cgroup_tryget(cgrp)) { + return -ENOENT; + } } - rcu_read_unlock(); css_task_iter_start(&cgrp->self, 0, &it); while ((tsk = css_task_iter_next(&it))) { @@ -1294,7 +1293,7 @@ struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id) struct cgroup_root *root; unsigned long flags; - rcu_read_lock(); + guard(rcu)(); for_each_root(root) { /* cgroup1 only*/ if (root == &cgrp_dfl_root) @@ -1308,7 +1307,7 @@ struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id) spin_unlock_irqrestore(&css_set_lock, flags); break; } - rcu_read_unlock(); + return cgrp; } diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index 54f80afe4f65..46b677a066d1 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -631,13 +631,8 @@ int __cgroup_task_count(const struct cgroup *cgrp) */ int cgroup_task_count(const struct cgroup *cgrp) { - int count; - - spin_lock_irq(&css_set_lock); - count = __cgroup_task_count(cgrp); - spin_unlock_irq(&css_set_lock); - - return count; + guard(spinlock_irq)(&css_set_lock); + return __cgroup_task_count(cgrp); } static struct cgroup *kn_priv(struct kernfs_node *kn) @@ -1202,11 +1197,11 @@ static struct css_set *find_css_set(struct css_set *old_cset, /* First see if we already have a cgroup group that matches * the desired set */ - spin_lock_irq(&css_set_lock); - cset = find_existing_css_set(old_cset, cgrp, template); - if (cset) - get_css_set(cset); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + cset = find_existing_css_set(old_cset, cgrp, template); + if (cset) + get_css_set(cset); + } if (cset) return cset; @@ -1238,34 +1233,33 @@ static struct css_set *find_css_set(struct css_set *old_cset, * find_existing_css_set() */ memcpy(cset->subsys, template, sizeof(cset->subsys)); - spin_lock_irq(&css_set_lock); - /* Add reference counts and links from the new css_set. */ - list_for_each_entry(link, &old_cset->cgrp_links, cgrp_link) { - struct cgroup *c = link->cgrp; + scoped_guard(spinlock_irq, &css_set_lock) { + /* Add reference counts and links from the new css_set. */ + list_for_each_entry(link, &old_cset->cgrp_links, cgrp_link) { + struct cgroup *c = link->cgrp; - if (c->root == cgrp->root) - c = cgrp; - link_css_set(&tmp_links, cset, c); - } + if (c->root == cgrp->root) + c = cgrp; + link_css_set(&tmp_links, cset, c); + } - BUG_ON(!list_empty(&tmp_links)); + BUG_ON(!list_empty(&tmp_links)); - css_set_count++; + css_set_count++; - /* Add @cset to the hash table */ - key = css_set_hash(cset->subsys); - hash_add(css_set_table, &cset->hlist, key); + /* Add @cset to the hash table */ + key = css_set_hash(cset->subsys); + hash_add(css_set_table, &cset->hlist, key); - for_each_subsys(ss, ssid) { - struct cgroup_subsys_state *css = cset->subsys[ssid]; + for_each_subsys(ss, ssid) { + struct cgroup_subsys_state *css = cset->subsys[ssid]; - list_add_tail(&cset->e_cset_node[ssid], - &css->cgroup->e_csets[ssid]); - css_get(css); + list_add_tail(&cset->e_cset_node[ssid], + &css->cgroup->e_csets[ssid]); + css_get(css); + } } - spin_unlock_irq(&css_set_lock); - /* * If @cset should be threaded, look up the matching dom_cset and * link them up. We first fully initialize @cset then look for the @@ -1281,11 +1275,11 @@ static struct css_set *find_css_set(struct css_set *old_cset, return NULL; } - spin_lock_irq(&css_set_lock); - cset->dom_cset = dcset; - list_add_tail(&cset->threaded_csets_node, - &dcset->threaded_csets); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + cset->dom_cset = dcset; + list_add_tail(&cset->threaded_csets_node, + &dcset->threaded_csets); + } } return cset; @@ -1362,16 +1356,14 @@ static void cgroup_destroy_root(struct cgroup_root *root) * Release all the links from cset_links to this hierarchy's * root cgroup */ - spin_lock_irq(&css_set_lock); - - list_for_each_entry_safe(link, tmp_link, &cgrp->cset_links, cset_link) { - list_del(&link->cset_link); - list_del(&link->cgrp_link); - kfree(link); + scoped_guard(spinlock_irq, &css_set_lock) { + list_for_each_entry_safe(link, tmp_link, &cgrp->cset_links, cset_link) { + list_del(&link->cset_link); + list_del(&link->cgrp_link); + kfree(link); + } } - spin_unlock_irq(&css_set_lock); - WARN_ON_ONCE(list_empty(&root->root_list)); list_del_rcu(&root->root_list); cgroup_root_count--; @@ -1437,13 +1429,10 @@ current_cgns_cgroup_from_root(struct cgroup_root *root) lockdep_assert_held(&css_set_lock); - rcu_read_lock(); - - cset = current->nsproxy->cgroup_ns->root_cset; - res = __cset_cgroup_from_root(cset, root); - - rcu_read_unlock(); - + scoped_guard(rcu) { + cset = current->nsproxy->cgroup_ns->root_cset; + res = __cset_cgroup_from_root(cset, root); + } /* * The namespace_sem is held by current, so the root cgroup can't * be umounted. Therefore, we can ensure that the res is non-NULL. @@ -1867,25 +1856,25 @@ int rebind_subsystems(struct cgroup_root *dst_root, u16 ss_mask) rcu_assign_pointer(dcgrp->subsys[ssid], css); ss->root = dst_root; - spin_lock_irq(&css_set_lock); - css->cgroup = dcgrp; - WARN_ON(!list_empty(&dcgrp->e_csets[ss->id])); - list_for_each_entry_safe(cset, cset_pos, &scgrp->e_csets[ss->id], - e_cset_node[ss->id]) { - list_move_tail(&cset->e_cset_node[ss->id], - &dcgrp->e_csets[ss->id]); - /* - * all css_sets of scgrp together in same order to dcgrp, - * patch in-flight iterators to preserve correct iteration. - * since the iterator is always advanced right away and - * finished when it->cset_pos meets it->cset_head, so only - * update it->cset_head is enough here. - */ - list_for_each_entry(it, &cset->task_iters, iters_node) - if (it->cset_head == &scgrp->e_csets[ss->id]) - it->cset_head = &dcgrp->e_csets[ss->id]; + scoped_guard(spinlock_irq, &css_set_lock) { + css->cgroup = dcgrp; + WARN_ON(!list_empty(&dcgrp->e_csets[ss->id])); + list_for_each_entry_safe(cset, cset_pos, &scgrp->e_csets[ss->id], + e_cset_node[ss->id]) { + list_move_tail(&cset->e_cset_node[ss->id], + &dcgrp->e_csets[ss->id]); + /* + * all css_sets of scgrp together in same order to dcgrp, + * patch in-flight iterators to preserve correct iteration. + * since the iterator is always advanced right away and + * finished when it->cset_pos meets it->cset_head, so only + * update it->cset_head is enough here. + */ + list_for_each_entry(it, &cset->task_iters, iters_node) + if (it->cset_head == &scgrp->e_csets[ss->id]) + it->cset_head = &dcgrp->e_csets[ss->id]; + } } - spin_unlock_irq(&css_set_lock); /* default hierarchy doesn't enable controllers by default */ dst_root->subsys_mask |= 1 << ssid; @@ -1921,10 +1910,10 @@ int cgroup_show_path(struct seq_file *sf, struct kernfs_node *kf_node, if (!buf) return -ENOMEM; - spin_lock_irq(&css_set_lock); - ns_cgroup = current_cgns_cgroup_from_root(kf_cgroot); - len = kernfs_path_from_node(kf_node, ns_cgroup->kn, buf, PATH_MAX); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + ns_cgroup = current_cgns_cgroup_from_root(kf_cgroot); + len = kernfs_path_from_node(kf_node, ns_cgroup->kn, buf, PATH_MAX); + } if (len == -E2BIG) len = -ERANGE; @@ -2175,13 +2164,13 @@ int cgroup_setup_root(struct cgroup_root *root, u16 ss_mask) * Link the root cgroup in this hierarchy into all the css_set * objects. */ - spin_lock_irq(&css_set_lock); - hash_for_each(css_set_table, i, cset, hlist) { - link_css_set(&tmp_links, cset, root_cgrp); - if (css_set_populated(cset)) - cgroup_update_populated(root_cgrp, true); + scoped_guard(spinlock_irq, &css_set_lock) { + hash_for_each(css_set_table, i, cset, hlist) { + link_css_set(&tmp_links, cset, root_cgrp); + if (css_set_populated(cset)) + cgroup_update_populated(root_cgrp, true); + } } - spin_unlock_irq(&css_set_lock); BUG_ON(!list_empty(&root_cgrp->self.children)); BUG_ON(atomic_read(&root->nr_cgrps) != 1); @@ -2225,11 +2214,8 @@ int cgroup_do_get_tree(struct fs_context *fc) struct cgroup *cgrp; scoped_guard(cgroup_mutex) { - spin_lock_irq(&css_set_lock); - + guard(spinlock_irq)(&css_set_lock); cgrp = cset_cgroup_from_root(ctx->ns->root_cset, ctx->root); - - spin_unlock_irq(&css_set_lock); } nsdentry = kernfs_node_dentry(cgrp->kn, sb); @@ -2438,16 +2424,10 @@ int cgroup_path_ns_locked(struct cgroup *cgrp, char *buf, size_t buflen, int cgroup_path_ns(struct cgroup *cgrp, char *buf, size_t buflen, struct cgroup_namespace *ns) { - int ret; - guard(cgroup_mutex)(); - spin_lock_irq(&css_set_lock); - - ret = cgroup_path_ns_locked(cgrp, buf, buflen, ns); - - spin_unlock_irq(&css_set_lock); + guard(spinlock_irq)(&css_set_lock); - return ret; + return cgroup_path_ns_locked(cgrp, buf, buflen, ns); } EXPORT_SYMBOL_GPL(cgroup_path_ns); @@ -2629,27 +2609,27 @@ static int cgroup_migrate_execute(struct cgroup_mgctx *mgctx) * the new cgroup. There are no failure cases after here, so this * is the commit point. */ - spin_lock_irq(&css_set_lock); - list_for_each_entry(cset, &tset->src_csets, mg_node) { - list_for_each_entry_safe(task, tmp_task, &cset->mg_tasks, cg_list) { - struct css_set *from_cset = task_css_set(task); - struct css_set *to_cset = cset->mg_dst_cset; - - get_css_set(to_cset); - to_cset->nr_tasks++; - css_set_move_task(task, from_cset, to_cset, true); - from_cset->nr_tasks--; - /* - * If the source or destination cgroup is frozen, - * the task might require to change its state. - */ - cgroup_freezer_migrate_task(task, from_cset->dfl_cgrp, - to_cset->dfl_cgrp); - put_css_set_locked(from_cset); + scoped_guard(spinlock_irq, &css_set_lock) { + list_for_each_entry(cset, &tset->src_csets, mg_node) { + list_for_each_entry_safe(task, tmp_task, &cset->mg_tasks, cg_list) { + struct css_set *from_cset = task_css_set(task); + struct css_set *to_cset = cset->mg_dst_cset; + + get_css_set(to_cset); + to_cset->nr_tasks++; + css_set_move_task(task, from_cset, to_cset, true); + from_cset->nr_tasks--; + /* + * If the source or destination cgroup is frozen, + * the task might require to change its state. + */ + cgroup_freezer_migrate_task(task, from_cset->dfl_cgrp, + to_cset->dfl_cgrp); + put_css_set_locked(from_cset); + } } } - spin_unlock_irq(&css_set_lock); /* * Migration is committed, all target tasks are now on dst_csets. @@ -2682,13 +2662,13 @@ static int cgroup_migrate_execute(struct cgroup_mgctx *mgctx) } while_each_subsys_mask(); } out_release_tset: - spin_lock_irq(&css_set_lock); - list_splice_init(&tset->dst_csets, &tset->src_csets); - list_for_each_entry_safe(cset, tmp_cset, &tset->src_csets, mg_node) { - list_splice_tail_init(&cset->mg_tasks, &cset->tasks); - list_del_init(&cset->mg_node); + scoped_guard(spinlock_irq, &css_set_lock) { + list_splice_init(&tset->dst_csets, &tset->src_csets); + list_for_each_entry_safe(cset, tmp_cset, &tset->src_csets, mg_node) { + list_splice_tail_init(&cset->mg_tasks, &cset->tasks); + list_del_init(&cset->mg_node); + } } - spin_unlock_irq(&css_set_lock); /* * Re-initialize the cgroup_taskset structure in case it is reused @@ -2746,7 +2726,7 @@ void cgroup_migrate_finish(struct cgroup_mgctx *mgctx) lockdep_assert_held(&cgroup_mutex); - spin_lock_irq(&css_set_lock); + guard(spinlock_irq)(&css_set_lock); list_for_each_entry_safe(cset, tmp_cset, &mgctx->preloaded_src_csets, mg_src_preload_node) { @@ -2765,8 +2745,6 @@ void cgroup_migrate_finish(struct cgroup_mgctx *mgctx) list_del_init(&cset->mg_dst_preload_node); put_css_set_locked(cset); } - - spin_unlock_irq(&css_set_lock); } /** @@ -2909,14 +2887,14 @@ int cgroup_migrate(struct task_struct *leader, bool threadgroup, * section to prevent tasks from being freed while taking the snapshot. * spin_lock_irq() implies RCU critical section here. */ - spin_lock_irq(&css_set_lock); - task = leader; - do { - cgroup_migrate_add_task(task, mgctx); - if (!threadgroup) - break; - } while_each_thread(leader, task); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + task = leader; + do { + cgroup_migrate_add_task(task, mgctx); + if (!threadgroup) + break; + } while_each_thread(leader, task); + } return cgroup_migrate_execute(mgctx); } @@ -2937,16 +2915,15 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader, int ret = 0; /* look up all src csets */ - spin_lock_irq(&css_set_lock); - rcu_read_lock(); - task = leader; - do { - cgroup_migrate_add_src(task_css_set(task), dst_cgrp, &mgctx); - if (!threadgroup) - break; - } while_each_thread(leader, task); - rcu_read_unlock(); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + guard(rcu)(); + task = leader; + do { + cgroup_migrate_add_src(task_css_set(task), dst_cgrp, &mgctx); + if (!threadgroup) + break; + } while_each_thread(leader, task); + } /* prepare dst csets and commit */ ret = cgroup_migrate_prepare_dst(&mgctx); @@ -3088,23 +3065,23 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp) lockdep_assert_held(&cgroup_mutex); /* look up all csses currently attached to @cgrp's subtree */ - spin_lock_irq(&css_set_lock); - cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) { - struct cgrp_cset_link *link; + scoped_guard(spinlock_irq, &css_set_lock) { + cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) { + struct cgrp_cset_link *link; - /* - * As cgroup_update_dfl_csses() is only called by - * cgroup_apply_control(). The csses associated with the - * given cgrp will not be affected by changes made to - * its subtree_control file. We can skip them. - */ - if (dsct == cgrp) - continue; + /* + * As cgroup_update_dfl_csses() is only called by + * cgroup_apply_control(). The csses associated with the + * given cgrp will not be affected by changes made to + * its subtree_control file. We can skip them. + */ + if (dsct == cgrp) + continue; - list_for_each_entry(link, &dsct->cset_links, cset_link) - cgroup_migrate_add_src(link->cset, dsct, &mgctx); + list_for_each_entry(link, &dsct->cset_links, cset_link) + cgroup_migrate_add_src(link->cset, dsct, &mgctx); + } } - spin_unlock_irq(&css_set_lock); /* * We need to write-lock threadgroup_rwsem while migrating tasks. @@ -3120,16 +3097,16 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp) if (ret) goto out_finish; - spin_lock_irq(&css_set_lock); - list_for_each_entry(src_cset, &mgctx.preloaded_src_csets, - mg_src_preload_node) { - struct task_struct *task, *ntask; + scoped_guard(spinlock_irq, &css_set_lock) { + list_for_each_entry(src_cset, &mgctx.preloaded_src_csets, + mg_src_preload_node) { + struct task_struct *task, *ntask; - /* all tasks in src_csets need to be migrated */ - list_for_each_entry_safe(task, ntask, &src_cset->tasks, cg_list) - cgroup_migrate_add_task(task, &mgctx); + /* all tasks in src_csets need to be migrated */ + list_for_each_entry_safe(task, ntask, &src_cset->tasks, cg_list) + cgroup_migrate_add_task(task, &mgctx); + } } - spin_unlock_irq(&css_set_lock); ret = cgroup_migrate_execute(&mgctx); out_finish: @@ -3734,7 +3711,8 @@ static int cgroup_stat_show(struct seq_file *seq, void *v) * Without proper lock protection, racing is possible. So the * numbers may not be consistent when that happens. */ - rcu_read_lock(); + guard(rcu)(); + for (ssid = 0; ssid < CGROUP_SUBSYS_COUNT; ssid++) { dying_cnt[ssid] = -1; if ((BIT(ssid) & cgrp_dfl_inhibit_ss_mask) || @@ -3753,7 +3731,6 @@ static int cgroup_stat_show(struct seq_file *seq, void *v) seq_printf(seq, "nr_dying_subsys_%s %d\n", cgroup_subsys[ssid]->name, dying_cnt[ssid]); } - rcu_read_unlock(); return 0; } @@ -3771,11 +3748,10 @@ static struct cgroup_subsys_state *cgroup_tryget_css(struct cgroup *cgrp, { struct cgroup_subsys_state *css; - rcu_read_lock(); + guard(rcu)(); css = cgroup_css(cgrp, ss); if (css && !css_tryget_online(css)) css = NULL; - rcu_read_unlock(); return css; } @@ -4056,9 +4032,9 @@ static void __cgroup_kill(struct cgroup *cgrp) lockdep_assert_held(&cgroup_mutex); - spin_lock_irq(&css_set_lock); - cgrp->kill_seq++; - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + cgrp->kill_seq++; + } css_task_iter_start(&cgrp->self, CSS_TASK_ITER_PROCS | CSS_TASK_ITER_THREADED, &it); while ((task = css_task_iter_next(&it))) { @@ -4187,9 +4163,9 @@ static ssize_t cgroup_file_write(struct kernfs_open_file *of, char *buf, * doesn't need to be pinned. The RCU locking is not necessary * either. It's just for the convenience of using cgroup_css(). */ - rcu_read_lock(); - css = cgroup_css(cgrp, cft->ss); - rcu_read_unlock(); + scoped_guard(rcu) { + css = cgroup_css(cgrp, cft->ss); + } if (cft->write_u64) { unsigned long long v; @@ -4815,14 +4791,14 @@ bool css_has_online_children(struct cgroup_subsys_state *css) struct cgroup_subsys_state *child; bool ret = false; - rcu_read_lock(); + guard(rcu)(); css_for_each_child(child, css) { if (child->flags & CSS_ONLINE) { ret = true; break; } } - rcu_read_unlock(); + return ret; } @@ -5247,9 +5223,9 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf, goto out_unlock; /* find the source cgroup */ - spin_lock_irq(&css_set_lock); - src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root); + } /* * Process and thread migrations follow same delegation rule. Check @@ -5531,11 +5507,11 @@ static inline void css_release_work_fn_locked(struct work_struct *work) css_rstat_flush(&cgrp->self); - spin_lock_irq(&css_set_lock); - for (tcgrp = cgroup_parent(cgrp); tcgrp; - tcgrp = cgroup_parent(tcgrp)) - tcgrp->nr_dying_descendants--; - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + for (tcgrp = cgroup_parent(cgrp); tcgrp; + tcgrp = cgroup_parent(tcgrp)) + tcgrp->nr_dying_descendants--; + } /* * There are two control paths which try to determine @@ -5790,20 +5766,20 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name, goto out_psi_free; /* allocation complete, commit to creation */ - spin_lock_irq(&css_set_lock); - for (i = 0; i < level; i++) { - tcgrp = cgrp->ancestors[i]; - tcgrp->nr_descendants++; + scoped_guard(spinlock_irq, &css_set_lock) { + for (i = 0; i < level; i++) { + tcgrp = cgrp->ancestors[i]; + tcgrp->nr_descendants++; - /* - * If the new cgroup is frozen, all ancestor cgroups get a new - * frozen descendant, but their state can't change because of - * this. - */ - if (cgrp->freezer.e_freeze) - tcgrp->freezer.nr_frozen_descendants++; + /* + * If the new cgroup is frozen, all ancestor cgroups get a new + * frozen descendant, but their state can't change because of + * this. + */ + if (cgrp->freezer.e_freeze) + tcgrp->freezer.nr_frozen_descendants++; + } } - spin_unlock_irq(&css_set_lock); list_add_tail_rcu(&cgrp->self.sibling, &cgroup_parent(cgrp)->self.children); atomic_inc(&root->nr_cgrps); @@ -6047,10 +6023,10 @@ static int cgroup_destroy_locked(struct cgroup *cgrp) */ cgrp->self.flags &= ~CSS_ONLINE; - spin_lock_irq(&css_set_lock); - list_for_each_entry(link, &cgrp->cset_links, cset_link) - link->cset->dead = true; - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + list_for_each_entry(link, &cgrp->cset_links, cset_link) + link->cset->dead = true; + } /* initiate massacre of all css's */ for_each_css(css, ssid, cgrp) @@ -6063,18 +6039,18 @@ static int cgroup_destroy_locked(struct cgroup *cgrp) if (cgroup_is_threaded(cgrp)) parent->nr_threaded_children--; - spin_lock_irq(&css_set_lock); - for (tcgrp = parent; tcgrp; tcgrp = cgroup_parent(tcgrp)) { - tcgrp->nr_descendants--; - tcgrp->nr_dying_descendants++; - /* - * If the dying cgroup is frozen, decrease frozen descendants - * counters of ancestor cgroups. - */ - if (test_bit(CGRP_FROZEN, &cgrp->flags)) - tcgrp->freezer.nr_frozen_descendants--; + scoped_guard(spinlock_irq, &css_set_lock) { + for (tcgrp = parent; tcgrp; tcgrp = cgroup_parent(tcgrp)) { + tcgrp->nr_descendants--; + tcgrp->nr_dying_descendants++; + /* + * If the dying cgroup is frozen, decrease frozen descendants + * counters of ancestor cgroups. + */ + if (test_bit(CGRP_FROZEN, &cgrp->flags)) + tcgrp->freezer.nr_frozen_descendants--; + } } - spin_unlock_irq(&css_set_lock); cgroup1_check_for_release(parent); @@ -6356,13 +6332,12 @@ struct cgroup *cgroup_get_from_id(u64 id) return ERR_PTR(-ENOENT); } - rcu_read_lock(); - - cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); - if (cgrp && !cgroup_tryget(cgrp)) - cgrp = NULL; + scoped_guard(rcu) { + cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); + if (cgrp && !cgroup_tryget(cgrp)) + cgrp = NULL; + } - rcu_read_unlock(); kernfs_put(kn); if (!cgrp) @@ -6539,14 +6514,14 @@ static int cgroup_css_set_fork(struct kernel_clone_args *kargs) cgroup_threadgroup_change_begin(current); - spin_lock_irq(&css_set_lock); - cset = task_css_set(current); - get_css_set(cset); - if (kargs->cgrp) - kargs->kill_seq = kargs->cgrp->kill_seq; - else - kargs->kill_seq = cset->dfl_cgrp->kill_seq; - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + cset = task_css_set(current); + get_css_set(cset); + if (kargs->cgrp) + kargs->kill_seq = kargs->cgrp->kill_seq; + else + kargs->kill_seq = cset->dfl_cgrp->kill_seq; + } if (!(kargs->flags & CLONE_INTO_CGROUP)) { kargs->cset = cset; @@ -6736,56 +6711,53 @@ void cgroup_post_fork(struct task_struct *child, cset = kargs->cset; kargs->cset = NULL; - spin_lock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + /* init tasks are special, only link regular threads */ + if (likely(child->pid)) { + if (kargs->cgrp) { + cgrp_flags = kargs->cgrp->flags; + cgrp_kill_seq = kargs->cgrp->kill_seq; + } else { + cgrp_flags = cset->dfl_cgrp->flags; + cgrp_kill_seq = cset->dfl_cgrp->kill_seq; + } - /* init tasks are special, only link regular threads */ - if (likely(child->pid)) { - if (kargs->cgrp) { - cgrp_flags = kargs->cgrp->flags; - cgrp_kill_seq = kargs->cgrp->kill_seq; + WARN_ON_ONCE(!list_empty(&child->cg_list)); + cset->nr_tasks++; + css_set_move_task(child, NULL, cset, false); } else { - cgrp_flags = cset->dfl_cgrp->flags; - cgrp_kill_seq = cset->dfl_cgrp->kill_seq; + put_css_set(cset); + cset = NULL; } - WARN_ON_ONCE(!list_empty(&child->cg_list)); - cset->nr_tasks++; - css_set_move_task(child, NULL, cset, false); - } else { - put_css_set(cset); - cset = NULL; - } - - if (!(child->flags & PF_KTHREAD)) { - if (unlikely(test_bit(CGRP_FREEZE, &cgrp_flags))) { - /* - * If the cgroup has to be frozen, the new task has - * too. Let's set the JOBCTL_TRAP_FREEZE jobctl bit to - * get the task into the frozen state. - */ - spin_lock(&child->sighand->siglock); - WARN_ON_ONCE(child->frozen); - child->jobctl |= JOBCTL_TRAP_FREEZE; - spin_unlock(&child->sighand->siglock); + if (!(child->flags & PF_KTHREAD)) { + if (unlikely(test_bit(CGRP_FREEZE, &cgrp_flags))) { + /* + * If the cgroup has to be frozen, the new task has + * too. Let's set the JOBCTL_TRAP_FREEZE jobctl bit to + * get the task into the frozen state. + */ + spin_lock(&child->sighand->siglock); + WARN_ON_ONCE(child->frozen); + child->jobctl |= JOBCTL_TRAP_FREEZE; + spin_unlock(&child->sighand->siglock); + + /* + * Calling cgroup_update_frozen() isn't required here, + * because it will be called anyway a bit later from + * do_freezer_trap(). So we avoid cgroup's transient + * switch from the frozen state and back. + */ + } /* - * Calling cgroup_update_frozen() isn't required here, - * because it will be called anyway a bit later from - * do_freezer_trap(). So we avoid cgroup's transient - * switch from the frozen state and back. + * If the cgroup is to be killed notice it now and take the + * child down right after we finished preparing it for + * userspace. */ + kill = kargs->kill_seq != cgrp_kill_seq; } - - /* - * If the cgroup is to be killed notice it now and take the - * child down right after we finished preparing it for - * userspace. - */ - kill = kargs->kill_seq != cgrp_kill_seq; } - - spin_unlock_irq(&css_set_lock); - /* * Call ss->fork(). This must happen after @child is linked on * css_set; otherwise, @child might change state between ->fork() @@ -6824,25 +6796,23 @@ void cgroup_exit(struct task_struct *tsk) struct css_set *cset; int i; - spin_lock_irq(&css_set_lock); - - WARN_ON_ONCE(list_empty(&tsk->cg_list)); - cset = task_css_set(tsk); - css_set_move_task(tsk, cset, NULL, false); - cset->nr_tasks--; - /* matches the signal->live check in css_task_iter_advance() */ - if (thread_group_leader(tsk) && atomic_read(&tsk->signal->live)) - list_add_tail(&tsk->cg_list, &cset->dying_tasks); - - if (dl_task(tsk)) - dec_dl_tasks_cs(tsk); + scoped_guard(spinlock_irq, &css_set_lock) { + WARN_ON_ONCE(list_empty(&tsk->cg_list)); + cset = task_css_set(tsk); + css_set_move_task(tsk, cset, NULL, false); + cset->nr_tasks--; + /* matches the signal->live check in css_task_iter_advance() */ + if (thread_group_leader(tsk) && atomic_read(&tsk->signal->live)) + list_add_tail(&tsk->cg_list, &cset->dying_tasks); - WARN_ON_ONCE(cgroup_task_frozen(tsk)); - if (unlikely(!(tsk->flags & PF_KTHREAD) && - test_bit(CGRP_FREEZE, &task_dfl_cgroup(tsk)->flags))) - cgroup_update_frozen(task_dfl_cgroup(tsk)); + if (dl_task(tsk)) + dec_dl_tasks_cs(tsk); - spin_unlock_irq(&css_set_lock); + WARN_ON_ONCE(cgroup_task_frozen(tsk)); + if (unlikely(!(tsk->flags & PF_KTHREAD) && + test_bit(CGRP_FREEZE, &task_dfl_cgroup(tsk)->flags))) + cgroup_update_frozen(task_dfl_cgroup(tsk)); + } /* see cgroup_post_fork() for details */ do_each_subsys_mask(ss, i, have_exit_callback) { @@ -6860,10 +6830,9 @@ void cgroup_release(struct task_struct *task) } while_each_subsys_mask(); if (!list_empty(&task->cg_list)) { - spin_lock_irq(&css_set_lock); + guard(spinlock_irq)(&css_set_lock); css_set_skip_task_iters(task_css_set(task), task); list_del_init(&task->cg_list); - spin_unlock_irq(&css_set_lock); } } @@ -6944,7 +6913,7 @@ struct cgroup_subsys_state *css_tryget_online_from_dir(struct dentry *dentry, !kn || kernfs_type(kn) != KERNFS_DIR) return ERR_PTR(-EBADF); - rcu_read_lock(); + guard(rcu)(); /* * This path doesn't originate from kernfs and @kn could already @@ -6958,7 +6927,6 @@ struct cgroup_subsys_state *css_tryget_online_from_dir(struct dentry *dentry, if (!css || !css_tryget_online(css)) css = ERR_PTR(-ENOENT); - rcu_read_unlock(); return css; } @@ -7001,13 +6969,11 @@ struct cgroup *cgroup_get_from_path(const char *path) goto out_kernfs; } - rcu_read_lock(); - - cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); - if (!cgrp || !cgroup_tryget(cgrp)) - cgrp = ERR_PTR(-ENOENT); - - rcu_read_unlock(); + scoped_guard(rcu) { + cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); + if (!cgrp || !cgroup_tryget(cgrp)) + cgrp = ERR_PTR(-ENOENT); + } out_kernfs: kernfs_put(kn); @@ -7106,28 +7072,28 @@ void cgroup_sk_alloc(struct sock_cgroup_data *skcd) { struct cgroup *cgroup; - rcu_read_lock(); - /* Don't associate the sock with unrelated interrupted task's cgroup. */ - if (in_interrupt()) { - cgroup = &cgrp_dfl_root.cgrp; - cgroup_get(cgroup); - goto out; - } + scoped_guard(rcu) { + /* Don't associate the sock with unrelated interrupted task's cgroup. */ + if (in_interrupt()) { + cgroup = &cgrp_dfl_root.cgrp; + cgroup_get(cgroup); + break; + } - while (true) { - struct css_set *cset; + while (true) { + struct css_set *cset; - cset = task_css_set(current); - if (likely(cgroup_tryget(cset->dfl_cgrp))) { - cgroup = cset->dfl_cgrp; - break; + cset = task_css_set(current); + if (likely(cgroup_tryget(cset->dfl_cgrp))) { + cgroup = cset->dfl_cgrp; + break; + } + cpu_relax(); } - cpu_relax(); + + skcd->cgroup = cgroup; + cgroup_bpf_get(cgroup); } -out: - skcd->cgroup = cgroup; - cgroup_bpf_get(cgroup); - rcu_read_unlock(); } void cgroup_sk_clone(struct sock_cgroup_data *skcd) diff --git a/kernel/cgroup/cpuset-v1.c b/kernel/cgroup/cpuset-v1.c index b69a7db67090..114a63432d23 100644 --- a/kernel/cgroup/cpuset-v1.c +++ b/kernel/cgroup/cpuset-v1.c @@ -140,9 +140,8 @@ int cpuset_memory_pressure_enabled __read_mostly; void __cpuset_memory_pressure_bump(void) { - rcu_read_lock(); + guard(rcu)(); fmeter_markevent(&task_cs(current)->fmeter); - rcu_read_unlock(); } static int update_relax_domain_level(struct cpuset *cs, s64 val) @@ -393,13 +392,12 @@ int proc_cpuset_show(struct seq_file *m, struct pid_namespace *ns, if (!buf) goto out; - rcu_read_lock(); - spin_lock_irq(&css_set_lock); - css = task_css(tsk, cpuset_cgrp_id); - retval = cgroup_path_ns_locked(css->cgroup, buf, PATH_MAX, - current->nsproxy->cgroup_ns); - spin_unlock_irq(&css_set_lock); - rcu_read_unlock(); + scoped_guard(rcu) { + guard(spinlock_irq)(&css_set_lock); + css = task_css(tsk, cpuset_cgrp_id); + retval = cgroup_path_ns_locked(css->cgroup, buf, PATH_MAX, + current->nsproxy->cgroup_ns); + } if (retval == -E2BIG) retval = -ENAMETOOLONG; diff --git a/kernel/cgroup/debug.c b/kernel/cgroup/debug.c index 80aa3f027ac3..2f04db56c8ac 100644 --- a/kernel/cgroup/debug.c +++ b/kernel/cgroup/debug.c @@ -48,27 +48,26 @@ static int current_css_set_read(struct seq_file *seq, void *v) if (!cgroup_kn_lock_live(of->kn, false)) return -ENODEV; - spin_lock_irq(&css_set_lock); - rcu_read_lock(); - cset = task_css_set(current); - refcnt = refcount_read(&cset->refcount); - seq_printf(seq, "css_set %pK %d", cset, refcnt); - if (refcnt > cset->nr_tasks) - seq_printf(seq, " +%d", refcnt - cset->nr_tasks); - seq_puts(seq, "\n"); + scoped_guard(spinlock_irq, &css_set_lock) { + guard(rcu)(); + cset = task_css_set(current); + refcnt = refcount_read(&cset->refcount); + seq_printf(seq, "css_set %pK %d", cset, refcnt); + if (refcnt > cset->nr_tasks) + seq_printf(seq, " +%d", refcnt - cset->nr_tasks); + seq_puts(seq, "\n"); - /* - * Print the css'es stored in the current css_set. - */ - for_each_subsys(ss, i) { - css = cset->subsys[ss->id]; - if (!css) - continue; - seq_printf(seq, "%2d: %-4s\t- %p[%d]\n", ss->id, ss->name, - css, css->id); + /* + * Print the css'es stored in the current css_set. + */ + for_each_subsys(ss, i) { + css = cset->subsys[ss->id]; + if (!css) + continue; + seq_printf(seq, "%2d: %-4s\t- %p[%d]\n", ss->id, ss->name, + css, css->id); + } } - rcu_read_unlock(); - spin_unlock_irq(&css_set_lock); cgroup_kn_unlock(of->kn); return 0; } @@ -76,12 +75,8 @@ static int current_css_set_read(struct seq_file *seq, void *v) static u64 current_css_set_refcount_read(struct cgroup_subsys_state *css, struct cftype *cft) { - u64 count; - - rcu_read_lock(); - count = refcount_read(&task_css_set(current)->refcount); - rcu_read_unlock(); - return count; + guard(rcu)(); + return refcount_read(&task_css_set(current)->refcount); } static int current_css_set_cg_links_read(struct seq_file *seq, void *v) @@ -94,18 +89,17 @@ static int current_css_set_cg_links_read(struct seq_file *seq, void *v) if (!name_buf) return -ENOMEM; - spin_lock_irq(&css_set_lock); - rcu_read_lock(); - cset = task_css_set(current); - list_for_each_entry(link, &cset->cgrp_links, cgrp_link) { - struct cgroup *c = link->cgrp; + scoped_guard(spinlock_irq, &css_set_lock) { + guard(rcu)(); + cset = task_css_set(current); + list_for_each_entry(link, &cset->cgrp_links, cgrp_link) { + struct cgroup *c = link->cgrp; - cgroup_name(c, name_buf, NAME_MAX + 1); - seq_printf(seq, "Root %d group %s\n", - c->root->hierarchy_id, name_buf); + cgroup_name(c, name_buf, NAME_MAX + 1); + seq_printf(seq, "Root %d group %s\n", + c->root->hierarchy_id, name_buf); + } } - rcu_read_unlock(); - spin_unlock_irq(&css_set_lock); kfree(name_buf); return 0; } @@ -117,74 +111,73 @@ static int cgroup_css_links_read(struct seq_file *seq, void *v) struct cgrp_cset_link *link; int dead_cnt = 0, extra_refs = 0, threaded_csets = 0; - spin_lock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + list_for_each_entry(link, &css->cgroup->cset_links, cset_link) { + struct css_set *cset = link->cset; + struct task_struct *task; + int count = 0; + int refcnt = refcount_read(&cset->refcount); + + /* + * Print out the proc_cset and threaded_cset relationship + * and highlight difference between refcount and task_count. + */ + seq_printf(seq, "css_set %pK", cset); + if (rcu_dereference_protected(cset->dom_cset, 1) != cset) { + threaded_csets++; + seq_printf(seq, "=>%pK", cset->dom_cset); + } + if (!list_empty(&cset->threaded_csets)) { + struct css_set *tcset; + int idx = 0; + + list_for_each_entry(tcset, &cset->threaded_csets, + threaded_csets_node) { + seq_puts(seq, idx ? "," : "<="); + seq_printf(seq, "%pK", tcset); + idx++; + } + } else { + seq_printf(seq, " %d", refcnt); + if (refcnt - cset->nr_tasks > 0) { + int extra = refcnt - cset->nr_tasks; + + seq_printf(seq, " +%d", extra); + /* + * Take out the one additional reference in + * init_css_set. + */ + if (cset == &init_css_set) + extra--; + extra_refs += extra; + } + } + seq_puts(seq, "\n"); - list_for_each_entry(link, &css->cgroup->cset_links, cset_link) { - struct css_set *cset = link->cset; - struct task_struct *task; - int count = 0; - int refcnt = refcount_read(&cset->refcount); + list_for_each_entry(task, &cset->tasks, cg_list) { + if (count++ <= MAX_TASKS_SHOWN_PER_CSS) + seq_printf(seq, " task %d\n", + task_pid_vnr(task)); + } - /* - * Print out the proc_cset and threaded_cset relationship - * and highlight difference between refcount and task_count. - */ - seq_printf(seq, "css_set %pK", cset); - if (rcu_dereference_protected(cset->dom_cset, 1) != cset) { - threaded_csets++; - seq_printf(seq, "=>%pK", cset->dom_cset); - } - if (!list_empty(&cset->threaded_csets)) { - struct css_set *tcset; - int idx = 0; - - list_for_each_entry(tcset, &cset->threaded_csets, - threaded_csets_node) { - seq_puts(seq, idx ? "," : "<="); - seq_printf(seq, "%pK", tcset); - idx++; + list_for_each_entry(task, &cset->mg_tasks, cg_list) { + if (count++ <= MAX_TASKS_SHOWN_PER_CSS) + seq_printf(seq, " task %d\n", + task_pid_vnr(task)); } - } else { - seq_printf(seq, " %d", refcnt); - if (refcnt - cset->nr_tasks > 0) { - int extra = refcnt - cset->nr_tasks; - - seq_printf(seq, " +%d", extra); - /* - * Take out the one additional reference in - * init_css_set. - */ - if (cset == &init_css_set) - extra--; - extra_refs += extra; + /* show # of overflowed tasks */ + if (count > MAX_TASKS_SHOWN_PER_CSS) + seq_printf(seq, " ... (%d)\n", + count - MAX_TASKS_SHOWN_PER_CSS); + + if (cset->dead) { + seq_puts(seq, " [dead]\n"); + dead_cnt++; } - } - seq_puts(seq, "\n"); - - list_for_each_entry(task, &cset->tasks, cg_list) { - if (count++ <= MAX_TASKS_SHOWN_PER_CSS) - seq_printf(seq, " task %d\n", - task_pid_vnr(task)); - } - list_for_each_entry(task, &cset->mg_tasks, cg_list) { - if (count++ <= MAX_TASKS_SHOWN_PER_CSS) - seq_printf(seq, " task %d\n", - task_pid_vnr(task)); + WARN_ON(count != cset->nr_tasks); } - /* show # of overflowed tasks */ - if (count > MAX_TASKS_SHOWN_PER_CSS) - seq_printf(seq, " ... (%d)\n", - count - MAX_TASKS_SHOWN_PER_CSS); - - if (cset->dead) { - seq_puts(seq, " [dead]\n"); - dead_cnt++; - } - - WARN_ON(count != cset->nr_tasks); } - spin_unlock_irq(&css_set_lock); if (!dead_cnt && !extra_refs && !threaded_csets) return 0; diff --git a/kernel/cgroup/freezer.c b/kernel/cgroup/freezer.c index bf1690a167dd..01134b3af176 100644 --- a/kernel/cgroup/freezer.c +++ b/kernel/cgroup/freezer.c @@ -108,12 +108,12 @@ void cgroup_enter_frozen(void) if (current->frozen) return; - spin_lock_irq(&css_set_lock); + guard(spinlock_irq)(&css_set_lock); + current->frozen = true; cgrp = task_dfl_cgroup(current); cgroup_inc_frozen_cnt(cgrp); cgroup_update_frozen(cgrp); - spin_unlock_irq(&css_set_lock); } /* @@ -129,7 +129,8 @@ void cgroup_leave_frozen(bool always_leave) { struct cgroup *cgrp; - spin_lock_irq(&css_set_lock); + guard(spinlock_irq)(&css_set_lock); + cgrp = task_dfl_cgroup(current); if (always_leave || !test_bit(CGRP_FREEZE, &cgrp->flags)) { cgroup_dec_frozen_cnt(cgrp); @@ -142,7 +143,6 @@ void cgroup_leave_frozen(bool always_leave) set_thread_flag(TIF_SIGPENDING); spin_unlock(¤t->sighand->siglock); } - spin_unlock_irq(&css_set_lock); } /* @@ -178,12 +178,12 @@ static void cgroup_do_freeze(struct cgroup *cgrp, bool freeze) lockdep_assert_held(&cgroup_mutex); - spin_lock_irq(&css_set_lock); - if (freeze) - set_bit(CGRP_FREEZE, &cgrp->flags); - else - clear_bit(CGRP_FREEZE, &cgrp->flags); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + if (freeze) + set_bit(CGRP_FREEZE, &cgrp->flags); + else + clear_bit(CGRP_FREEZE, &cgrp->flags); + } if (freeze) TRACE_CGROUP_PATH(freeze, cgrp); @@ -206,10 +206,10 @@ static void cgroup_do_freeze(struct cgroup *cgrp, bool freeze) * Cgroup state should be revisited here to cover empty leaf cgroups * and cgroups which descendants are already in the desired state. */ - spin_lock_irq(&css_set_lock); - if (cgrp->nr_descendants == cgrp->freezer.nr_frozen_descendants) - cgroup_update_frozen(cgrp); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + if (cgrp->nr_descendants == cgrp->freezer.nr_frozen_descendants) + cgroup_update_frozen(cgrp); + } } /* diff --git a/kernel/cgroup/namespace.c b/kernel/cgroup/namespace.c index 144a464e45c6..3e2454a106aa 100644 --- a/kernel/cgroup/namespace.c +++ b/kernel/cgroup/namespace.c @@ -71,10 +71,10 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags, return ERR_PTR(-ENOSPC); /* It is not safe to take cgroup_mutex here */ - spin_lock_irq(&css_set_lock); - cset = task_css_set(current); - get_css_set(cset); - spin_unlock_irq(&css_set_lock); + scoped_guard(spinlock_irq, &css_set_lock) { + cset = task_css_set(current); + get_css_set(cset); + } new_ns = alloc_cgroup_ns(); if (IS_ERR(new_ns)) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/3] cgroup: add lock guard support for css_set_lock and rcu 2025-06-06 16:18 ` [PATCH v1 2/3] cgroup: add lock guard support for css_set_lock and rcu Jemmy Wong @ 2025-06-17 9:09 ` Michal Koutný 0 siblings, 0 replies; 15+ messages in thread From: Michal Koutný @ 2025-06-17 9:09 UTC (permalink / raw) To: Jemmy Wong; +Cc: Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel [-- Attachment #1: Type: text/plain, Size: 46207 bytes --] I'd separate css_set_lock and RCU here, they're very different species. On Sat, Jun 07, 2025 at 12:18:40AM +0800, Jemmy Wong <jemmywong512@gmail.com> wrote: > Signed-off-by: Jemmy Wong <jemmywong512@gmail.com> > > --- > kernel/cgroup/cgroup-v1.c | 29 +- > kernel/cgroup/cgroup.c | 580 ++++++++++++++++++-------------------- > kernel/cgroup/cpuset-v1.c | 16 +- > kernel/cgroup/debug.c | 185 ++++++------ > kernel/cgroup/freezer.c | 28 +- > kernel/cgroup/namespace.c | 8 +- > 6 files changed, 401 insertions(+), 445 deletions(-) > > diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c > index f4658eda4445..fcc2d474b470 100644 > --- a/kernel/cgroup/cgroup-v1.c > +++ b/kernel/cgroup/cgroup-v1.c > @@ -70,9 +70,9 @@ int cgroup_attach_task_all(struct task_struct *from, struct task_struct *tsk) > for_each_root(root) { > struct cgroup *from_cgrp; > > - spin_lock_irq(&css_set_lock); > - from_cgrp = task_cgroup_from_root(from, root); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + from_cgrp = task_cgroup_from_root(from, root); > + } > > retval = cgroup_attach_task(from_cgrp, tsk, false); > if (retval) > @@ -117,10 +117,10 @@ int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from) > cgroup_attach_lock(true); > > /* all tasks in @from are being moved, all csets are source */ > - spin_lock_irq(&css_set_lock); > - list_for_each_entry(link, &from->cset_links, cset_link) > - cgroup_migrate_add_src(link->cset, to, &mgctx); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + list_for_each_entry(link, &from->cset_links, cset_link) > + cgroup_migrate_add_src(link->cset, to, &mgctx); > + } > > ret = cgroup_migrate_prepare_dst(&mgctx); > if (ret) > @@ -728,13 +728,12 @@ int cgroupstats_build(struct cgroupstats *stats, struct dentry *dentry) > * @kn->priv's validity. For this and css_tryget_online_from_dir(), > * @kn->priv is RCU safe. Let's do the RCU dancing. > */ > - rcu_read_lock(); > - cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); > - if (!cgrp || !cgroup_tryget(cgrp)) { > - rcu_read_unlock(); > - return -ENOENT; > + scoped_guard(rcu) { > + cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); > + if (!cgrp || !cgroup_tryget(cgrp)) { > + return -ENOENT; > + } As this became a single statement, braces are unnecessary. > } > - rcu_read_unlock(); > > css_task_iter_start(&cgrp->self, 0, &it); > while ((tsk = css_task_iter_next(&it))) { > @@ -1294,7 +1293,7 @@ struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id) > struct cgroup_root *root; > unsigned long flags; > > - rcu_read_lock(); > + guard(rcu)(); > for_each_root(root) { > /* cgroup1 only*/ > if (root == &cgrp_dfl_root) > @@ -1308,7 +1307,7 @@ struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id) > spin_unlock_irqrestore(&css_set_lock, flags); > break; > } > - rcu_read_unlock(); > + > return cgrp; > } > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c > index 54f80afe4f65..46b677a066d1 100644 > --- a/kernel/cgroup/cgroup.c > +++ b/kernel/cgroup/cgroup.c > @@ -631,13 +631,8 @@ int __cgroup_task_count(const struct cgroup *cgrp) > */ > int cgroup_task_count(const struct cgroup *cgrp) > { > - int count; > - > - spin_lock_irq(&css_set_lock); > - count = __cgroup_task_count(cgrp); > - spin_unlock_irq(&css_set_lock); > - > - return count; > + guard(spinlock_irq)(&css_set_lock); > + return __cgroup_task_count(cgrp); > } > > static struct cgroup *kn_priv(struct kernfs_node *kn) > @@ -1202,11 +1197,11 @@ static struct css_set *find_css_set(struct css_set *old_cset, > > /* First see if we already have a cgroup group that matches > * the desired set */ > - spin_lock_irq(&css_set_lock); > - cset = find_existing_css_set(old_cset, cgrp, template); > - if (cset) > - get_css_set(cset); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + cset = find_existing_css_set(old_cset, cgrp, template); > + if (cset) > + get_css_set(cset); > + } Here it could work to add a guard() into find_existing_css_set() instead. > > if (cset) > return cset; > @@ -1238,34 +1233,33 @@ static struct css_set *find_css_set(struct css_set *old_cset, > * find_existing_css_set() */ > memcpy(cset->subsys, template, sizeof(cset->subsys)); > > - spin_lock_irq(&css_set_lock); > - /* Add reference counts and links from the new css_set. */ > - list_for_each_entry(link, &old_cset->cgrp_links, cgrp_link) { > - struct cgroup *c = link->cgrp; > + scoped_guard(spinlock_irq, &css_set_lock) { > + /* Add reference counts and links from the new css_set. */ > + list_for_each_entry(link, &old_cset->cgrp_links, cgrp_link) { > + struct cgroup *c = link->cgrp; > > - if (c->root == cgrp->root) > - c = cgrp; > - link_css_set(&tmp_links, cset, c); > - } > + if (c->root == cgrp->root) > + c = cgrp; > + link_css_set(&tmp_links, cset, c); > + } > > - BUG_ON(!list_empty(&tmp_links)); > + BUG_ON(!list_empty(&tmp_links)); > > - css_set_count++; > + css_set_count++; > > - /* Add @cset to the hash table */ > - key = css_set_hash(cset->subsys); > - hash_add(css_set_table, &cset->hlist, key); > + /* Add @cset to the hash table */ > + key = css_set_hash(cset->subsys); > + hash_add(css_set_table, &cset->hlist, key); > > - for_each_subsys(ss, ssid) { > - struct cgroup_subsys_state *css = cset->subsys[ssid]; > + for_each_subsys(ss, ssid) { > + struct cgroup_subsys_state *css = cset->subsys[ssid]; > > - list_add_tail(&cset->e_cset_node[ssid], > - &css->cgroup->e_csets[ssid]); > - css_get(css); > + list_add_tail(&cset->e_cset_node[ssid], > + &css->cgroup->e_csets[ssid]); > + css_get(css); > + } > } > > - spin_unlock_irq(&css_set_lock); > - > /* > * If @cset should be threaded, look up the matching dom_cset and > * link them up. We first fully initialize @cset then look for the > @@ -1281,11 +1275,11 @@ static struct css_set *find_css_set(struct css_set *old_cset, > return NULL; > } > > - spin_lock_irq(&css_set_lock); > - cset->dom_cset = dcset; > - list_add_tail(&cset->threaded_csets_node, > - &dcset->threaded_csets); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + cset->dom_cset = dcset; > + list_add_tail(&cset->threaded_csets_node, > + &dcset->threaded_csets); > + } > } I admit this part of find_css_set() is less nice after rework :-/ > > return cset; > @@ -1362,16 +1356,14 @@ static void cgroup_destroy_root(struct cgroup_root *root) > * Release all the links from cset_links to this hierarchy's > * root cgroup > */ > - spin_lock_irq(&css_set_lock); > - > - list_for_each_entry_safe(link, tmp_link, &cgrp->cset_links, cset_link) { > - list_del(&link->cset_link); > - list_del(&link->cgrp_link); > - kfree(link); > + scoped_guard(spinlock_irq, &css_set_lock) { > + list_for_each_entry_safe(link, tmp_link, &cgrp->cset_links, cset_link) { > + list_del(&link->cset_link); > + list_del(&link->cgrp_link); > + kfree(link); > + } > } > > - spin_unlock_irq(&css_set_lock); > - > WARN_ON_ONCE(list_empty(&root->root_list)); > list_del_rcu(&root->root_list); > cgroup_root_count--; > @@ -1437,13 +1429,10 @@ current_cgns_cgroup_from_root(struct cgroup_root *root) > > lockdep_assert_held(&css_set_lock); > > - rcu_read_lock(); > - > - cset = current->nsproxy->cgroup_ns->root_cset; > - res = __cset_cgroup_from_root(cset, root); > - > - rcu_read_unlock(); > - > + scoped_guard(rcu) { > + cset = current->nsproxy->cgroup_ns->root_cset; > + res = __cset_cgroup_from_root(cset, root); > + } > /* > * The namespace_sem is held by current, so the root cgroup can't > * be umounted. Therefore, we can ensure that the res is non-NULL. > @@ -1867,25 +1856,25 @@ int rebind_subsystems(struct cgroup_root *dst_root, u16 ss_mask) > rcu_assign_pointer(dcgrp->subsys[ssid], css); > ss->root = dst_root; > > - spin_lock_irq(&css_set_lock); > - css->cgroup = dcgrp; > - WARN_ON(!list_empty(&dcgrp->e_csets[ss->id])); > - list_for_each_entry_safe(cset, cset_pos, &scgrp->e_csets[ss->id], > - e_cset_node[ss->id]) { > - list_move_tail(&cset->e_cset_node[ss->id], > - &dcgrp->e_csets[ss->id]); > - /* > - * all css_sets of scgrp together in same order to dcgrp, > - * patch in-flight iterators to preserve correct iteration. > - * since the iterator is always advanced right away and > - * finished when it->cset_pos meets it->cset_head, so only > - * update it->cset_head is enough here. > - */ > - list_for_each_entry(it, &cset->task_iters, iters_node) > - if (it->cset_head == &scgrp->e_csets[ss->id]) > - it->cset_head = &dcgrp->e_csets[ss->id]; > + scoped_guard(spinlock_irq, &css_set_lock) { > + css->cgroup = dcgrp; > + WARN_ON(!list_empty(&dcgrp->e_csets[ss->id])); > + list_for_each_entry_safe(cset, cset_pos, &scgrp->e_csets[ss->id], > + e_cset_node[ss->id]) { > + list_move_tail(&cset->e_cset_node[ss->id], > + &dcgrp->e_csets[ss->id]); > + /* > + * all css_sets of scgrp together in same order to dcgrp, > + * patch in-flight iterators to preserve correct iteration. > + * since the iterator is always advanced right away and > + * finished when it->cset_pos meets it->cset_head, so only > + * update it->cset_head is enough here. > + */ > + list_for_each_entry(it, &cset->task_iters, iters_node) > + if (it->cset_head == &scgrp->e_csets[ss->id]) > + it->cset_head = &dcgrp->e_csets[ss->id]; > + } > } > - spin_unlock_irq(&css_set_lock); > > /* default hierarchy doesn't enable controllers by default */ > dst_root->subsys_mask |= 1 << ssid; > @@ -1921,10 +1910,10 @@ int cgroup_show_path(struct seq_file *sf, struct kernfs_node *kf_node, > if (!buf) > return -ENOMEM; > > - spin_lock_irq(&css_set_lock); > - ns_cgroup = current_cgns_cgroup_from_root(kf_cgroot); > - len = kernfs_path_from_node(kf_node, ns_cgroup->kn, buf, PATH_MAX); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + ns_cgroup = current_cgns_cgroup_from_root(kf_cgroot); > + len = kernfs_path_from_node(kf_node, ns_cgroup->kn, buf, PATH_MAX); > + } > > if (len == -E2BIG) > len = -ERANGE; > @@ -2175,13 +2164,13 @@ int cgroup_setup_root(struct cgroup_root *root, u16 ss_mask) > * Link the root cgroup in this hierarchy into all the css_set > * objects. > */ > - spin_lock_irq(&css_set_lock); > - hash_for_each(css_set_table, i, cset, hlist) { > - link_css_set(&tmp_links, cset, root_cgrp); > - if (css_set_populated(cset)) > - cgroup_update_populated(root_cgrp, true); > + scoped_guard(spinlock_irq, &css_set_lock) { > + hash_for_each(css_set_table, i, cset, hlist) { > + link_css_set(&tmp_links, cset, root_cgrp); > + if (css_set_populated(cset)) > + cgroup_update_populated(root_cgrp, true); > + } > } > - spin_unlock_irq(&css_set_lock); > > BUG_ON(!list_empty(&root_cgrp->self.children)); > BUG_ON(atomic_read(&root->nr_cgrps) != 1); > @@ -2225,11 +2214,8 @@ int cgroup_do_get_tree(struct fs_context *fc) > struct cgroup *cgrp; > > scoped_guard(cgroup_mutex) { > - spin_lock_irq(&css_set_lock); > - > + guard(spinlock_irq)(&css_set_lock); > cgrp = cset_cgroup_from_root(ctx->ns->root_cset, ctx->root); > - > - spin_unlock_irq(&css_set_lock); > } > > nsdentry = kernfs_node_dentry(cgrp->kn, sb); > @@ -2438,16 +2424,10 @@ int cgroup_path_ns_locked(struct cgroup *cgrp, char *buf, size_t buflen, > int cgroup_path_ns(struct cgroup *cgrp, char *buf, size_t buflen, > struct cgroup_namespace *ns) > { > - int ret; > - > guard(cgroup_mutex)(); > - spin_lock_irq(&css_set_lock); > - > - ret = cgroup_path_ns_locked(cgrp, buf, buflen, ns); > - > - spin_unlock_irq(&css_set_lock); > + guard(spinlock_irq)(&css_set_lock); > > - return ret; > + return cgroup_path_ns_locked(cgrp, buf, buflen, ns); > } > EXPORT_SYMBOL_GPL(cgroup_path_ns); This is an example of good condensation. > > @@ -2629,27 +2609,27 @@ static int cgroup_migrate_execute(struct cgroup_mgctx *mgctx) > * the new cgroup. There are no failure cases after here, so this > * is the commit point. > */ > - spin_lock_irq(&css_set_lock); > - list_for_each_entry(cset, &tset->src_csets, mg_node) { > - list_for_each_entry_safe(task, tmp_task, &cset->mg_tasks, cg_list) { > - struct css_set *from_cset = task_css_set(task); > - struct css_set *to_cset = cset->mg_dst_cset; > - > - get_css_set(to_cset); > - to_cset->nr_tasks++; > - css_set_move_task(task, from_cset, to_cset, true); > - from_cset->nr_tasks--; > - /* > - * If the source or destination cgroup is frozen, > - * the task might require to change its state. > - */ > - cgroup_freezer_migrate_task(task, from_cset->dfl_cgrp, > - to_cset->dfl_cgrp); > - put_css_set_locked(from_cset); > + scoped_guard(spinlock_irq, &css_set_lock) { > + list_for_each_entry(cset, &tset->src_csets, mg_node) { > + list_for_each_entry_safe(task, tmp_task, &cset->mg_tasks, cg_list) { > + struct css_set *from_cset = task_css_set(task); > + struct css_set *to_cset = cset->mg_dst_cset; > + > + get_css_set(to_cset); > + to_cset->nr_tasks++; > + css_set_move_task(task, from_cset, to_cset, true); > + from_cset->nr_tasks--; > + /* > + * If the source or destination cgroup is frozen, > + * the task might require to change its state. > + */ > + cgroup_freezer_migrate_task(task, from_cset->dfl_cgrp, > + to_cset->dfl_cgrp); > + put_css_set_locked(from_cset); > > + } > } > } > - spin_unlock_irq(&css_set_lock); > > /* > * Migration is committed, all target tasks are now on dst_csets. > @@ -2682,13 +2662,13 @@ static int cgroup_migrate_execute(struct cgroup_mgctx *mgctx) > } while_each_subsys_mask(); > } > out_release_tset: > - spin_lock_irq(&css_set_lock); > - list_splice_init(&tset->dst_csets, &tset->src_csets); > - list_for_each_entry_safe(cset, tmp_cset, &tset->src_csets, mg_node) { > - list_splice_tail_init(&cset->mg_tasks, &cset->tasks); > - list_del_init(&cset->mg_node); > + scoped_guard(spinlock_irq, &css_set_lock) { > + list_splice_init(&tset->dst_csets, &tset->src_csets); > + list_for_each_entry_safe(cset, tmp_cset, &tset->src_csets, mg_node) { > + list_splice_tail_init(&cset->mg_tasks, &cset->tasks); > + list_del_init(&cset->mg_node); > + } > } > - spin_unlock_irq(&css_set_lock); > > /* > * Re-initialize the cgroup_taskset structure in case it is reused > @@ -2746,7 +2726,7 @@ void cgroup_migrate_finish(struct cgroup_mgctx *mgctx) > > lockdep_assert_held(&cgroup_mutex); > > - spin_lock_irq(&css_set_lock); > + guard(spinlock_irq)(&css_set_lock); > > list_for_each_entry_safe(cset, tmp_cset, &mgctx->preloaded_src_csets, > mg_src_preload_node) { > @@ -2765,8 +2745,6 @@ void cgroup_migrate_finish(struct cgroup_mgctx *mgctx) > list_del_init(&cset->mg_dst_preload_node); > put_css_set_locked(cset); > } > - > - spin_unlock_irq(&css_set_lock); > } > > /** > @@ -2909,14 +2887,14 @@ int cgroup_migrate(struct task_struct *leader, bool threadgroup, > * section to prevent tasks from being freed while taking the snapshot. > * spin_lock_irq() implies RCU critical section here. > */ > - spin_lock_irq(&css_set_lock); > - task = leader; > - do { > - cgroup_migrate_add_task(task, mgctx); > - if (!threadgroup) > - break; > - } while_each_thread(leader, task); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + task = leader; > + do { > + cgroup_migrate_add_task(task, mgctx); > + if (!threadgroup) > + break; > + } while_each_thread(leader, task); > + } > > return cgroup_migrate_execute(mgctx); > } > @@ -2937,16 +2915,15 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader, > int ret = 0; > > /* look up all src csets */ > - spin_lock_irq(&css_set_lock); > - rcu_read_lock(); > - task = leader; > - do { > - cgroup_migrate_add_src(task_css_set(task), dst_cgrp, &mgctx); > - if (!threadgroup) > - break; > - } while_each_thread(leader, task); > - rcu_read_unlock(); > - spin_unlock_irq(&css_set_lock); As I look at it now, the RCU lock seems pointless here. That'd deserve a separate patch (independent of this guard series). > + scoped_guard(spinlock_irq, &css_set_lock) { > + guard(rcu)(); > + task = leader; > + do { > + cgroup_migrate_add_src(task_css_set(task), dst_cgrp, &mgctx); > + if (!threadgroup) > + break; > + } while_each_thread(leader, task); > + } > > /* prepare dst csets and commit */ > ret = cgroup_migrate_prepare_dst(&mgctx); > @@ -3088,23 +3065,23 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp) > lockdep_assert_held(&cgroup_mutex); > > /* look up all csses currently attached to @cgrp's subtree */ > - spin_lock_irq(&css_set_lock); > - cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) { > - struct cgrp_cset_link *link; > + scoped_guard(spinlock_irq, &css_set_lock) { > + cgroup_for_each_live_descendant_pre(dsct, d_css, cgrp) { > + struct cgrp_cset_link *link; > > - /* > - * As cgroup_update_dfl_csses() is only called by > - * cgroup_apply_control(). The csses associated with the > - * given cgrp will not be affected by changes made to > - * its subtree_control file. We can skip them. > - */ > - if (dsct == cgrp) > - continue; > + /* > + * As cgroup_update_dfl_csses() is only called by > + * cgroup_apply_control(). The csses associated with the > + * given cgrp will not be affected by changes made to > + * its subtree_control file. We can skip them. > + */ > + if (dsct == cgrp) > + continue; > > - list_for_each_entry(link, &dsct->cset_links, cset_link) > - cgroup_migrate_add_src(link->cset, dsct, &mgctx); > + list_for_each_entry(link, &dsct->cset_links, cset_link) > + cgroup_migrate_add_src(link->cset, dsct, &mgctx); > + } > } > - spin_unlock_irq(&css_set_lock); > > /* > * We need to write-lock threadgroup_rwsem while migrating tasks. > @@ -3120,16 +3097,16 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp) > if (ret) > goto out_finish; > > - spin_lock_irq(&css_set_lock); > - list_for_each_entry(src_cset, &mgctx.preloaded_src_csets, > - mg_src_preload_node) { > - struct task_struct *task, *ntask; > + scoped_guard(spinlock_irq, &css_set_lock) { > + list_for_each_entry(src_cset, &mgctx.preloaded_src_csets, > + mg_src_preload_node) { > + struct task_struct *task, *ntask; > > - /* all tasks in src_csets need to be migrated */ > - list_for_each_entry_safe(task, ntask, &src_cset->tasks, cg_list) > - cgroup_migrate_add_task(task, &mgctx); > + /* all tasks in src_csets need to be migrated */ > + list_for_each_entry_safe(task, ntask, &src_cset->tasks, cg_list) > + cgroup_migrate_add_task(task, &mgctx); > + } > } > - spin_unlock_irq(&css_set_lock); > > ret = cgroup_migrate_execute(&mgctx); > out_finish: > @@ -3734,7 +3711,8 @@ static int cgroup_stat_show(struct seq_file *seq, void *v) > * Without proper lock protection, racing is possible. So the > * numbers may not be consistent when that happens. > */ > - rcu_read_lock(); > + guard(rcu)(); > + > for (ssid = 0; ssid < CGROUP_SUBSYS_COUNT; ssid++) { > dying_cnt[ssid] = -1; > if ((BIT(ssid) & cgrp_dfl_inhibit_ss_mask) || > @@ -3753,7 +3731,6 @@ static int cgroup_stat_show(struct seq_file *seq, void *v) > seq_printf(seq, "nr_dying_subsys_%s %d\n", > cgroup_subsys[ssid]->name, dying_cnt[ssid]); > } > - rcu_read_unlock(); > return 0; > } > > @@ -3771,11 +3748,10 @@ static struct cgroup_subsys_state *cgroup_tryget_css(struct cgroup *cgrp, > { > struct cgroup_subsys_state *css; > > - rcu_read_lock(); > + guard(rcu)(); > css = cgroup_css(cgrp, ss); > if (css && !css_tryget_online(css)) > css = NULL; > - rcu_read_unlock(); > > return css; > } > @@ -4056,9 +4032,9 @@ static void __cgroup_kill(struct cgroup *cgrp) > > lockdep_assert_held(&cgroup_mutex); > > - spin_lock_irq(&css_set_lock); > - cgrp->kill_seq++; > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + cgrp->kill_seq++; > + } > > css_task_iter_start(&cgrp->self, CSS_TASK_ITER_PROCS | CSS_TASK_ITER_THREADED, &it); > while ((task = css_task_iter_next(&it))) { > @@ -4187,9 +4163,9 @@ static ssize_t cgroup_file_write(struct kernfs_open_file *of, char *buf, > * doesn't need to be pinned. The RCU locking is not necessary > * either. It's just for the convenience of using cgroup_css(). > */ > - rcu_read_lock(); > - css = cgroup_css(cgrp, cft->ss); > - rcu_read_unlock(); > + scoped_guard(rcu) { > + css = cgroup_css(cgrp, cft->ss); > + } > > if (cft->write_u64) { > unsigned long long v; > @@ -4815,14 +4791,14 @@ bool css_has_online_children(struct cgroup_subsys_state *css) > struct cgroup_subsys_state *child; > bool ret = false; > > - rcu_read_lock(); > + guard(rcu)(); > css_for_each_child(child, css) { > if (child->flags & CSS_ONLINE) { > ret = true; > break; > } > } > - rcu_read_unlock(); > + > return ret; > } > > @@ -5247,9 +5223,9 @@ static ssize_t __cgroup_procs_write(struct kernfs_open_file *of, char *buf, > goto out_unlock; > > /* find the source cgroup */ > - spin_lock_irq(&css_set_lock); > - src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root); > + } > > /* > * Process and thread migrations follow same delegation rule. Check > @@ -5531,11 +5507,11 @@ static inline void css_release_work_fn_locked(struct work_struct *work) > > css_rstat_flush(&cgrp->self); > > - spin_lock_irq(&css_set_lock); > - for (tcgrp = cgroup_parent(cgrp); tcgrp; > - tcgrp = cgroup_parent(tcgrp)) > - tcgrp->nr_dying_descendants--; > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + for (tcgrp = cgroup_parent(cgrp); tcgrp; > + tcgrp = cgroup_parent(tcgrp)) > + tcgrp->nr_dying_descendants--; > + } > > /* > * There are two control paths which try to determine > @@ -5790,20 +5766,20 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name, > goto out_psi_free; > > /* allocation complete, commit to creation */ > - spin_lock_irq(&css_set_lock); > - for (i = 0; i < level; i++) { > - tcgrp = cgrp->ancestors[i]; > - tcgrp->nr_descendants++; > + scoped_guard(spinlock_irq, &css_set_lock) { > + for (i = 0; i < level; i++) { > + tcgrp = cgrp->ancestors[i]; > + tcgrp->nr_descendants++; > > - /* > - * If the new cgroup is frozen, all ancestor cgroups get a new > - * frozen descendant, but their state can't change because of > - * this. > - */ > - if (cgrp->freezer.e_freeze) > - tcgrp->freezer.nr_frozen_descendants++; > + /* > + * If the new cgroup is frozen, all ancestor cgroups get a new > + * frozen descendant, but their state can't change because of > + * this. > + */ > + if (cgrp->freezer.e_freeze) > + tcgrp->freezer.nr_frozen_descendants++; > + } > } > - spin_unlock_irq(&css_set_lock); > > list_add_tail_rcu(&cgrp->self.sibling, &cgroup_parent(cgrp)->self.children); > atomic_inc(&root->nr_cgrps); > @@ -6047,10 +6023,10 @@ static int cgroup_destroy_locked(struct cgroup *cgrp) > */ > cgrp->self.flags &= ~CSS_ONLINE; > > - spin_lock_irq(&css_set_lock); > - list_for_each_entry(link, &cgrp->cset_links, cset_link) > - link->cset->dead = true; > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + list_for_each_entry(link, &cgrp->cset_links, cset_link) > + link->cset->dead = true; > + } > > /* initiate massacre of all css's */ > for_each_css(css, ssid, cgrp) > @@ -6063,18 +6039,18 @@ static int cgroup_destroy_locked(struct cgroup *cgrp) > if (cgroup_is_threaded(cgrp)) > parent->nr_threaded_children--; > > - spin_lock_irq(&css_set_lock); > - for (tcgrp = parent; tcgrp; tcgrp = cgroup_parent(tcgrp)) { > - tcgrp->nr_descendants--; > - tcgrp->nr_dying_descendants++; > - /* > - * If the dying cgroup is frozen, decrease frozen descendants > - * counters of ancestor cgroups. > - */ > - if (test_bit(CGRP_FROZEN, &cgrp->flags)) > - tcgrp->freezer.nr_frozen_descendants--; > + scoped_guard(spinlock_irq, &css_set_lock) { > + for (tcgrp = parent; tcgrp; tcgrp = cgroup_parent(tcgrp)) { > + tcgrp->nr_descendants--; > + tcgrp->nr_dying_descendants++; > + /* > + * If the dying cgroup is frozen, decrease frozen descendants > + * counters of ancestor cgroups. > + */ > + if (test_bit(CGRP_FROZEN, &cgrp->flags)) > + tcgrp->freezer.nr_frozen_descendants--; > + } > } > - spin_unlock_irq(&css_set_lock); > > cgroup1_check_for_release(parent); > > @@ -6356,13 +6332,12 @@ struct cgroup *cgroup_get_from_id(u64 id) > return ERR_PTR(-ENOENT); > } > > - rcu_read_lock(); > - > - cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); > - if (cgrp && !cgroup_tryget(cgrp)) > - cgrp = NULL; > + scoped_guard(rcu) { > + cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); > + if (cgrp && !cgroup_tryget(cgrp)) > + cgrp = NULL; > + } > > - rcu_read_unlock(); > kernfs_put(kn); > > if (!cgrp) > @@ -6539,14 +6514,14 @@ static int cgroup_css_set_fork(struct kernel_clone_args *kargs) > > cgroup_threadgroup_change_begin(current); > > - spin_lock_irq(&css_set_lock); > - cset = task_css_set(current); > - get_css_set(cset); > - if (kargs->cgrp) > - kargs->kill_seq = kargs->cgrp->kill_seq; > - else > - kargs->kill_seq = cset->dfl_cgrp->kill_seq; > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + cset = task_css_set(current); > + get_css_set(cset); > + if (kargs->cgrp) > + kargs->kill_seq = kargs->cgrp->kill_seq; > + else > + kargs->kill_seq = cset->dfl_cgrp->kill_seq; > + } > > if (!(kargs->flags & CLONE_INTO_CGROUP)) { > kargs->cset = cset; > @@ -6736,56 +6711,53 @@ void cgroup_post_fork(struct task_struct *child, > cset = kargs->cset; > kargs->cset = NULL; > > - spin_lock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + /* init tasks are special, only link regular threads */ > + if (likely(child->pid)) { > + if (kargs->cgrp) { > + cgrp_flags = kargs->cgrp->flags; > + cgrp_kill_seq = kargs->cgrp->kill_seq; > + } else { > + cgrp_flags = cset->dfl_cgrp->flags; > + cgrp_kill_seq = cset->dfl_cgrp->kill_seq; > + } > > - /* init tasks are special, only link regular threads */ > - if (likely(child->pid)) { > - if (kargs->cgrp) { > - cgrp_flags = kargs->cgrp->flags; > - cgrp_kill_seq = kargs->cgrp->kill_seq; > + WARN_ON_ONCE(!list_empty(&child->cg_list)); > + cset->nr_tasks++; > + css_set_move_task(child, NULL, cset, false); > } else { > - cgrp_flags = cset->dfl_cgrp->flags; > - cgrp_kill_seq = cset->dfl_cgrp->kill_seq; > + put_css_set(cset); > + cset = NULL; > } > > - WARN_ON_ONCE(!list_empty(&child->cg_list)); > - cset->nr_tasks++; > - css_set_move_task(child, NULL, cset, false); > - } else { > - put_css_set(cset); > - cset = NULL; > - } > - > - if (!(child->flags & PF_KTHREAD)) { > - if (unlikely(test_bit(CGRP_FREEZE, &cgrp_flags))) { > - /* > - * If the cgroup has to be frozen, the new task has > - * too. Let's set the JOBCTL_TRAP_FREEZE jobctl bit to > - * get the task into the frozen state. > - */ > - spin_lock(&child->sighand->siglock); > - WARN_ON_ONCE(child->frozen); > - child->jobctl |= JOBCTL_TRAP_FREEZE; > - spin_unlock(&child->sighand->siglock); > + if (!(child->flags & PF_KTHREAD)) { > + if (unlikely(test_bit(CGRP_FREEZE, &cgrp_flags))) { > + /* > + * If the cgroup has to be frozen, the new task has > + * too. Let's set the JOBCTL_TRAP_FREEZE jobctl bit to > + * get the task into the frozen state. > + */ > + spin_lock(&child->sighand->siglock); > + WARN_ON_ONCE(child->frozen); > + child->jobctl |= JOBCTL_TRAP_FREEZE; > + spin_unlock(&child->sighand->siglock); > + > + /* > + * Calling cgroup_update_frozen() isn't required here, > + * because it will be called anyway a bit later from > + * do_freezer_trap(). So we avoid cgroup's transient > + * switch from the frozen state and back. > + */ > + } > > /* > - * Calling cgroup_update_frozen() isn't required here, > - * because it will be called anyway a bit later from > - * do_freezer_trap(). So we avoid cgroup's transient > - * switch from the frozen state and back. > + * If the cgroup is to be killed notice it now and take the > + * child down right after we finished preparing it for > + * userspace. > */ > + kill = kargs->kill_seq != cgrp_kill_seq; > } > - > - /* > - * If the cgroup is to be killed notice it now and take the > - * child down right after we finished preparing it for > - * userspace. > - */ > - kill = kargs->kill_seq != cgrp_kill_seq; > } > - > - spin_unlock_irq(&css_set_lock); > - > /* > * Call ss->fork(). This must happen after @child is linked on > * css_set; otherwise, @child might change state between ->fork() > @@ -6824,25 +6796,23 @@ void cgroup_exit(struct task_struct *tsk) > struct css_set *cset; > int i; > > - spin_lock_irq(&css_set_lock); > - > - WARN_ON_ONCE(list_empty(&tsk->cg_list)); > - cset = task_css_set(tsk); > - css_set_move_task(tsk, cset, NULL, false); > - cset->nr_tasks--; > - /* matches the signal->live check in css_task_iter_advance() */ > - if (thread_group_leader(tsk) && atomic_read(&tsk->signal->live)) > - list_add_tail(&tsk->cg_list, &cset->dying_tasks); > - > - if (dl_task(tsk)) > - dec_dl_tasks_cs(tsk); > + scoped_guard(spinlock_irq, &css_set_lock) { > + WARN_ON_ONCE(list_empty(&tsk->cg_list)); > + cset = task_css_set(tsk); > + css_set_move_task(tsk, cset, NULL, false); > + cset->nr_tasks--; > + /* matches the signal->live check in css_task_iter_advance() */ > + if (thread_group_leader(tsk) && atomic_read(&tsk->signal->live)) > + list_add_tail(&tsk->cg_list, &cset->dying_tasks); > > - WARN_ON_ONCE(cgroup_task_frozen(tsk)); > - if (unlikely(!(tsk->flags & PF_KTHREAD) && > - test_bit(CGRP_FREEZE, &task_dfl_cgroup(tsk)->flags))) > - cgroup_update_frozen(task_dfl_cgroup(tsk)); > + if (dl_task(tsk)) > + dec_dl_tasks_cs(tsk); > > - spin_unlock_irq(&css_set_lock); > + WARN_ON_ONCE(cgroup_task_frozen(tsk)); > + if (unlikely(!(tsk->flags & PF_KTHREAD) && > + test_bit(CGRP_FREEZE, &task_dfl_cgroup(tsk)->flags))) > + cgroup_update_frozen(task_dfl_cgroup(tsk)); > + } > > /* see cgroup_post_fork() for details */ > do_each_subsys_mask(ss, i, have_exit_callback) { > @@ -6860,10 +6830,9 @@ void cgroup_release(struct task_struct *task) > } while_each_subsys_mask(); > > if (!list_empty(&task->cg_list)) { > - spin_lock_irq(&css_set_lock); > + guard(spinlock_irq)(&css_set_lock); > css_set_skip_task_iters(task_css_set(task), task); > list_del_init(&task->cg_list); > - spin_unlock_irq(&css_set_lock); > } > } > > @@ -6944,7 +6913,7 @@ struct cgroup_subsys_state *css_tryget_online_from_dir(struct dentry *dentry, > !kn || kernfs_type(kn) != KERNFS_DIR) > return ERR_PTR(-EBADF); > > - rcu_read_lock(); > + guard(rcu)(); > > /* > * This path doesn't originate from kernfs and @kn could already > @@ -6958,7 +6927,6 @@ struct cgroup_subsys_state *css_tryget_online_from_dir(struct dentry *dentry, > if (!css || !css_tryget_online(css)) > css = ERR_PTR(-ENOENT); > > - rcu_read_unlock(); > return css; > } > > @@ -7001,13 +6969,11 @@ struct cgroup *cgroup_get_from_path(const char *path) > goto out_kernfs; > } > > - rcu_read_lock(); > - > - cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); > - if (!cgrp || !cgroup_tryget(cgrp)) > - cgrp = ERR_PTR(-ENOENT); > - > - rcu_read_unlock(); > + scoped_guard(rcu) { > + cgrp = rcu_dereference(*(void __rcu __force **)&kn->priv); > + if (!cgrp || !cgroup_tryget(cgrp)) > + cgrp = ERR_PTR(-ENOENT); > + } > > out_kernfs: > kernfs_put(kn); > @@ -7106,28 +7072,28 @@ void cgroup_sk_alloc(struct sock_cgroup_data *skcd) > { > struct cgroup *cgroup; > > - rcu_read_lock(); > - /* Don't associate the sock with unrelated interrupted task's cgroup. */ > - if (in_interrupt()) { > - cgroup = &cgrp_dfl_root.cgrp; > - cgroup_get(cgroup); > - goto out; > - } > + scoped_guard(rcu) { Here could be unscoped guard()(rcu)? > + /* Don't associate the sock with unrelated interrupted task's cgroup. */ > + if (in_interrupt()) { > + cgroup = &cgrp_dfl_root.cgrp; > + cgroup_get(cgroup); > + break; > + } > > - while (true) { > - struct css_set *cset; > + while (true) { > + struct css_set *cset; > > - cset = task_css_set(current); > - if (likely(cgroup_tryget(cset->dfl_cgrp))) { > - cgroup = cset->dfl_cgrp; > - break; > + cset = task_css_set(current); > + if (likely(cgroup_tryget(cset->dfl_cgrp))) { > + cgroup = cset->dfl_cgrp; > + break; > + } > + cpu_relax(); > } > - cpu_relax(); > + > + skcd->cgroup = cgroup; > + cgroup_bpf_get(cgroup); > } > -out: > - skcd->cgroup = cgroup; > - cgroup_bpf_get(cgroup); > - rcu_read_unlock(); > } > > void cgroup_sk_clone(struct sock_cgroup_data *skcd) > diff --git a/kernel/cgroup/cpuset-v1.c b/kernel/cgroup/cpuset-v1.c > index b69a7db67090..114a63432d23 100644 > --- a/kernel/cgroup/cpuset-v1.c > +++ b/kernel/cgroup/cpuset-v1.c > @@ -140,9 +140,8 @@ int cpuset_memory_pressure_enabled __read_mostly; > > void __cpuset_memory_pressure_bump(void) > { > - rcu_read_lock(); > + guard(rcu)(); > fmeter_markevent(&task_cs(current)->fmeter); > - rcu_read_unlock(); > } > > static int update_relax_domain_level(struct cpuset *cs, s64 val) > @@ -393,13 +392,12 @@ int proc_cpuset_show(struct seq_file *m, struct pid_namespace *ns, > if (!buf) > goto out; > > - rcu_read_lock(); > - spin_lock_irq(&css_set_lock); > - css = task_css(tsk, cpuset_cgrp_id); > - retval = cgroup_path_ns_locked(css->cgroup, buf, PATH_MAX, > - current->nsproxy->cgroup_ns); > - spin_unlock_irq(&css_set_lock); > - rcu_read_unlock(); > + scoped_guard(rcu) { > + guard(spinlock_irq)(&css_set_lock); > + css = task_css(tsk, cpuset_cgrp_id); > + retval = cgroup_path_ns_locked(css->cgroup, buf, PATH_MAX, > + current->nsproxy->cgroup_ns); > + } > > if (retval == -E2BIG) > retval = -ENAMETOOLONG; > diff --git a/kernel/cgroup/debug.c b/kernel/cgroup/debug.c > index 80aa3f027ac3..2f04db56c8ac 100644 > --- a/kernel/cgroup/debug.c > +++ b/kernel/cgroup/debug.c > @@ -48,27 +48,26 @@ static int current_css_set_read(struct seq_file *seq, void *v) > if (!cgroup_kn_lock_live(of->kn, false)) > return -ENODEV; > > - spin_lock_irq(&css_set_lock); > - rcu_read_lock(); > - cset = task_css_set(current); > - refcnt = refcount_read(&cset->refcount); > - seq_printf(seq, "css_set %pK %d", cset, refcnt); > - if (refcnt > cset->nr_tasks) > - seq_printf(seq, " +%d", refcnt - cset->nr_tasks); > - seq_puts(seq, "\n"); > + scoped_guard(spinlock_irq, &css_set_lock) { > + guard(rcu)(); > + cset = task_css_set(current); > + refcnt = refcount_read(&cset->refcount); > + seq_printf(seq, "css_set %pK %d", cset, refcnt); > + if (refcnt > cset->nr_tasks) > + seq_printf(seq, " +%d", refcnt - cset->nr_tasks); > + seq_puts(seq, "\n"); > > - /* > - * Print the css'es stored in the current css_set. > - */ > - for_each_subsys(ss, i) { > - css = cset->subsys[ss->id]; > - if (!css) > - continue; > - seq_printf(seq, "%2d: %-4s\t- %p[%d]\n", ss->id, ss->name, > - css, css->id); > + /* > + * Print the css'es stored in the current css_set. > + */ > + for_each_subsys(ss, i) { > + css = cset->subsys[ss->id]; > + if (!css) > + continue; > + seq_printf(seq, "%2d: %-4s\t- %p[%d]\n", ss->id, ss->name, > + css, css->id); > + } > } > - rcu_read_unlock(); > - spin_unlock_irq(&css_set_lock); > cgroup_kn_unlock(of->kn); > return 0; > } > @@ -76,12 +75,8 @@ static int current_css_set_read(struct seq_file *seq, void *v) > static u64 current_css_set_refcount_read(struct cgroup_subsys_state *css, > struct cftype *cft) > { > - u64 count; > - > - rcu_read_lock(); > - count = refcount_read(&task_css_set(current)->refcount); > - rcu_read_unlock(); > - return count; > + guard(rcu)(); > + return refcount_read(&task_css_set(current)->refcount); > } > > static int current_css_set_cg_links_read(struct seq_file *seq, void *v) > @@ -94,18 +89,17 @@ static int current_css_set_cg_links_read(struct seq_file *seq, void *v) > if (!name_buf) > return -ENOMEM; > > - spin_lock_irq(&css_set_lock); > - rcu_read_lock(); > - cset = task_css_set(current); > - list_for_each_entry(link, &cset->cgrp_links, cgrp_link) { > - struct cgroup *c = link->cgrp; > + scoped_guard(spinlock_irq, &css_set_lock) { > + guard(rcu)(); > + cset = task_css_set(current); > + list_for_each_entry(link, &cset->cgrp_links, cgrp_link) { > + struct cgroup *c = link->cgrp; > > - cgroup_name(c, name_buf, NAME_MAX + 1); > - seq_printf(seq, "Root %d group %s\n", > - c->root->hierarchy_id, name_buf); > + cgroup_name(c, name_buf, NAME_MAX + 1); > + seq_printf(seq, "Root %d group %s\n", > + c->root->hierarchy_id, name_buf); > + } > } > - rcu_read_unlock(); > - spin_unlock_irq(&css_set_lock); > kfree(name_buf); > return 0; > } > @@ -117,74 +111,73 @@ static int cgroup_css_links_read(struct seq_file *seq, void *v) > struct cgrp_cset_link *link; > int dead_cnt = 0, extra_refs = 0, threaded_csets = 0; > > - spin_lock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + list_for_each_entry(link, &css->cgroup->cset_links, cset_link) { > + struct css_set *cset = link->cset; > + struct task_struct *task; > + int count = 0; > + int refcnt = refcount_read(&cset->refcount); > + > + /* > + * Print out the proc_cset and threaded_cset relationship > + * and highlight difference between refcount and task_count. > + */ > + seq_printf(seq, "css_set %pK", cset); > + if (rcu_dereference_protected(cset->dom_cset, 1) != cset) { > + threaded_csets++; > + seq_printf(seq, "=>%pK", cset->dom_cset); > + } > + if (!list_empty(&cset->threaded_csets)) { > + struct css_set *tcset; > + int idx = 0; > + > + list_for_each_entry(tcset, &cset->threaded_csets, > + threaded_csets_node) { > + seq_puts(seq, idx ? "," : "<="); > + seq_printf(seq, "%pK", tcset); > + idx++; > + } > + } else { > + seq_printf(seq, " %d", refcnt); > + if (refcnt - cset->nr_tasks > 0) { > + int extra = refcnt - cset->nr_tasks; > + > + seq_printf(seq, " +%d", extra); > + /* > + * Take out the one additional reference in > + * init_css_set. > + */ > + if (cset == &init_css_set) > + extra--; > + extra_refs += extra; > + } > + } > + seq_puts(seq, "\n"); > > - list_for_each_entry(link, &css->cgroup->cset_links, cset_link) { > - struct css_set *cset = link->cset; > - struct task_struct *task; > - int count = 0; > - int refcnt = refcount_read(&cset->refcount); > + list_for_each_entry(task, &cset->tasks, cg_list) { > + if (count++ <= MAX_TASKS_SHOWN_PER_CSS) > + seq_printf(seq, " task %d\n", > + task_pid_vnr(task)); > + } > > - /* > - * Print out the proc_cset and threaded_cset relationship > - * and highlight difference between refcount and task_count. > - */ > - seq_printf(seq, "css_set %pK", cset); > - if (rcu_dereference_protected(cset->dom_cset, 1) != cset) { > - threaded_csets++; > - seq_printf(seq, "=>%pK", cset->dom_cset); > - } > - if (!list_empty(&cset->threaded_csets)) { > - struct css_set *tcset; > - int idx = 0; > - > - list_for_each_entry(tcset, &cset->threaded_csets, > - threaded_csets_node) { > - seq_puts(seq, idx ? "," : "<="); > - seq_printf(seq, "%pK", tcset); > - idx++; > + list_for_each_entry(task, &cset->mg_tasks, cg_list) { > + if (count++ <= MAX_TASKS_SHOWN_PER_CSS) > + seq_printf(seq, " task %d\n", > + task_pid_vnr(task)); > } > - } else { > - seq_printf(seq, " %d", refcnt); > - if (refcnt - cset->nr_tasks > 0) { > - int extra = refcnt - cset->nr_tasks; > - > - seq_printf(seq, " +%d", extra); > - /* > - * Take out the one additional reference in > - * init_css_set. > - */ > - if (cset == &init_css_set) > - extra--; > - extra_refs += extra; > + /* show # of overflowed tasks */ > + if (count > MAX_TASKS_SHOWN_PER_CSS) > + seq_printf(seq, " ... (%d)\n", > + count - MAX_TASKS_SHOWN_PER_CSS); > + > + if (cset->dead) { > + seq_puts(seq, " [dead]\n"); > + dead_cnt++; > } > - } > - seq_puts(seq, "\n"); > - > - list_for_each_entry(task, &cset->tasks, cg_list) { > - if (count++ <= MAX_TASKS_SHOWN_PER_CSS) > - seq_printf(seq, " task %d\n", > - task_pid_vnr(task)); > - } > > - list_for_each_entry(task, &cset->mg_tasks, cg_list) { > - if (count++ <= MAX_TASKS_SHOWN_PER_CSS) > - seq_printf(seq, " task %d\n", > - task_pid_vnr(task)); > + WARN_ON(count != cset->nr_tasks); > } > - /* show # of overflowed tasks */ > - if (count > MAX_TASKS_SHOWN_PER_CSS) > - seq_printf(seq, " ... (%d)\n", > - count - MAX_TASKS_SHOWN_PER_CSS); > - > - if (cset->dead) { > - seq_puts(seq, " [dead]\n"); > - dead_cnt++; > - } > - > - WARN_ON(count != cset->nr_tasks); > } > - spin_unlock_irq(&css_set_lock); > > if (!dead_cnt && !extra_refs && !threaded_csets) > return 0; > diff --git a/kernel/cgroup/freezer.c b/kernel/cgroup/freezer.c > index bf1690a167dd..01134b3af176 100644 > --- a/kernel/cgroup/freezer.c > +++ b/kernel/cgroup/freezer.c > @@ -108,12 +108,12 @@ void cgroup_enter_frozen(void) > if (current->frozen) > return; > > - spin_lock_irq(&css_set_lock); > + guard(spinlock_irq)(&css_set_lock); > + > current->frozen = true; > cgrp = task_dfl_cgroup(current); > cgroup_inc_frozen_cnt(cgrp); > cgroup_update_frozen(cgrp); > - spin_unlock_irq(&css_set_lock); > } > > /* > @@ -129,7 +129,8 @@ void cgroup_leave_frozen(bool always_leave) > { > struct cgroup *cgrp; > > - spin_lock_irq(&css_set_lock); > + guard(spinlock_irq)(&css_set_lock); > + > cgrp = task_dfl_cgroup(current); > if (always_leave || !test_bit(CGRP_FREEZE, &cgrp->flags)) { > cgroup_dec_frozen_cnt(cgrp); > @@ -142,7 +143,6 @@ void cgroup_leave_frozen(bool always_leave) > set_thread_flag(TIF_SIGPENDING); > spin_unlock(¤t->sighand->siglock); > } > - spin_unlock_irq(&css_set_lock); > } > > /* > @@ -178,12 +178,12 @@ static void cgroup_do_freeze(struct cgroup *cgrp, bool freeze) > > lockdep_assert_held(&cgroup_mutex); > > - spin_lock_irq(&css_set_lock); > - if (freeze) > - set_bit(CGRP_FREEZE, &cgrp->flags); > - else > - clear_bit(CGRP_FREEZE, &cgrp->flags); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + if (freeze) > + set_bit(CGRP_FREEZE, &cgrp->flags); > + else > + clear_bit(CGRP_FREEZE, &cgrp->flags); > + } > > if (freeze) > TRACE_CGROUP_PATH(freeze, cgrp); > @@ -206,10 +206,10 @@ static void cgroup_do_freeze(struct cgroup *cgrp, bool freeze) > * Cgroup state should be revisited here to cover empty leaf cgroups > * and cgroups which descendants are already in the desired state. > */ > - spin_lock_irq(&css_set_lock); > - if (cgrp->nr_descendants == cgrp->freezer.nr_frozen_descendants) > - cgroup_update_frozen(cgrp); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + if (cgrp->nr_descendants == cgrp->freezer.nr_frozen_descendants) > + cgroup_update_frozen(cgrp); > + } > } > > /* > diff --git a/kernel/cgroup/namespace.c b/kernel/cgroup/namespace.c > index 144a464e45c6..3e2454a106aa 100644 > --- a/kernel/cgroup/namespace.c > +++ b/kernel/cgroup/namespace.c > @@ -71,10 +71,10 @@ struct cgroup_namespace *copy_cgroup_ns(unsigned long flags, > return ERR_PTR(-ENOSPC); > > /* It is not safe to take cgroup_mutex here */ > - spin_lock_irq(&css_set_lock); > - cset = task_css_set(current); > - get_css_set(cset); > - spin_unlock_irq(&css_set_lock); > + scoped_guard(spinlock_irq, &css_set_lock) { > + cset = task_css_set(current); > + get_css_set(cset); > + } > > new_ns = alloc_cgroup_ns(); > if (IS_ERR(new_ns)) { > -- > 2.43.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 3/3] cgroup: add lock guard support for others 2025-06-06 16:18 [PATCH v1 0/3] cgroup: Add lock guard support Jemmy Wong 2025-06-06 16:18 ` [PATCH v1 1/3] cgroup: add lock guard support for cgroup_muetx Jemmy Wong 2025-06-06 16:18 ` [PATCH v1 2/3] cgroup: add lock guard support for css_set_lock and rcu Jemmy Wong @ 2025-06-06 16:18 ` Jemmy Wong 2025-06-07 10:50 ` kernel test robot 2025-06-17 9:10 ` Michal Koutný 2025-06-09 16:34 ` [PATCH v1 0/3] cgroup: Add lock guard support Tejun Heo 2025-06-17 9:08 ` Michal Koutný 4 siblings, 2 replies; 15+ messages in thread From: Jemmy Wong @ 2025-06-06 16:18 UTC (permalink / raw) To: Michal Koutný Cc: Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel Signed-off-by: Jemmy Wong <jemmywong512@gmail.com> --- kernel/cgroup/cgroup-internal.h | 8 ++-- kernel/cgroup/cgroup-v1.c | 11 +++-- kernel/cgroup/cgroup.c | 73 ++++++++++++--------------------- 3 files changed, 35 insertions(+), 57 deletions(-) diff --git a/kernel/cgroup/cgroup-internal.h b/kernel/cgroup/cgroup-internal.h index b14e61c64a34..5430454d38ca 100644 --- a/kernel/cgroup/cgroup-internal.h +++ b/kernel/cgroup/cgroup-internal.h @@ -198,8 +198,6 @@ void put_css_set_locked(struct css_set *cset); static inline void put_css_set(struct css_set *cset) { - unsigned long flags; - /* * Ensure that the refcount doesn't hit zero while any readers * can see it. Similar to atomic_dec_and_lock(), but for an @@ -208,9 +206,9 @@ static inline void put_css_set(struct css_set *cset) if (refcount_dec_not_one(&cset->refcount)) return; - spin_lock_irqsave(&css_set_lock, flags); - put_css_set_locked(cset); - spin_unlock_irqrestore(&css_set_lock, flags); + scoped_guard(spinlock_irqsave, &css_set_lock) { + put_css_set_locked(cset); + } } /* diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c index fcc2d474b470..91c6ba4e441d 100644 --- a/kernel/cgroup/cgroup-v1.c +++ b/kernel/cgroup/cgroup-v1.c @@ -1291,7 +1291,6 @@ struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id) { struct cgroup *cgrp = ERR_PTR(-ENOENT); struct cgroup_root *root; - unsigned long flags; guard(rcu)(); for_each_root(root) { @@ -1300,11 +1299,11 @@ struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id) continue; if (root->hierarchy_id != hierarchy_id) continue; - spin_lock_irqsave(&css_set_lock, flags); - cgrp = task_cgroup_from_root(tsk, root); - if (!cgrp || !cgroup_tryget(cgrp)) - cgrp = ERR_PTR(-ENOENT); - spin_unlock_irqrestore(&css_set_lock, flags); + scoped_guard(spinlock_irqsave, &css_set_lock) { + cgrp = task_cgroup_from_root(tsk, root); + if (!cgrp || !cgroup_tryget(cgrp)) + cgrp = ERR_PTR(-ENOENT); + } break; } diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index 46b677a066d1..ea98679b01e1 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -335,28 +335,23 @@ static int cgroup_idr_alloc(struct idr *idr, void *ptr, int start, int end, int ret; idr_preload(gfp_mask); - spin_lock_bh(&cgroup_idr_lock); - ret = idr_alloc(idr, ptr, start, end, gfp_mask & ~__GFP_DIRECT_RECLAIM); - spin_unlock_bh(&cgroup_idr_lock); + scoped_guard(spinlock_bh, &cgroup_idr_lock) { + ret = idr_alloc(idr, ptr, start, end, gfp_mask & ~__GFP_DIRECT_RECLAIM); + } idr_preload_end(); return ret; } static void *cgroup_idr_replace(struct idr *idr, void *ptr, int id) { - void *ret; - - spin_lock_bh(&cgroup_idr_lock); - ret = idr_replace(idr, ptr, id); - spin_unlock_bh(&cgroup_idr_lock); - return ret; + guard(spinlock_bh)(&cgroup_idr_lock); + return idr_replace(idr, ptr, id); } static void cgroup_idr_remove(struct idr *idr, int id) { - spin_lock_bh(&cgroup_idr_lock); + guard(spinlock_bh)(&cgroup_idr_lock); idr_remove(idr, id); - spin_unlock_bh(&cgroup_idr_lock); } static bool cgroup_has_tasks(struct cgroup *cgrp) @@ -583,20 +578,19 @@ struct cgroup_subsys_state *cgroup_get_e_css(struct cgroup *cgrp, if (!CGROUP_HAS_SUBSYS_CONFIG) return NULL; - rcu_read_lock(); + guard(rcu)(); do { css = cgroup_css(cgrp, ss); if (css && css_tryget_online(css)) - goto out_unlock; + return css; cgrp = cgroup_parent(cgrp); } while (cgrp); css = init_css_set.subsys[ss->id]; css_get(css); -out_unlock: - rcu_read_unlock(); + return css; } EXPORT_SYMBOL_GPL(cgroup_get_e_css); @@ -1691,9 +1685,9 @@ static void cgroup_rm_file(struct cgroup *cgrp, const struct cftype *cft) struct cgroup_subsys_state *css = cgroup_css(cgrp, cft->ss); struct cgroup_file *cfile = (void *)css + cft->file_offset; - spin_lock_irq(&cgroup_file_kn_lock); - cfile->kn = NULL; - spin_unlock_irq(&cgroup_file_kn_lock); + scoped_guard(spinlock_irq, &cgroup_file_kn_lock) { + cfile->kn = NULL; + } timer_delete_sync(&cfile->notify_timer); } @@ -4277,9 +4271,9 @@ static int cgroup_add_file(struct cgroup_subsys_state *css, struct cgroup *cgrp, timer_setup(&cfile->notify_timer, cgroup_file_notify_timer, 0); - spin_lock_irq(&cgroup_file_kn_lock); - cfile->kn = kn; - spin_unlock_irq(&cgroup_file_kn_lock); + scoped_guard(spinlock_irq, &cgroup_file_kn_lock) { + cfile->kn = kn; + } } return 0; @@ -4534,9 +4528,7 @@ int cgroup_add_legacy_cftypes(struct cgroup_subsys *ss, struct cftype *cfts) */ void cgroup_file_notify(struct cgroup_file *cfile) { - unsigned long flags; - - spin_lock_irqsave(&cgroup_file_kn_lock, flags); + guard(spinlock_irqsave)(&cgroup_file_kn_lock); if (cfile->kn) { unsigned long last = cfile->notified_at; unsigned long next = last + CGROUP_FILE_NOTIFY_MIN_INTV; @@ -4548,7 +4540,6 @@ void cgroup_file_notify(struct cgroup_file *cfile) cfile->notified_at = jiffies; } } - spin_unlock_irqrestore(&cgroup_file_kn_lock, flags); } /** @@ -4560,10 +4551,10 @@ void cgroup_file_show(struct cgroup_file *cfile, bool show) { struct kernfs_node *kn; - spin_lock_irq(&cgroup_file_kn_lock); - kn = cfile->kn; - kernfs_get(kn); - spin_unlock_irq(&cgroup_file_kn_lock); + scoped_guard(spinlock_irq, &cgroup_file_kn_lock) { + kn = cfile->kn; + kernfs_get(kn); + } if (kn) kernfs_show(kn, show); @@ -4987,11 +4978,10 @@ static void css_task_iter_advance(struct css_task_iter *it) void css_task_iter_start(struct cgroup_subsys_state *css, unsigned int flags, struct css_task_iter *it) { - unsigned long irqflags; memset(it, 0, sizeof(*it)); - spin_lock_irqsave(&css_set_lock, irqflags); + guard(spinlock_irqsave)(&css_set_lock); it->ss = css->ss; it->flags = flags; @@ -5004,8 +4994,6 @@ void css_task_iter_start(struct cgroup_subsys_state *css, unsigned int flags, it->cset_head = it->cset_pos; css_task_iter_advance(it); - - spin_unlock_irqrestore(&css_set_lock, irqflags); } /** @@ -5018,14 +5006,12 @@ void css_task_iter_start(struct cgroup_subsys_state *css, unsigned int flags, */ struct task_struct *css_task_iter_next(struct css_task_iter *it) { - unsigned long irqflags; - if (it->cur_task) { put_task_struct(it->cur_task); it->cur_task = NULL; } - spin_lock_irqsave(&css_set_lock, irqflags); + guard(spinlock_irqsave)(&css_set_lock); /* @it may be half-advanced by skips, finish advancing */ if (it->flags & CSS_TASK_ITER_SKIPPED) @@ -5038,8 +5024,6 @@ struct task_struct *css_task_iter_next(struct css_task_iter *it) css_task_iter_advance(it); } - spin_unlock_irqrestore(&css_set_lock, irqflags); - return it->cur_task; } @@ -5051,13 +5035,10 @@ struct task_struct *css_task_iter_next(struct css_task_iter *it) */ void css_task_iter_end(struct css_task_iter *it) { - unsigned long irqflags; - if (it->cur_cset) { - spin_lock_irqsave(&css_set_lock, irqflags); + guard(spinlock_irqsave)(&css_set_lock); list_del(&it->iters_node); put_css_set_locked(it->cur_cset); - spin_unlock_irqrestore(&css_set_lock, irqflags); } if (it->cur_dcset) @@ -6737,10 +6718,10 @@ void cgroup_post_fork(struct task_struct *child, * too. Let's set the JOBCTL_TRAP_FREEZE jobctl bit to * get the task into the frozen state. */ - spin_lock(&child->sighand->siglock); - WARN_ON_ONCE(child->frozen); - child->jobctl |= JOBCTL_TRAP_FREEZE; - spin_unlock(&child->sighand->siglock); + scoped_guard(spinlock, &child->sighand->siglock) { + WARN_ON_ONCE(child->frozen); + child->jobctl |= JOBCTL_TRAP_FREEZE; + } /* * Calling cgroup_update_frozen() isn't required here, -- 2.43.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 3/3] cgroup: add lock guard support for others 2025-06-06 16:18 ` [PATCH v1 3/3] cgroup: add lock guard support for others Jemmy Wong @ 2025-06-07 10:50 ` kernel test robot 2025-06-08 8:52 ` Jemmy Wong 2025-06-17 9:10 ` Michal Koutný 1 sibling, 1 reply; 15+ messages in thread From: kernel test robot @ 2025-06-07 10:50 UTC (permalink / raw) To: Jemmy Wong, Michal Koutný Cc: oe-kbuild-all, Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel Hi Jemmy, kernel test robot noticed the following build warnings: [auto build test WARNING on tj-cgroup/for-next] [also build test WARNING on linus/master next-20250606] [cannot apply to v6.15] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Jemmy-Wong/cgroup-add-lock-guard-support-for-cgroup_muetx/20250607-002109 base: https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-next patch link: https://lore.kernel.org/r/20250606161841.44354-4-jemmywong512%40gmail.com patch subject: [PATCH v1 3/3] cgroup: add lock guard support for others config: sparc-randconfig-r121-20250607 (https://download.01.org/0day-ci/archive/20250607/202506071822.ERv4i34r-lkp@intel.com/config) compiler: sparc-linux-gcc (GCC) 8.5.0 reproduce: (https://download.01.org/0day-ci/archive/20250607/202506071822.ERv4i34r-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202506071822.ERv4i34r-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> kernel/cgroup/cgroup.c:6721:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *l @@ got struct spinlock [noderef] __rcu * @@ kernel/cgroup/cgroup.c:6721:33: sparse: expected struct spinlock [usertype] *l kernel/cgroup/cgroup.c:6721:33: sparse: got struct spinlock [noderef] __rcu * kernel/cgroup/cgroup.c:345:13: sparse: sparse: context imbalance in 'cgroup_idr_replace' - wrong count at exit kernel/cgroup/cgroup.c:351:13: sparse: sparse: context imbalance in 'cgroup_idr_remove' - wrong count at exit kernel/cgroup/cgroup.c:626:5: sparse: sparse: context imbalance in 'cgroup_task_count' - wrong count at exit kernel/cgroup/cgroup.c:2225:9: sparse: sparse: context imbalance in 'cgroup_do_get_tree' - different lock contexts for basic block kernel/cgroup/cgroup.c:2418:5: sparse: sparse: context imbalance in 'cgroup_path_ns' - wrong count at exit kernel/cgroup/cgroup.c:2734:9: sparse: sparse: context imbalance in 'cgroup_migrate_finish' - wrong count at exit kernel/cgroup/cgroup.c:3131:9: sparse: sparse: context imbalance in 'cgroup_lock_and_drain_offline' - wrong count at exit kernel/cgroup/cgroup.c:4532:9: sparse: sparse: context imbalance in 'cgroup_file_notify' - wrong count at exit kernel/cgroup/cgroup.c:4994:9: sparse: sparse: context imbalance in 'css_task_iter_start' - wrong count at exit kernel/cgroup/cgroup.c:5027:9: sparse: sparse: context imbalance in 'css_task_iter_next' - wrong count at exit kernel/cgroup/cgroup.c:5047:9: sparse: sparse: context imbalance in 'css_task_iter_end' - wrong count at exit kernel/cgroup/cgroup.c:6485:12: sparse: sparse: context imbalance in 'cgroup_css_set_fork' - wrong count at exit kernel/cgroup/cgroup.c:6601:9: sparse: sparse: context imbalance in 'cgroup_css_set_put_fork' - wrong count at exit kernel/cgroup/cgroup.c:6621:5: sparse: sparse: context imbalance in 'cgroup_can_fork' - wrong count at exit kernel/cgroup/cgroup.c:6670:9: sparse: sparse: context imbalance in 'cgroup_cancel_fork' - unexpected unlock kernel/cgroup/cgroup.c:6813:9: sparse: sparse: context imbalance in 'cgroup_release' - different lock contexts for basic block vim +6721 kernel/cgroup/cgroup.c 6672 6673 /** 6674 * cgroup_post_fork - finalize cgroup setup for the child process 6675 * @child: the child process 6676 * @kargs: the arguments passed to create the child process 6677 * 6678 * Attach the child process to its css_set calling the subsystem fork() 6679 * callbacks. 6680 */ 6681 void cgroup_post_fork(struct task_struct *child, 6682 struct kernel_clone_args *kargs) 6683 __releases(&cgroup_threadgroup_rwsem) __releases(&cgroup_mutex) 6684 { 6685 unsigned int cgrp_kill_seq = 0; 6686 unsigned long cgrp_flags = 0; 6687 bool kill = false; 6688 struct cgroup_subsys *ss; 6689 struct css_set *cset; 6690 int i; 6691 6692 cset = kargs->cset; 6693 kargs->cset = NULL; 6694 6695 scoped_guard(spinlock_irq, &css_set_lock) { 6696 /* init tasks are special, only link regular threads */ 6697 if (likely(child->pid)) { 6698 if (kargs->cgrp) { 6699 cgrp_flags = kargs->cgrp->flags; 6700 cgrp_kill_seq = kargs->cgrp->kill_seq; 6701 } else { 6702 cgrp_flags = cset->dfl_cgrp->flags; 6703 cgrp_kill_seq = cset->dfl_cgrp->kill_seq; 6704 } 6705 6706 WARN_ON_ONCE(!list_empty(&child->cg_list)); 6707 cset->nr_tasks++; 6708 css_set_move_task(child, NULL, cset, false); 6709 } else { 6710 put_css_set(cset); 6711 cset = NULL; 6712 } 6713 6714 if (!(child->flags & PF_KTHREAD)) { 6715 if (unlikely(test_bit(CGRP_FREEZE, &cgrp_flags))) { 6716 /* 6717 * If the cgroup has to be frozen, the new task has 6718 * too. Let's set the JOBCTL_TRAP_FREEZE jobctl bit to 6719 * get the task into the frozen state. 6720 */ > 6721 scoped_guard(spinlock, &child->sighand->siglock) { 6722 WARN_ON_ONCE(child->frozen); 6723 child->jobctl |= JOBCTL_TRAP_FREEZE; 6724 } 6725 6726 /* 6727 * Calling cgroup_update_frozen() isn't required here, 6728 * because it will be called anyway a bit later from 6729 * do_freezer_trap(). So we avoid cgroup's transient 6730 * switch from the frozen state and back. 6731 */ 6732 } 6733 6734 /* 6735 * If the cgroup is to be killed notice it now and take the 6736 * child down right after we finished preparing it for 6737 * userspace. 6738 */ 6739 kill = kargs->kill_seq != cgrp_kill_seq; 6740 } 6741 } 6742 /* 6743 * Call ss->fork(). This must happen after @child is linked on 6744 * css_set; otherwise, @child might change state between ->fork() 6745 * and addition to css_set. 6746 */ 6747 do_each_subsys_mask(ss, i, have_fork_callback) { 6748 ss->fork(child); 6749 } while_each_subsys_mask(); 6750 6751 /* Make the new cset the root_cset of the new cgroup namespace. */ 6752 if (kargs->flags & CLONE_NEWCGROUP) { 6753 struct css_set *rcset = child->nsproxy->cgroup_ns->root_cset; 6754 6755 get_css_set(cset); 6756 child->nsproxy->cgroup_ns->root_cset = cset; 6757 put_css_set(rcset); 6758 } 6759 6760 /* Cgroup has to be killed so take down child immediately. */ 6761 if (unlikely(kill)) 6762 do_send_sig_info(SIGKILL, SEND_SIG_NOINFO, child, PIDTYPE_TGID); 6763 6764 cgroup_css_set_put_fork(kargs); 6765 } 6766 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 3/3] cgroup: add lock guard support for others 2025-06-07 10:50 ` kernel test robot @ 2025-06-08 8:52 ` Jemmy Wong 0 siblings, 0 replies; 15+ messages in thread From: Jemmy Wong @ 2025-06-08 8:52 UTC (permalink / raw) To: kernel test robot, peterz, dan.j.williams, mingo Cc: Jemmy, Michal Koutný, oe-kbuild-all, Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel Hi Peter, Dam, Ingo, Do you have any recommendations to eliminate this false positive, given that sparse recognizes scoped_guard but not guard? Looks like false positives. Some of these warnings were already present due to asymmetric lock/unlock behavior in a specific function, while others were introduced by the use of guard, which can be resolved by replacing it with scoped_guard, but I don’t like this solution which could add additional indentation. —— Previous warnings - caused by asymmetric lock/unlock: kernel/cgroup/cgroup.c:3161:9: sparse: warning: context imbalance in 'cgroup_lock_and_drain_offline' - wrong count at exit kernel/cgroup/cgroup.c:6530:12: sparse: warning: context imbalance in 'cgroup_css_set_fork' - wrong count at exit kernel/cgroup/cgroup.c:6646:9: sparse: warning: context imbalance in 'cgroup_css_set_put_fork' - wrong count at exit kernel/cgroup/cgroup.c:6666:5: sparse: warning: context imbalance in 'cgroup_can_fork' - wrong count at exit kernel/cgroup/cgroup.c:6715:9: sparse: warning: context imbalance in 'cgroup_cancel_fork' - unexpected unlock New warnings - caused by guard: kernel/cgroup/cgroup.c:345:13: sparse: sparse: context imbalance in 'cgroup_idr_replace' - wrong count at exit kernel/cgroup/cgroup.c:351:13: sparse: sparse: context imbalance in 'cgroup_idr_remove' - wrong count at exit kernel/cgroup/cgroup.c:626:5: sparse: sparse: context imbalance in 'cgroup_task_count' - wrong count at exit kernel/cgroup/cgroup.c:2225:9: sparse: sparse: context imbalance in 'cgroup_do_get_tree' - different lock contexts for basic block kernel/cgroup/cgroup.c:2418:5: sparse: sparse: context imbalance in 'cgroup_path_ns' - wrong count at exit kernel/cgroup/cgroup.c:2734:9: sparse: sparse: context imbalance in 'cgroup_migrate_finish' - wrong count at exit kernel/cgroup/cgroup.c:4532:9: sparse: sparse: context imbalance in 'cgroup_file_notify' - wrong count at exit kernel/cgroup/cgroup.c:4994:9: sparse: sparse: context imbalance in 'css_task_iter_start' - wrong count at exit kernel/cgroup/cgroup.c:5027:9: sparse: sparse: context imbalance in 'css_task_iter_next' - wrong count at exit kernel/cgroup/cgroup.c:5047:9: sparse: sparse: context imbalance in 'css_task_iter_end' - wrong count at exit Best, Jemmy > On Jun 7, 2025, at 6:50 PM, kernel test robot <lkp@intel.com> wrote: > > Hi Jemmy, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on tj-cgroup/for-next] > [also build test WARNING on linus/master next-20250606] > [cannot apply to v6.15] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Jemmy-Wong/cgroup-add-lock-guard-support-for-cgroup_muetx/20250607-002109 > base: https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-next > patch link: https://lore.kernel.org/r/20250606161841.44354-4-jemmywong512%40gmail.com > patch subject: [PATCH v1 3/3] cgroup: add lock guard support for others > config: sparc-randconfig-r121-20250607 (https://download.01.org/0day-ci/archive/20250607/202506071822.ERv4i34r-lkp@intel.com/config) > compiler: sparc-linux-gcc (GCC) 8.5.0 > reproduce: (https://download.01.org/0day-ci/archive/20250607/202506071822.ERv4i34r-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202506071822.ERv4i34r-lkp@intel.com/ > > sparse warnings: (new ones prefixed by >>) >>> kernel/cgroup/cgroup.c:6721:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *l @@ got struct spinlock [noderef] __rcu * @@ > kernel/cgroup/cgroup.c:6721:33: sparse: expected struct spinlock [usertype] *l > kernel/cgroup/cgroup.c:6721:33: sparse: got struct spinlock [noderef] __rcu * > kernel/cgroup/cgroup.c:345:13: sparse: sparse: context imbalance in 'cgroup_idr_replace' - wrong count at exit > kernel/cgroup/cgroup.c:351:13: sparse: sparse: context imbalance in 'cgroup_idr_remove' - wrong count at exit > kernel/cgroup/cgroup.c:626:5: sparse: sparse: context imbalance in 'cgroup_task_count' - wrong count at exit > kernel/cgroup/cgroup.c:2225:9: sparse: sparse: context imbalance in 'cgroup_do_get_tree' - different lock contexts for basic block > kernel/cgroup/cgroup.c:2418:5: sparse: sparse: context imbalance in 'cgroup_path_ns' - wrong count at exit > kernel/cgroup/cgroup.c:2734:9: sparse: sparse: context imbalance in 'cgroup_migrate_finish' - wrong count at exit > kernel/cgroup/cgroup.c:3131:9: sparse: sparse: context imbalance in 'cgroup_lock_and_drain_offline' - wrong count at exit > kernel/cgroup/cgroup.c:4532:9: sparse: sparse: context imbalance in 'cgroup_file_notify' - wrong count at exit > kernel/cgroup/cgroup.c:4994:9: sparse: sparse: context imbalance in 'css_task_iter_start' - wrong count at exit > kernel/cgroup/cgroup.c:5027:9: sparse: sparse: context imbalance in 'css_task_iter_next' - wrong count at exit > kernel/cgroup/cgroup.c:5047:9: sparse: sparse: context imbalance in 'css_task_iter_end' - wrong count at exit > kernel/cgroup/cgroup.c:6485:12: sparse: sparse: context imbalance in 'cgroup_css_set_fork' - wrong count at exit > kernel/cgroup/cgroup.c:6601:9: sparse: sparse: context imbalance in 'cgroup_css_set_put_fork' - wrong count at exit > kernel/cgroup/cgroup.c:6621:5: sparse: sparse: context imbalance in 'cgroup_can_fork' - wrong count at exit > kernel/cgroup/cgroup.c:6670:9: sparse: sparse: context imbalance in 'cgroup_cancel_fork' - unexpected unlock > kernel/cgroup/cgroup.c:6813:9: sparse: sparse: context imbalance in 'cgroup_release' - different lock contexts for basic block > > vim +6721 kernel/cgroup/cgroup.c > > 6672 > 6673 /** > 6674 * cgroup_post_fork - finalize cgroup setup for the child process > 6675 * @child: the child process > 6676 * @kargs: the arguments passed to create the child process > 6677 * > 6678 * Attach the child process to its css_set calling the subsystem fork() > 6679 * callbacks. > 6680 */ > 6681 void cgroup_post_fork(struct task_struct *child, > 6682 struct kernel_clone_args *kargs) > 6683 __releases(&cgroup_threadgroup_rwsem) __releases(&cgroup_mutex) > 6684 { > 6685 unsigned int cgrp_kill_seq = 0; > 6686 unsigned long cgrp_flags = 0; > 6687 bool kill = false; > 6688 struct cgroup_subsys *ss; > 6689 struct css_set *cset; > 6690 int i; > 6691 > 6692 cset = kargs->cset; > 6693 kargs->cset = NULL; > 6694 > 6695 scoped_guard(spinlock_irq, &css_set_lock) { > 6696 /* init tasks are special, only link regular threads */ > 6697 if (likely(child->pid)) { > 6698 if (kargs->cgrp) { > 6699 cgrp_flags = kargs->cgrp->flags; > 6700 cgrp_kill_seq = kargs->cgrp->kill_seq; > 6701 } else { > 6702 cgrp_flags = cset->dfl_cgrp->flags; > 6703 cgrp_kill_seq = cset->dfl_cgrp->kill_seq; > 6704 } > 6705 > 6706 WARN_ON_ONCE(!list_empty(&child->cg_list)); > 6707 cset->nr_tasks++; > 6708 css_set_move_task(child, NULL, cset, false); > 6709 } else { > 6710 put_css_set(cset); > 6711 cset = NULL; > 6712 } > 6713 > 6714 if (!(child->flags & PF_KTHREAD)) { > 6715 if (unlikely(test_bit(CGRP_FREEZE, &cgrp_flags))) { > 6716 /* > 6717 * If the cgroup has to be frozen, the new task has > 6718 * too. Let's set the JOBCTL_TRAP_FREEZE jobctl bit to > 6719 * get the task into the frozen state. > 6720 */ >> 6721 scoped_guard(spinlock, &child->sighand->siglock) { > 6722 WARN_ON_ONCE(child->frozen); > 6723 child->jobctl |= JOBCTL_TRAP_FREEZE; > 6724 } > 6725 > 6726 /* > 6727 * Calling cgroup_update_frozen() isn't required here, > 6728 * because it will be called anyway a bit later from > 6729 * do_freezer_trap(). So we avoid cgroup's transient > 6730 * switch from the frozen state and back. > 6731 */ > 6732 } > 6733 > 6734 /* > 6735 * If the cgroup is to be killed notice it now and take the > 6736 * child down right after we finished preparing it for > 6737 * userspace. > 6738 */ > 6739 kill = kargs->kill_seq != cgrp_kill_seq; > 6740 } > 6741 } > 6742 /* > 6743 * Call ss->fork(). This must happen after @child is linked on > 6744 * css_set; otherwise, @child might change state between ->fork() > 6745 * and addition to css_set. > 6746 */ > 6747 do_each_subsys_mask(ss, i, have_fork_callback) { > 6748 ss->fork(child); > 6749 } while_each_subsys_mask(); > 6750 > 6751 /* Make the new cset the root_cset of the new cgroup namespace. */ > 6752 if (kargs->flags & CLONE_NEWCGROUP) { > 6753 struct css_set *rcset = child->nsproxy->cgroup_ns->root_cset; > 6754 > 6755 get_css_set(cset); > 6756 child->nsproxy->cgroup_ns->root_cset = cset; > 6757 put_css_set(rcset); > 6758 } > 6759 > 6760 /* Cgroup has to be killed so take down child immediately. */ > 6761 if (unlikely(kill)) > 6762 do_send_sig_info(SIGKILL, SEND_SIG_NOINFO, child, PIDTYPE_TGID); > 6763 > 6764 cgroup_css_set_put_fork(kargs); > 6765 } > 6766 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 3/3] cgroup: add lock guard support for others 2025-06-06 16:18 ` [PATCH v1 3/3] cgroup: add lock guard support for others Jemmy Wong 2025-06-07 10:50 ` kernel test robot @ 2025-06-17 9:10 ` Michal Koutný 1 sibling, 0 replies; 15+ messages in thread From: Michal Koutný @ 2025-06-17 9:10 UTC (permalink / raw) To: Jemmy Wong; +Cc: Tejun Heo, Johannes Weiner, Waiman Long, cgroups, linux-kernel [-- Attachment #1: Type: text/plain, Size: 8798 bytes --] On Sat, Jun 07, 2025 at 12:18:41AM +0800, Jemmy Wong <jemmywong512@gmail.com> wrote: > Signed-off-by: Jemmy Wong <jemmywong512@gmail.com> > > --- > kernel/cgroup/cgroup-internal.h | 8 ++-- > kernel/cgroup/cgroup-v1.c | 11 +++-- > kernel/cgroup/cgroup.c | 73 ++++++++++++--------------------- > 3 files changed, 35 insertions(+), 57 deletions(-) > > diff --git a/kernel/cgroup/cgroup-internal.h b/kernel/cgroup/cgroup-internal.h > index b14e61c64a34..5430454d38ca 100644 > --- a/kernel/cgroup/cgroup-internal.h > +++ b/kernel/cgroup/cgroup-internal.h > @@ -198,8 +198,6 @@ void put_css_set_locked(struct css_set *cset); > > static inline void put_css_set(struct css_set *cset) > { > - unsigned long flags; > - > /* > * Ensure that the refcount doesn't hit zero while any readers > * can see it. Similar to atomic_dec_and_lock(), but for an > @@ -208,9 +206,9 @@ static inline void put_css_set(struct css_set *cset) > if (refcount_dec_not_one(&cset->refcount)) > return; > > - spin_lock_irqsave(&css_set_lock, flags); > - put_css_set_locked(cset); > - spin_unlock_irqrestore(&css_set_lock, flags); > + scoped_guard(spinlock_irqsave, &css_set_lock) { > + put_css_set_locked(cset); > + } > } This should go to the css_set_lock patch. > /* > diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c > index fcc2d474b470..91c6ba4e441d 100644 > --- a/kernel/cgroup/cgroup-v1.c > +++ b/kernel/cgroup/cgroup-v1.c > @@ -1291,7 +1291,6 @@ struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id) > { > struct cgroup *cgrp = ERR_PTR(-ENOENT); > struct cgroup_root *root; > - unsigned long flags; > > guard(rcu)(); > for_each_root(root) { > @@ -1300,11 +1299,11 @@ struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id) > continue; > if (root->hierarchy_id != hierarchy_id) > continue; > - spin_lock_irqsave(&css_set_lock, flags); > - cgrp = task_cgroup_from_root(tsk, root); > - if (!cgrp || !cgroup_tryget(cgrp)) > - cgrp = ERR_PTR(-ENOENT); > - spin_unlock_irqrestore(&css_set_lock, flags); > + scoped_guard(spinlock_irqsave, &css_set_lock) { > + cgrp = task_cgroup_from_root(tsk, root); > + if (!cgrp || !cgroup_tryget(cgrp)) > + cgrp = ERR_PTR(-ENOENT); > + } Ditto. > break; > } > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c > index 46b677a066d1..ea98679b01e1 100644 > --- a/kernel/cgroup/cgroup.c > +++ b/kernel/cgroup/cgroup.c > @@ -335,28 +335,23 @@ static int cgroup_idr_alloc(struct idr *idr, void *ptr, int start, int end, > int ret; > > idr_preload(gfp_mask); > - spin_lock_bh(&cgroup_idr_lock); > - ret = idr_alloc(idr, ptr, start, end, gfp_mask & ~__GFP_DIRECT_RECLAIM); > - spin_unlock_bh(&cgroup_idr_lock); > + scoped_guard(spinlock_bh, &cgroup_idr_lock) { > + ret = idr_alloc(idr, ptr, start, end, gfp_mask & ~__GFP_DIRECT_RECLAIM); > + } > idr_preload_end(); > return ret; > } > > static void *cgroup_idr_replace(struct idr *idr, void *ptr, int id) > { > - void *ret; > - > - spin_lock_bh(&cgroup_idr_lock); > - ret = idr_replace(idr, ptr, id); > - spin_unlock_bh(&cgroup_idr_lock); > - return ret; > + guard(spinlock_bh)(&cgroup_idr_lock); > + return idr_replace(idr, ptr, id); > } > > static void cgroup_idr_remove(struct idr *idr, int id) > { > - spin_lock_bh(&cgroup_idr_lock); > + guard(spinlock_bh)(&cgroup_idr_lock); > idr_remove(idr, id); > - spin_unlock_bh(&cgroup_idr_lock); > } > > static bool cgroup_has_tasks(struct cgroup *cgrp) > @@ -583,20 +578,19 @@ struct cgroup_subsys_state *cgroup_get_e_css(struct cgroup *cgrp, > if (!CGROUP_HAS_SUBSYS_CONFIG) > return NULL; > > - rcu_read_lock(); > + guard(rcu)(); > > do { > css = cgroup_css(cgrp, ss); > > if (css && css_tryget_online(css)) > - goto out_unlock; > + return css; > cgrp = cgroup_parent(cgrp); > } while (cgrp); > > css = init_css_set.subsys[ss->id]; > css_get(css); > -out_unlock: > - rcu_read_unlock(); > + > return css; > } > EXPORT_SYMBOL_GPL(cgroup_get_e_css); This should go to the RCU patch. > @@ -1691,9 +1685,9 @@ static void cgroup_rm_file(struct cgroup *cgrp, const struct cftype *cft) > struct cgroup_subsys_state *css = cgroup_css(cgrp, cft->ss); > struct cgroup_file *cfile = (void *)css + cft->file_offset; > > - spin_lock_irq(&cgroup_file_kn_lock); > - cfile->kn = NULL; > - spin_unlock_irq(&cgroup_file_kn_lock); > + scoped_guard(spinlock_irq, &cgroup_file_kn_lock) { > + cfile->kn = NULL; > + } > > timer_delete_sync(&cfile->notify_timer); > } > @@ -4277,9 +4271,9 @@ static int cgroup_add_file(struct cgroup_subsys_state *css, struct cgroup *cgrp, > > timer_setup(&cfile->notify_timer, cgroup_file_notify_timer, 0); > > - spin_lock_irq(&cgroup_file_kn_lock); > - cfile->kn = kn; > - spin_unlock_irq(&cgroup_file_kn_lock); > + scoped_guard(spinlock_irq, &cgroup_file_kn_lock) { > + cfile->kn = kn; > + } > } > > return 0; > @@ -4534,9 +4528,7 @@ int cgroup_add_legacy_cftypes(struct cgroup_subsys *ss, struct cftype *cfts) > */ > void cgroup_file_notify(struct cgroup_file *cfile) > { > - unsigned long flags; > - > - spin_lock_irqsave(&cgroup_file_kn_lock, flags); > + guard(spinlock_irqsave)(&cgroup_file_kn_lock); > if (cfile->kn) { > unsigned long last = cfile->notified_at; > unsigned long next = last + CGROUP_FILE_NOTIFY_MIN_INTV; > @@ -4548,7 +4540,6 @@ void cgroup_file_notify(struct cgroup_file *cfile) > cfile->notified_at = jiffies; > } > } > - spin_unlock_irqrestore(&cgroup_file_kn_lock, flags); > } > > /** > @@ -4560,10 +4551,10 @@ void cgroup_file_show(struct cgroup_file *cfile, bool show) > { > struct kernfs_node *kn; > > - spin_lock_irq(&cgroup_file_kn_lock); > - kn = cfile->kn; > - kernfs_get(kn); > - spin_unlock_irq(&cgroup_file_kn_lock); > + scoped_guard(spinlock_irq, &cgroup_file_kn_lock) { > + kn = cfile->kn; > + kernfs_get(kn); > + } > > if (kn) > kernfs_show(kn, show); > @@ -4987,11 +4978,10 @@ static void css_task_iter_advance(struct css_task_iter *it) > void css_task_iter_start(struct cgroup_subsys_state *css, unsigned int flags, > struct css_task_iter *it) > { > - unsigned long irqflags; > > memset(it, 0, sizeof(*it)); > > - spin_lock_irqsave(&css_set_lock, irqflags); > + guard(spinlock_irqsave)(&css_set_lock); > > it->ss = css->ss; > it->flags = flags; > @@ -5004,8 +4994,6 @@ void css_task_iter_start(struct cgroup_subsys_state *css, unsigned int flags, > it->cset_head = it->cset_pos; > > css_task_iter_advance(it); > - > - spin_unlock_irqrestore(&css_set_lock, irqflags); > } > css_set_lock > /** > @@ -5018,14 +5006,12 @@ void css_task_iter_start(struct cgroup_subsys_state *css, unsigned int flags, > */ > struct task_struct *css_task_iter_next(struct css_task_iter *it) > { > - unsigned long irqflags; > - > if (it->cur_task) { > put_task_struct(it->cur_task); > it->cur_task = NULL; > } > > - spin_lock_irqsave(&css_set_lock, irqflags); > + guard(spinlock_irqsave)(&css_set_lock); > > /* @it may be half-advanced by skips, finish advancing */ > if (it->flags & CSS_TASK_ITER_SKIPPED) > @@ -5038,8 +5024,6 @@ struct task_struct *css_task_iter_next(struct css_task_iter *it) > css_task_iter_advance(it); > } > > - spin_unlock_irqrestore(&css_set_lock, irqflags); > - > return it->cur_task; > } > css_set_lock > @@ -5051,13 +5035,10 @@ struct task_struct *css_task_iter_next(struct css_task_iter *it) > */ > void css_task_iter_end(struct css_task_iter *it) > { > - unsigned long irqflags; > - > if (it->cur_cset) { > - spin_lock_irqsave(&css_set_lock, irqflags); > + guard(spinlock_irqsave)(&css_set_lock); > list_del(&it->iters_node); > put_css_set_locked(it->cur_cset); > - spin_unlock_irqrestore(&css_set_lock, irqflags); > } > > if (it->cur_dcset) css_set_lock > @@ -6737,10 +6718,10 @@ void cgroup_post_fork(struct task_struct *child, > * too. Let's set the JOBCTL_TRAP_FREEZE jobctl bit to > * get the task into the frozen state. > */ > - spin_lock(&child->sighand->siglock); > - WARN_ON_ONCE(child->frozen); > - child->jobctl |= JOBCTL_TRAP_FREEZE; > - spin_unlock(&child->sighand->siglock); > + scoped_guard(spinlock, &child->sighand->siglock) { > + WARN_ON_ONCE(child->frozen); > + child->jobctl |= JOBCTL_TRAP_FREEZE; > + } > > /* > * Calling cgroup_update_frozen() isn't required here, > -- > 2.43.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 0/3] cgroup: Add lock guard support 2025-06-06 16:18 [PATCH v1 0/3] cgroup: Add lock guard support Jemmy Wong ` (2 preceding siblings ...) 2025-06-06 16:18 ` [PATCH v1 3/3] cgroup: add lock guard support for others Jemmy Wong @ 2025-06-09 16:34 ` Tejun Heo 2025-06-17 9:08 ` Michal Koutný 4 siblings, 0 replies; 15+ messages in thread From: Tejun Heo @ 2025-06-09 16:34 UTC (permalink / raw) To: Jemmy Wong Cc: Michal Koutný, Johannes Weiner, Waiman Long, cgroups, linux-kernel On Sat, Jun 07, 2025 at 12:18:38AM +0800, Jemmy Wong wrote: > v1 changes: > - remove guard support for BPF > - split patch into parts > > v0 link: > https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/ > > Jemmy Wong (3): > cgroup: add lock guard support for cgroup_muetx > cgroup: add lock guard support for css_set_lock and rcu > cgroup: add lock guard support for others So, I'm rather ambivalent about this patchset but leaning towards not applying them. The lock guards are fine but I'm not sure what converting the existing code base wholesale buys us. We're already pretty good at detecting locking problems with lockdep and all and the code being modified hasn't seen significant locking changes in ages. There are no practical benefits to converting the code base at this point. Thanks. -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 0/3] cgroup: Add lock guard support 2025-06-06 16:18 [PATCH v1 0/3] cgroup: Add lock guard support Jemmy Wong ` (3 preceding siblings ...) 2025-06-09 16:34 ` [PATCH v1 0/3] cgroup: Add lock guard support Tejun Heo @ 2025-06-17 9:08 ` Michal Koutný 2025-06-20 10:45 ` Jemmy Wong 4 siblings, 1 reply; 15+ messages in thread From: Michal Koutný @ 2025-06-17 9:08 UTC (permalink / raw) To: Jemmy Wong, Tejun Heo, Johannes Weiner; +Cc: Waiman Long, cgroups, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2290 bytes --] Hello. I understand why this might have been controversial but I'm surprised it remains so when the lock guards are already used in the kernel code. Documentation/process/coding-style.rst isn't partisan in either way. Johannes: > heebeejeebees - it's asymmetric and critical sections don't stand out > visually at all. - I'd say that's the point of it for functions that are made to manipulate data structures under the lock. Not to spoil the code. - Yes, it's a different coding style that one has to get used to. > extra indentation - deeper indentation == deeper locking, wary of that - although I admit, in some cases of the reworked code, it's overly deep Further reasoning is laid out in include/linux/cleanup.h. I noticed there exists no_free_ptr() macro and that suggests an idea for analogous no_exit_class() (or unguard()) macro (essential an unlock + signal for compiler to not call cleanup function after this BB). Tejun: > There are no practical benefits to converting the code base at this point. I'd expect future backports (into such code) to be more robust wrt pairing errors. At the same time this is also my biggest concern about this change, the wide-spread diff would make current backporting more difficult. (But I'd counter argue that one should think forward here.) Further benefits I see: - it is fewer lines (code is to the point), - reliable cleanup paths, - it is modern and evolution step forward (given such constructs eventually emerge in various languages). On Sat, Jun 07, 2025 at 12:18:38AM +0800, Jemmy Wong <jemmywong512@gmail.com> wrote: > v1 changes: > - remove guard support for BPF > - split patch into parts > > v0 link: > https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/ > > Jemmy Wong (3): > cgroup: add lock guard support for cgroup_muetx > cgroup: add lock guard support for css_set_lock and rcu > cgroup: add lock guard support for others As for the series in general - I'm still in favor of pursuing it (with remarks to individual patches), - it's a good opportunity to also to append sparse __acquires/__releases cleanup to it (see also [1]). Regards, Michal [1] https://lore.kernel.org/r/Z_6z2-qqLI7dbl8h@slm.duckdns.org [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 0/3] cgroup: Add lock guard support 2025-06-17 9:08 ` Michal Koutný @ 2025-06-20 10:45 ` Jemmy Wong 2025-06-21 2:52 ` Tejun Heo 0 siblings, 1 reply; 15+ messages in thread From: Jemmy Wong @ 2025-06-20 10:45 UTC (permalink / raw) To: Michal Koutný, tj, hannes; +Cc: Jemmy, Waiman Long, cgroups, linux-kernel Hi Michal, Tejun, Johannes, Thank you, Michal, for supporting this modernization effort to adopt guard constructs. With 3,326 instances already in use across the kernel upstream, guards offer improved safety and readability. I look forward to working with the community to integrate them into cgroup and welcome feedback to ensure a smooth transition. Best, Jemmy > On Jun 17, 2025, at 5:08 PM, Michal Koutný <mkoutny@suse.com> wrote: > > Hello. > > I understand why this might have been controversial but I'm surprised it > remains so when the lock guards are already used in the kernel code. > Documentation/process/coding-style.rst isn't partisan in either way. > > Johannes: >> heebeejeebees - it's asymmetric and critical sections don't stand out >> visually at all. > - I'd say that's the point of it for functions that are made to > manipulate data structures under the lock. Not to spoil the code. > - Yes, it's a different coding style that one has to get used to. > >> extra indentation > - deeper indentation == deeper locking, wary of that > - although I admit, in some cases of the reworked code, it's overly deep > > Further reasoning is laid out in include/linux/cleanup.h. I noticed > there exists no_free_ptr() macro and that suggests an idea for analogous > no_exit_class() (or unguard()) macro (essential an unlock + signal for > compiler to not call cleanup function after this BB). > > Tejun: >> There are no practical benefits to converting the code base at this point. > > I'd expect future backports (into such code) to be more robust wrt > pairing errors. > At the same time this is also my biggest concern about this change, the > wide-spread diff would make current backporting more difficult. (But > I'd counter argue that one should think forward here.) > > > Further benefits I see: > - it is fewer lines (code is to the point), > - reliable cleanup paths, > - it is modern and evolution step forward (given such constructs > eventually emerge in various languages). > > > On Sat, Jun 07, 2025 at 12:18:38AM +0800, Jemmy Wong <jemmywong512@gmail.com> wrote: >> v1 changes: >> - remove guard support for BPF >> - split patch into parts >> >> v0 link: >> https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/ >> >> Jemmy Wong (3): >> cgroup: add lock guard support for cgroup_muetx >> cgroup: add lock guard support for css_set_lock and rcu >> cgroup: add lock guard support for others > > As for the series in general > - I'm still in favor of pursuing it (with remarks to individual > patches), > - it's a good opportunity to also to append sparse __acquires/__releases > cleanup to it (see also [1]). > > Regards, > Michal > > [1] https://lore.kernel.org/r/Z_6z2-qqLI7dbl8h@slm.duckdns.org > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 0/3] cgroup: Add lock guard support 2025-06-20 10:45 ` Jemmy Wong @ 2025-06-21 2:52 ` Tejun Heo 2025-06-23 14:03 ` Johannes Weiner 0 siblings, 1 reply; 15+ messages in thread From: Tejun Heo @ 2025-06-21 2:52 UTC (permalink / raw) To: Jemmy Wong; +Cc: Michal Koutný, hannes, Waiman Long, cgroups, linux-kernel On Fri, Jun 20, 2025 at 06:45:54PM +0800, Jemmy Wong wrote: ... > > Tejun: > >> There are no practical benefits to converting the code base at this point. > > > > I'd expect future backports (into such code) to be more robust wrt > > pairing errors. > > At the same time this is also my biggest concern about this change, the > > wide-spread diff would make current backporting more difficult. (But > > I'd counter argue that one should think forward here.) Well, I'm not necessarily against it but I generally dislike wholesale cleanups which create big patch application boundaries. If there are enough practical benefits, sure, we should do it, but when it's things like this - maybe possibly it's a bit better in the long term - the calculus isn't clear cut. People can argue these things to high heavens on abstract grounds, but if you break it down to practical gains vs. costs, it's not a huge difference. But, again, I'm not against it. Johannes, any second thoughts? Thanks. -- tejun ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 0/3] cgroup: Add lock guard support 2025-06-21 2:52 ` Tejun Heo @ 2025-06-23 14:03 ` Johannes Weiner 2025-06-30 17:39 ` Michal Koutný 0 siblings, 1 reply; 15+ messages in thread From: Johannes Weiner @ 2025-06-23 14:03 UTC (permalink / raw) To: Tejun Heo Cc: Jemmy Wong, Michal Koutný, Waiman Long, cgroups, linux-kernel On Fri, Jun 20, 2025 at 04:52:03PM -1000, Tejun Heo wrote: > On Fri, Jun 20, 2025 at 06:45:54PM +0800, Jemmy Wong wrote: > ... > > > Tejun: > > >> There are no practical benefits to converting the code base at this point. > > > > > > I'd expect future backports (into such code) to be more robust wrt > > > pairing errors. > > > At the same time this is also my biggest concern about this change, the > > > wide-spread diff would make current backporting more difficult. (But > > > I'd counter argue that one should think forward here.) > > Well, I'm not necessarily against it but I generally dislike wholesale > cleanups which create big patch application boundaries. If there are enough > practical benefits, sure, we should do it, but when it's things like this - > maybe possibly it's a bit better in the long term - the calculus isn't clear > cut. People can argue these things to high heavens on abstract grounds, but > if you break it down to practical gains vs. costs, it's not a huge > difference. > > But, again, I'm not against it. Johannes, any second thoughts? Yeah, letting the primitives get used organically in new code and patches sounds better to me than retrofitting it into an existing function graph that wasn't designed with these in mind. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 0/3] cgroup: Add lock guard support 2025-06-23 14:03 ` Johannes Weiner @ 2025-06-30 17:39 ` Michal Koutný 0 siblings, 0 replies; 15+ messages in thread From: Michal Koutný @ 2025-06-30 17:39 UTC (permalink / raw) To: Johannes Weiner; +Cc: Tejun Heo, Jemmy Wong, Waiman Long, cgroups, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1064 bytes --] On Mon, Jun 23, 2025 at 04:03:21PM +0200, Johannes Weiner <hannes@cmpxchg.org> wrote: > > People can argue these things to high heavens on abstract grounds, > > but if you break it down to practical gains vs. costs, it's not a > > huge difference. This makes it sound like we were discussing tabs-vs-spaces (at least I perceive there are more benefits of guard locks ;-)) (I also believe that the encouraged separation per lock (locking type) would allow easier backporting of this transformation.) > > But, again, I'm not against it. Johannes, any second thoughts? > > Yeah, letting the primitives get used organically in new code and > patches sounds better to me than retrofitting it into an existing > function graph that wasn't designed with these in mind. But OK, it sounds there's no objection against combining *_lock calling- and guarded code at one time, so in the future the ratio of those two may be more favorable for one-time switch to the latter. I thank Jemmy for giving the preview of the transformation. Michal [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-06-30 17:39 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-06 16:18 [PATCH v1 0/3] cgroup: Add lock guard support Jemmy Wong 2025-06-06 16:18 ` [PATCH v1 1/3] cgroup: add lock guard support for cgroup_muetx Jemmy Wong 2025-06-17 9:09 ` Michal Koutný 2025-06-06 16:18 ` [PATCH v1 2/3] cgroup: add lock guard support for css_set_lock and rcu Jemmy Wong 2025-06-17 9:09 ` Michal Koutný 2025-06-06 16:18 ` [PATCH v1 3/3] cgroup: add lock guard support for others Jemmy Wong 2025-06-07 10:50 ` kernel test robot 2025-06-08 8:52 ` Jemmy Wong 2025-06-17 9:10 ` Michal Koutný 2025-06-09 16:34 ` [PATCH v1 0/3] cgroup: Add lock guard support Tejun Heo 2025-06-17 9:08 ` Michal Koutný 2025-06-20 10:45 ` Jemmy Wong 2025-06-21 2:52 ` Tejun Heo 2025-06-23 14:03 ` Johannes Weiner 2025-06-30 17:39 ` Michal Koutný
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).