Re: [PATCH 4/4] mm, swap_cgroup: remove global swap cgroup lock

[Yosry Ahmed <yosryahmed@google.com>]

On Tue, Dec 3, 2024 at 10:20 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Tue, Dec 3, 2024 at 4:36 AM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Mon, Dec 2, 2024 at 11:28 AM Yosry Ahmed <yosryahmed@google.com> wrote:
> > >
> > > On Mon, Dec 2, 2024 at 10:42 AM Kairui Song <ryncsn@gmail.com> wrote:
> > > >
> > > > From: Kairui Song <kasong@tencent.com>
> > > >
> > > > commit e9e58a4ec3b1 ("memcg: avoid use cmpxchg in swap cgroup maintainance")
> > > > replaced the cmpxchg/xchg with a global irq spinlock because some archs
> > > > doesn't support 2 bytes cmpxchg/xchg. Clearly this won't scale well.
> > > >
> > > > And as commented in swap_cgroup.c, this lock is not needed for map
> > > > synchronization.
> > > >
> > > > Emulation of 2 bytes cmpxchg/xchg with atomic isn't hard, so implement
> > > > it to get rid of this lock.
> > > >
> > > > Testing using 64G brd and build with build kernel with make -j96 in 1.5G
> > > > memory cgroup using 4k folios showed below improvement (10 test run):
> > > >
> > > > Before this series:
> > > > Sys time: 10730.08 (stdev 49.030728)
> > > > Real time: 171.03 (stdev 0.850355)
> > > >
> > > > After this commit:
> > > > Sys time: 9612.24 (stdev 66.310789), -10.42%
> > > > Real time: 159.78 (stdev 0.577193), -6.57%
> > > >
> > > > With 64k folios and 2G memcg:
> > > > Before this series:
> > > > Sys time: 7626.77 (stdev 43.545517)
> > > > Real time: 136.22 (stdev 1.265544)
> > > >
> > > > After this commit:
> > > > Sys time: 6936.03 (stdev 39.996280), -9.06%
> > > > Real time: 129.65 (stdev 0.880039), -4.82%
> > > >
> > > > Sequential swapout of 8G 4k zero folios (24 test run):
> > > > Before this series:
> > > > 5461409.12 us (stdev 183957.827084)
> > > >
> > > > After this commit:
> > > > 5420447.26 us (stdev 196419.240317)
> > > >
> > > > Sequential swapin of 8G 4k zero folios (24 test run):
> > > > Before this series:
> > > > 19736958.916667 us (stdev 189027.246676)
> > > >
> > > > After this commit:
> > > > 19662182.629630 us (stdev 172717.640614)
> > > >
> > > > Performance is better or at least not worse for all tests above.
> > > >
> > > > Signed-off-by: Kairui Song <kasong@tencent.com>
> > > > ---
> > > >  mm/swap_cgroup.c | 56 +++++++++++++++++++++++++++++++++++-------------
> > > >  1 file changed, 41 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c
> > > > index a76afdc3666a..028f5e6be3f0 100644
> > > > --- a/mm/swap_cgroup.c
> > > > +++ b/mm/swap_cgroup.c
> > > > @@ -5,6 +5,15 @@
> > > >
> > > >  #include <linux/swapops.h> /* depends on mm.h include */
> > > >
> > > > +#define ID_PER_UNIT (sizeof(atomic_t) / sizeof(unsigned short))
> > > > +struct swap_cgroup_unit {
> > > > +       union {
> > > > +               int raw;
> > > > +               atomic_t val;
> > > > +               unsigned short __id[ID_PER_UNIT];
> > > > +       };
> > > > +};
> > >
> > > This doubles the size of the per-entry data, right?
> >
> > Oh we don't, we just store 2 ids in an int instead of storing each id
> > individually. But the question below still stands, can't we just use
> > cmpxchg() directly on the id?
>
> Hi Yosry,
>
> Last time I checked the xchg status some archs still don't support
> xchg for 2 bytes, I just found things may have changed slightly but it
> seems at least parisc still doesn't support that. And looking at the
> code some arches still don't support cmpxchg of 2 bytes today (And I
> just dropped cmpxchg helper for swap_cgroup so that should be OK). RCU
> just dropped one-byte cmpxchg emulation 2 months ago in d4e287d7caff
> so that area is changing. Lacking such support is exactly the reason
> why there was a global lock previously, so I think the safe move is
> just to emulate the operation manually for now?

+Paul E. McKenney

If there's already work to support 2-byte cmpxchg() I'd rather wait
for that. Alternatively, if it's not too difficult, we should
generalize this emulation to something like cmpxchg_emu_u8() and add
the missing arch support. It doesn't feel right to have our own custom
2-byte cmpxchg() emulation here.

>
> >
> > >
> > > Why do we need this? I thought cmpxchg() supports multiple sizes and
> > > will already do the emulation for us.
> >