Re: [PATCH] mm, memcg: reduce size of struct mem_cgroup by using bit field

[Yafang Shao <laoar.shao@gmail.com>]

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