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

[Yosry Ahmed <yosryahmed@google.com>]

On Wed, Dec 4, 2024 at 9:58 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Wed, Dec 4, 2024 at 3:18 AM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > 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.
>
> Actually here we need 2-byte xchg, not cmpxchg. I'm not exactly sure
> if any arch still has anything missing for that support, or is there a
> plan to support it for all archs?

Not sure to be honest.

Taking a step back, with swap_cgroup_cmpxchg() do we still need the
synchronization to begin with? It seems like swap_cgroup_record() is
the only modifier now, could multiple callers be racing for the same
swap slot?