On 3/20/25 2:44 PM, Tejun Heo wrote:
Hello,
On Wed, Mar 19, 2025 at 03:21:50PM -0700, JP Kobryn wrote:
The cgroup_rstat_cpu struct contains rstat node pointers and also the base
stat objects. Since ownership of the cgroup_rstat_cpu has shifted from
cgroup to cgroup_subsys_state, css's other than cgroup::self are now
carrying along these base stat objects which go unused. Eliminate this
wasted memory by splitting up cgroup_rstat_cpu into two separate structs.
The cgroup_rstat_cpu struct is modified in a way that it now contains only
the rstat node pointers. css's that are associated with a subsystem
(memory, io) use this compact struct to participate in rstat without the
memory overhead of the base stat objects.
As for css's represented by cgroup::self, a new cgroup_rstat_base_cpu
struct is introduced. It contains the new compact cgroup_rstat_cpu struct
as its first field followed by the base stat objects.
Because the rstat pointers exist at the same offset (beginning) in both
structs, cgroup_subsys_state is modified to contain a union of the two
structs. Where css initialization is done, the compact struct is allocated
when the css is associated with a subsystem. When the css is not associated
with a subsystem, the full struct is allocated. The union allows the
existing rstat updated/flush routines to work with any css regardless of
subsystem association. The base stats routines however, were modified to
access the full struct field in the union.
Can you do this as a part of prep patch? ie. Move the bstat out of rstat_cpu
into the containing cgroup before switching to css based structure? It's
rather odd to claim memory saving after bloating it up due to patch
sequencing.
I think it's possible. I'll give my thoughts in response to your
question below.
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 0ffc8438c6d9..f9b84e7f718d 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -170,7 +170,10 @@ struct cgroup_subsys_state {
struct percpu_ref refcnt;
/* per-cpu recursive resource statistics */
- struct css_rstat_cpu __percpu *rstat_cpu;
+ union {
+ struct css_rstat_cpu __percpu *rstat_cpu;
+ struct css_rstat_base_cpu __percpu *rstat_base_cpu;
+ };
I have a bit of difficult time following this. All that bstat is is the
counters that the cgroup itself carries regardless of the subsystem but they
would be collected and flushed the same way. Wouldn't that mean that the
cgroup css should carry the same css_rstat_cpu as other csses but would have
separate struct to carry the counters? Why is it multiplexed on
css_rstat_cpu?
Originally the idea came from wanting to avoid having extra objects
(even if just one pointer) in the css that would not be used. This setup
allowed for what felt like (to me at least) an organized way of keeping
the objects used in rstat in the same container (css). You initialize
everything within the css and also have consistency with the css-based
rstat API's - using the css for updated/flushed allows you to get the
needed rstat objects via css. So if we moved the base stat objects to
the cgroup, we could where applicable check for css_is_cgroup() and then
use css->cgroup to get the base stats which we kind of do anyway. I
don't see a problem there. The only complication I think is on the init
side, we would have to perform a separate percpu allocation for these
base stats in the cgroup. I can give this a try for next rev unless
anyone feels otherwise.
In general, I'll wait to see if Yosry, Michal, or Shakeel want any other
changes before sending out the next rev.
Thanks.
