| Message ID | 1577450633-2098-1-git-send-email-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, memcg: reduce size of struct mem_cgroup by using bit field | expand |
On Fri, Dec 27, 2019 at 07:43:52AM -0500, Yafang Shao wrote: > There are some members in struct mem_group can be either 0(false) or > 1(true), so we can define them using bit field to reduce size. With this > patch, the size of struct mem_cgroup can be reduced by 64 bytes in theory, > but as there're some MEMCG_PADDING()s, the real number may be different, > which is relate with the cacheline size. Anyway, this patch could reduce > the size of struct mem_cgroup more or less. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Cc: Roman Gushchin <guro@fb.com> > --- > include/linux/memcontrol.h | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index a7a0a1a5..f68a9ef 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -229,20 +229,26 @@ struct mem_cgroup { > /* > * Should the accounting and control be hierarchical, per subtree? > */ > - bool use_hierarchy; > + unsigned int use_hierarchy : 1; > + > + /* Legacy tcp memory accounting */ > + unsigned int tcpmem_active : 1; > + unsigned int tcpmem_pressure : 1; > > /* > * Should the OOM killer kill all belonging tasks, had it kill one? > */ > - bool oom_group; > + unsigned int oom_group : 1; > > /* protected by memcg_oom_lock */ > - bool oom_lock; > - int under_oom; > + unsigned int oom_lock : 1; Hm, looking at the original code, it was clear that oom_lock and under_oom are protected with memcg_oom_lock; but not oom_kill_disable. This information seems to be lost. Also, I'd look at the actual memory savings. Is it worth it? Or it's all eaten by the padding. Thanks! > > - int swappiness; > /* OOM-Killer disable */ > - int oom_kill_disable; > + unsigned int oom_kill_disable : 1; > + > + int under_oom; > + > + int swappiness; > > /* memory.events and memory.events.local */ > struct cgroup_file events_file; > @@ -297,9 +303,6 @@ struct mem_cgroup { > > unsigned long socket_pressure; > > - /* Legacy tcp memory accounting */ > - bool tcpmem_active; > - int tcpmem_pressure; > > #ifdef CONFIG_MEMCG_KMEM > /* Index in the kmem_cache->memcg_params.memcg_caches array */ > -- > 1.8.3.1 >
On Sat, Dec 28, 2019 at 7:55 AM Roman Gushchin <guro@fb.com> wrote: > > On Fri, Dec 27, 2019 at 07:43:52AM -0500, Yafang Shao wrote: > > There are some members in struct mem_group can be either 0(false) or > > 1(true), so we can define them using bit field to reduce size. With this > > patch, the size of struct mem_cgroup can be reduced by 64 bytes in theory, > > but as there're some MEMCG_PADDING()s, the real number may be different, > > which is relate with the cacheline size. Anyway, this patch could reduce > > the size of struct mem_cgroup more or less. > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > Cc: Roman Gushchin <guro@fb.com> > > --- > > include/linux/memcontrol.h | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index a7a0a1a5..f68a9ef 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -229,20 +229,26 @@ struct mem_cgroup { > > /* > > * Should the accounting and control be hierarchical, per subtree? > > */ > > - bool use_hierarchy; > > + unsigned int use_hierarchy : 1; > > + > > + /* Legacy tcp memory accounting */ > > + unsigned int tcpmem_active : 1; > > + unsigned int tcpmem_pressure : 1; > > > > /* > > * Should the OOM killer kill all belonging tasks, had it kill one? > > */ > > - bool oom_group; > > + unsigned int oom_group : 1; > > > > /* protected by memcg_oom_lock */ > > - bool oom_lock; > > - int under_oom; > > + unsigned int oom_lock : 1; > > Hm, looking at the original code, it was clear that oom_lock > and under_oom are protected with memcg_oom_lock; but not oom_kill_disable. > > This information seems to be lost. > Should add this comment. Thanks for pointing this out. > Also, I'd look at the actual memory savings. Is it worth it? > Or it's all eaten by the padding. > As explained in the commit log, the real size depends on the cacheline size, and in the future we may introduce other new bool members. I have verified it on my server with 64B-cacheline, and the saveing is 0. Actually there's no strong reason to make this minor optimization. > Thanks! > > > > > - int swappiness; > > /* OOM-Killer disable */ > > - int oom_kill_disable; > > + unsigned int oom_kill_disable : 1; > > + > > + int under_oom; > > + > > + int swappiness; > > > > /* memory.events and memory.events.local */ > > struct cgroup_file events_file; > > @@ -297,9 +303,6 @@ struct mem_cgroup { > > > > unsigned long socket_pressure; > > > > - /* Legacy tcp memory accounting */ > > - bool tcpmem_active; > > - int tcpmem_pressure; > > > > #ifdef CONFIG_MEMCG_KMEM > > /* Index in the kmem_cache->memcg_params.memcg_caches array */ > > -- > > 1.8.3.1 > >
On Fri, 27 Dec 2019 07:43:52 -0500 Yafang Shao <laoar.shao@gmail.com> wrote: > There are some members in struct mem_group can be either 0(false) or > 1(true), so we can define them using bit field to reduce size. With this > patch, the size of struct mem_cgroup can be reduced by 64 bytes in theory, > but as there're some MEMCG_PADDING()s, the real number may be different, > which is relate with the cacheline size. Anyway, this patch could reduce > the size of struct mem_cgroup more or less. > > ... > > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -229,20 +229,26 @@ struct mem_cgroup { > /* > * Should the accounting and control be hierarchical, per subtree? > */ > - bool use_hierarchy; > + unsigned int use_hierarchy : 1; > + > + /* Legacy tcp memory accounting */ > + unsigned int tcpmem_active : 1; > + unsigned int tcpmem_pressure : 1; Kernel coding style for this is normally no-spaces: bool foo:1; More significantly... Now that these fields share the same word of memory, what prevents races when two CPUs do read-modify-write operations on adjacent bitfields? This: struct foo { int a; int b; }; doesn't need locking to prevent modifications of `a' from scribbling on `b'. But with this: struct foo { int a:1; int b:1; } a simple `a = 1' on CPU1 could race with a `b = 1' on CPU2. I think. Maybe the compiler can take care of this in some fashion, but it would require atomic bitops and I doubt if gcc does that for us?
On Wed, Jan 1, 2020 at 6:31 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Fri, 27 Dec 2019 07:43:52 -0500 Yafang Shao <laoar.shao@gmail.com> wrote: > > > There are some members in struct mem_group can be either 0(false) or > > 1(true), so we can define them using bit field to reduce size. With this > > patch, the size of struct mem_cgroup can be reduced by 64 bytes in theory, > > but as there're some MEMCG_PADDING()s, the real number may be different, > > which is relate with the cacheline size. Anyway, this patch could reduce > > the size of struct mem_cgroup more or less. > > > > ... > > > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -229,20 +229,26 @@ struct mem_cgroup { > > /* > > * Should the accounting and control be hierarchical, per subtree? > > */ > > - bool use_hierarchy; > > + unsigned int use_hierarchy : 1; > > + > > + /* Legacy tcp memory accounting */ > > + unsigned int tcpmem_active : 1; > > + unsigned int tcpmem_pressure : 1; > > Kernel coding style for this is normally no-spaces: > > bool foo:1; > I always learn the kernel coding style from the kernel source code. Before I tried to define them using bit field in the kernel, I checked what the kernel did in the past. I found there're some places defined with spaces[1], some places defined without spaces[2]. Finally I selected the one with spaces, unfortunately that's the wrong one . Anyway I know what the right thing is now, thanks. [1]. https://elixir.bootlin.com/linux/v5.5-rc4/source/include/linux/tcp.h#L87 [2]. https://elixir.bootlin.com/linux/v5.5-rc4/source/include/linux/tcp.h#L213 > > > More significantly... Now that these fields share the same word of > memory, what prevents races when two CPUs do read-modify-write > operations on adjacent bitfields? > > This: > > struct foo { > int a; > int b; > }; > > doesn't need locking to prevent modifications of `a' from scribbling on > `b'. But with this: > > struct foo { > int a:1; > int b:1; > } > > a simple `a = 1' on CPU1 could race with a `b = 1' on CPU2. > > I think. Maybe the compiler can take care of this in some fashion, but > it would require atomic bitops and I doubt if gcc does that for us? > > GCC can guarantees that accesses to distinct structure members (which aren't part of a bit-field) are independent, but it can't guarantee that to bitfields. I thought there are some synchronization mechanism to protect memcg against concurrent access. Thanks Yafang
On Fri 27-12-19 07:43:52, Yafang Shao wrote: > There are some members in struct mem_group can be either 0(false) or > 1(true), so we can define them using bit field to reduce size. With this > patch, the size of struct mem_cgroup can be reduced by 64 bytes in theory, > but as there're some MEMCG_PADDING()s, the real number may be different, > which is relate with the cacheline size. Anyway, this patch could reduce > the size of struct mem_cgroup more or less. Pahole says /* size: 2496, cachelines: 39, members: 48 */ /* sum members: 2364, holes: 8, sum holes: 108 */ /* padding: 24 */ /* forced alignments: 3, forced holes: 2, sum forced holes: 88 */ after the change /* size: 2496, cachelines: 39, members: 48 */ /* sum members: 2352, holes: 7, sum holes: 108 */ /* sum bitfield members: 6 bits, bit holes: 1, sum bit holes: 26 bits */ /* padding: 32 */ /* forced alignments: 3, forced holes: 2, sum forced holes: 88 */ so exactly 0 change. I do not think this reshuffling is worth it. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Cc: Roman Gushchin <guro@fb.com> > --- > include/linux/memcontrol.h | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index a7a0a1a5..f68a9ef 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -229,20 +229,26 @@ struct mem_cgroup { > /* > * Should the accounting and control be hierarchical, per subtree? > */ > - bool use_hierarchy; > + unsigned int use_hierarchy : 1; > + > + /* Legacy tcp memory accounting */ > + unsigned int tcpmem_active : 1; > + unsigned int tcpmem_pressure : 1; > > /* > * Should the OOM killer kill all belonging tasks, had it kill one? > */ > - bool oom_group; > + unsigned int oom_group : 1; > > /* protected by memcg_oom_lock */ > - bool oom_lock; > - int under_oom; > + unsigned int oom_lock : 1; > > - int swappiness; > /* OOM-Killer disable */ > - int oom_kill_disable; > + unsigned int oom_kill_disable : 1; > + > + int under_oom; > + > + int swappiness; > > /* memory.events and memory.events.local */ > struct cgroup_file events_file; > @@ -297,9 +303,6 @@ struct mem_cgroup { > > unsigned long socket_pressure; > > - /* Legacy tcp memory accounting */ > - bool tcpmem_active; > - int tcpmem_pressure; > > #ifdef CONFIG_MEMCG_KMEM > /* Index in the kmem_cache->memcg_params.memcg_caches array */ > -- > 1.8.3.1
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index a7a0a1a5..f68a9ef 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -229,20 +229,26 @@ struct mem_cgroup { /* * Should the accounting and control be hierarchical, per subtree? */ - bool use_hierarchy; + unsigned int use_hierarchy : 1; + + /* Legacy tcp memory accounting */ + unsigned int tcpmem_active : 1; + unsigned int tcpmem_pressure : 1; /* * Should the OOM killer kill all belonging tasks, had it kill one? */ - bool oom_group; + unsigned int oom_group : 1; /* protected by memcg_oom_lock */ - bool oom_lock; - int under_oom; + unsigned int oom_lock : 1; - int swappiness; /* OOM-Killer disable */ - int oom_kill_disable; + unsigned int oom_kill_disable : 1; + + int under_oom; + + int swappiness; /* memory.events and memory.events.local */ struct cgroup_file events_file; @@ -297,9 +303,6 @@ struct mem_cgroup { unsigned long socket_pressure; - /* Legacy tcp memory accounting */ - bool tcpmem_active; - int tcpmem_pressure; #ifdef CONFIG_MEMCG_KMEM /* Index in the kmem_cache->memcg_params.memcg_caches array */
There are some members in struct mem_group can be either 0(false) or 1(true), so we can define them using bit field to reduce size. With this patch, the size of struct mem_cgroup can be reduced by 64 bytes in theory, but as there're some MEMCG_PADDING()s, the real number may be different, which is relate with the cacheline size. Anyway, this patch could reduce the size of struct mem_cgroup more or less. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Cc: Roman Gushchin <guro@fb.com> --- include/linux/memcontrol.h | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)