On Mon, Mar 03, 2025 at 04:22:42PM +0100, Michal Koutný wrote:
> On Thu, Feb 27, 2025 at 01:55:42PM -0800, inwardvessel <inwardvessel@xxxxxxxxx> wrote:
> > From: JP Kobryn <inwardvessel@xxxxxxxxx>
> ...
> > +static inline bool is_base_css(struct cgroup_subsys_state *css)
> > +{
> > + return css->ss == NULL;
> > +}
>
> Similar predicate is also used in cgroup.c (various cgroup vs subsys
> lifecycle functions, e.g. css_free_rwork_fn()). I think it'd better
> unified, i.e. open code the predicate here or use the helper in both
> cases (css_is_cgroup() or similar).
>
> > void __init cgroup_rstat_boot(void)
> > {
> > - int cpu;
> > + struct cgroup_subsys *ss;
> > + int cpu, ssid;
> >
> > - for_each_possible_cpu(cpu)
> > - raw_spin_lock_init(per_cpu_ptr(&cgroup_rstat_cpu_lock, cpu));
> > + for_each_subsys(ss, ssid) {
> > + spin_lock_init(&cgroup_rstat_subsys_lock[ssid]);
> > + }
>
> Hm, with this loop I realize it may be worth putting this lock into
> struct cgroup_subsys_state and initializing them in
> cgroup_init_subsys() to keep all per-subsys data in one pack.
I thought about this, but this would have unnecessary memory overhead as
we only need one lock per-subsystem. So having a lock in every single
css is wasteful.
Maybe we can put the lock in struct cgroup_subsys? Then we can still
initialize them in cgroup_init_subsys().
>
> > +
> > + for_each_possible_cpu(cpu) {
> > + raw_spin_lock_init(per_cpu_ptr(&cgroup_rstat_base_cpu_lock, cpu));
> > +
> > + for_each_subsys(ss, ssid) {
> > + raw_spin_lock_init(
> > + per_cpu_ptr(cgroup_rstat_subsys_cpu_lock, cpu) + ssid);
> > + }
>
> Similar here, and keep cgroup_rstat_boot() for the base locks only.
I think it will be confusing to have cgroup_rstat_boot() only initialize
some of the locks.
I think if we initialize the subsys locks in cgroup_init_subsys(), then
we should open code initializing the base locks in cgroup_init(), and
remove cgroup_rstat_boot().
Alternatively, we can make cgroup_rstat_boot() take in a subsys and
initialize its lock. If we pass NULL, then it initialize the base locks.
In this case we can call cgroup_rstat_boot() for each subsystem that has
an rstat callback in cgroup_init() (or cgroup_init_subsys()), and then
once for the base locks.
WDYT?
