| Message ID | 20240420094428.1028477-1-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] cgroup: Introduce css_is_online() helper | expand |
On Sat 20-04-24 09:44:28, Xiu Jianfeng wrote: > Introduce css_is_online() helper to test if whether the specified > css is online, avoid testing css.flags with CSS_ONLINE directly > outside of cgroup.c. > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> One style nit below: > +/* > + * css_is_online - test whether the specified css is online > + * @css: target css > + */ > +static inline bool css_is_online(struct cgroup_subsys_state *css) > +{ > + return !!(css->flags & CSS_ONLINE); > +} Since the return type is 'bool', you don't need the !! magic in the statement above. Honza
Hello, On Tue, Apr 23, 2024 at 03:49:23PM +0200, Jan Kara wrote: > On Sat 20-04-24 09:44:28, Xiu Jianfeng wrote: > > Introduce css_is_online() helper to test if whether the specified > > css is online, avoid testing css.flags with CSS_ONLINE directly > > outside of cgroup.c. > > > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > > Looks good. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> I'm a bit skeptical about these trivial helpers. If the test is something more involved or has complications which need documentation (e.g. regarding synchronization and what not), the helper would be useful even if it's just as a place to centrally document what's going on. However, here, it's just testing one flag and I'm not sure what benefits the helper brings. Thanks.
On Tue 23-04-24 05:56:38, Tejun Heo wrote: > Hello, > > On Tue, Apr 23, 2024 at 03:49:23PM +0200, Jan Kara wrote: > > On Sat 20-04-24 09:44:28, Xiu Jianfeng wrote: > > > Introduce css_is_online() helper to test if whether the specified > > > css is online, avoid testing css.flags with CSS_ONLINE directly > > > outside of cgroup.c. > > > > > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > > > > Looks good. Feel free to add: > > > > Reviewed-by: Jan Kara <jack@suse.cz> > > I'm a bit skeptical about these trivial helpers. If the test is something > more involved or has complications which need documentation (e.g. regarding > synchronization and what not), the helper would be useful even if it's just > as a place to centrally document what's going on. However, here, it's just > testing one flag and I'm not sure what benefits the helper brings. Yeah OK. I felt the motivation was so that writeback code doesn't have to peek into cgroup "internals" so I'm fine with the change from writeback POV. But if you don't see the point from cgroup side than sure I'm fine without this change as well. Honza
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 92a5b8283528..bb84c6a2fa8e 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -916,7 +916,7 @@ void wbc_account_cgroup_owner(struct writeback_control *wbc, struct page *page, folio = page_folio(page); css = mem_cgroup_css_from_folio(folio); /* dead cgroups shouldn't contribute to inode ownership arbitration */ - if (!(css->flags & CSS_ONLINE)) + if (!css_is_online(css)) return; id = css->id; diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index 2150ca60394b..e6b6f3418da8 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -346,6 +346,15 @@ static inline bool css_is_dying(struct cgroup_subsys_state *css) return !(css->flags & CSS_NO_REF) && percpu_ref_is_dying(&css->refcnt); } +/* + * css_is_online - test whether the specified css is online + * @css: target css + */ +static inline bool css_is_online(struct cgroup_subsys_state *css) +{ + return !!(css->flags & CSS_ONLINE); +} + static inline void cgroup_get(struct cgroup *cgrp) { css_get(&cgrp->self); diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 8f332b4ae84c..cd6b3bfd070f 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -939,7 +939,7 @@ static inline bool mem_cgroup_online(struct mem_cgroup *memcg) { if (mem_cgroup_disabled()) return true; - return !!(memcg->css.flags & CSS_ONLINE); + return css_is_online(&memcg->css); } void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list lru, diff --git a/kernel/cgroup/cgroup-internal.h b/kernel/cgroup/cgroup-internal.h index 520b90dd97ec..feeaf172844d 100644 --- a/kernel/cgroup/cgroup-internal.h +++ b/kernel/cgroup/cgroup-internal.h @@ -183,7 +183,7 @@ extern struct list_head cgroup_roots; static inline bool cgroup_is_dead(const struct cgroup *cgrp) { - return !(cgrp->self.flags & CSS_ONLINE); + return !css_is_online(&cgrp->self); } static inline bool notify_on_release(const struct cgroup *cgrp) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 7703ced535a3..e77e9e1911e6 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -405,7 +405,7 @@ ino_t page_cgroup_ino(struct page *page) /* page_folio() is racy here, but the entire function is racy anyway */ memcg = folio_memcg_check(page_folio(page)); - while (memcg && !(memcg->css.flags & CSS_ONLINE)) + while (memcg && !css_is_online(&memcg->css)) memcg = parent_mem_cgroup(memcg); if (memcg) ino = cgroup_ino(memcg->css.cgroup); diff --git a/mm/page_owner.c b/mm/page_owner.c index 75c23302868a..7accb25e6fe6 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -523,7 +523,7 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret, if (!memcg) goto out_unlock; - online = (memcg->css.flags & CSS_ONLINE); + online = css_is_online(&memcg->css); cgroup_name(memcg->css.cgroup, name, sizeof(name)); ret += scnprintf(kbuf + ret, count - ret, "Charged %sto %smemcg %s\n",
Introduce css_is_online() helper to test if whether the specified css is online, avoid testing css.flags with CSS_ONLINE directly outside of cgroup.c. Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> --- fs/fs-writeback.c | 2 +- include/linux/cgroup.h | 9 +++++++++ include/linux/memcontrol.h | 2 +- kernel/cgroup/cgroup-internal.h | 2 +- mm/memcontrol.c | 2 +- mm/page_owner.c | 2 +- 6 files changed, 14 insertions(+), 5 deletions(-)