On Wed, Jan 1, 2020 at 6:31 AM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, 27 Dec 2019 07:43:52 -0500 Yafang Shao <laoar.shao@xxxxxxxxx> 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
