Re: [PATCH v4] zswap: memcontrol: implement zswap writeback disabling

[Chris Li]

On Fri, Nov 10, 2023 at 4:10 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> > I notice the bool is between two integers.
> > mem_cgroup structure has a few bool sprinkle in different locations.
> > Arrange them together might save a few padding bytes. We can also
> > consider using bit fields.
> > It is a very minor point, the condition also exists before your change.
>
> This sounds like an optimization worthy of its own patch. Two random
> thoughts however:

Sure. I consider this a very minor point as well.

>
> a) Can this be done at the compiler level? I believe you can reduce
> the padding required by sorting the fields of a struct by its size, correct?
> That sounds like a job that a compiler should do for us...

According to the C standard, the struct member should be layered out
in the order it was declared. There are too many codes that assume the
first member has the same address of the struct. Consider we use
struct for DMA descriptor as well, where the memory layout needs to
match the underlying hardware. Re-ordering the members will be really
bad there. There are gcc extensions to do structure member
randomization. But the randomization layout is determined by the
randomization seed. The compiler actually doesn't have the flexibility
to rearrange the member orders to reduce the padding either.

>
> b) Re: the bitfield idea, some of the fields are CONFIG-dependent (well
> like this one). Might be a bit hairier to do it...

You can declare the bit field under preprocessor condition as well,
just like a normal declare. Can you clarify why it is more hairier?
The bitfield does not have a pointer address associated with it, the
compiler can actually move the bit field bits around. You get the
compiler to do it for you in this case.

>
> >
> > >  #endif /* _LINUX_ZSWAP_H */
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index e43b5aba8efc..9cb3ea912cbe 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -5545,6 +5545,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
> > >         WRITE_ONCE(memcg->soft_limit, PAGE_COUNTER_MAX);
> > >  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> > >         memcg->zswap_max = PAGE_COUNTER_MAX;
> > > +
> > > +       if (parent)
> > > +               WRITE_ONCE(memcg->zswap_writeback, READ_ONCE(parent->zswap_writeback));
> > > +       else
> > > +               WRITE_ONCE(memcg->zswap_writeback, true);
> >
> > You can combine this two WRITE_ONCE to one
> >
> > bool writeback = !parent ||   READ_ONCE(parent->zswap_writeback);
> > WRITE_ONCE(memcg->zswap_writeback, writeback);
> >
>
> Yeah I originally did something similar, but then decided to do the if-else
> instead. Honest no strong preference here - just felt that the if-else was
> cleaner at that moment.

One WRITE_ONCE will produce slightly better machine code as less
memory store instructions. Normally the compiler is allowed to do the
common expression elimination to merge the write. However here it has
explicite WRITE_ONCE, so the compiler has to place two memory stores
instructions, because you have two WRITE_ONCE. My suggestion will only
have one memory store instruction. I agree it is micro optimization.

Chris