Re: [PATCH 3/4] mm/swap_cgroup: simplify swap cgroup definitions

[Kairui Song <ryncsn@gmail.com>]

On Tue, Dec 3, 2024 at 3:26 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>
> >
> > Remove the intermediate struct swap_cgroup, it just a unsigned short
> > wrapper, simplify the code.
>
> Did you actually remove the struct? It doesn't seem like it.

Oops, I forgot to drop these lines of code indeed, I'll send V2
merging this into patch 4/4 so this commit can be ignored for now.

>
> >
> > Also zero the map on initialization to prevent unexpected behaviour as
> > swap cgroup helpers are suppose to return 0 on error.
>
> All the callers lookup the id of an already swapped out page, so it
> should never be uninitialized. Maybe we should WARN if the result of
> the lookup is 0 in this case?

Yes, just a fallback for robustness.

Lookup returning 0 is expected in some cases, eg. Cgroup V1 will call
mem_cgroup_swapin_uncharge_swap early when the folio is added to swap
cache, and erase the record. Then the mem_cgroup_uncharge_swap in
swapfile.c (called when the swap cache is being dropped and entry is
freed) won't double uncharge it. So we can't WARN on that.

>
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > ---
> >  mm/swap_cgroup.c | 45 +++++++++++++++++++--------------------------
> >  1 file changed, 19 insertions(+), 26 deletions(-)
> >
> > diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c
> > index 1770b076f6b7..a76afdc3666a 100644
> > --- a/mm/swap_cgroup.c
> > +++ b/mm/swap_cgroup.c
> > @@ -12,14 +12,12 @@ struct swap_cgroup {
> >  };
> >
> >  struct swap_cgroup_ctrl {
> > -       struct swap_cgroup *map;
> > +       unsigned short  *map;
> >         spinlock_t      lock;
> >  };
> >
> >  static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
> >
> > -#define SC_PER_PAGE    (PAGE_SIZE/sizeof(struct swap_cgroup))
> > -
> >  /*
> >   * SwapCgroup implements "lookup" and "exchange" operations.
> >   * In typical usage, this swap_cgroup is accessed via memcg's charge/uncharge
> > @@ -33,18 +31,6 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
> >   *
> >   * TODO: we can push these buffers out to HIGHMEM.
> >   */
> > -static struct swap_cgroup *lookup_swap_cgroup(swp_entry_t ent,
> > -                                       struct swap_cgroup_ctrl **ctrlp)
> > -{
> > -       pgoff_t offset = swp_offset(ent);
> > -       struct swap_cgroup_ctrl *ctrl;
> > -
> > -       ctrl = &swap_cgroup_ctrl[swp_type(ent)];
> > -       if (ctrlp)
> > -               *ctrlp = ctrl;
> > -       return &ctrl->map[offset];
> > -}
> > -
> >  /**
> >   * swap_cgroup_record - record mem_cgroup for a set of swap entries
> >   * @ent: the first swap entry to be recorded into
> > @@ -58,20 +44,21 @@ unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
> >                                   unsigned int nr_ents)
> >  {
> >         struct swap_cgroup_ctrl *ctrl;
> > -       struct swap_cgroup *sc;
> > +       unsigned short *map;
> >         unsigned short old;
> >         unsigned long flags;
> >         pgoff_t offset = swp_offset(ent);
> >         pgoff_t end = offset + nr_ents;
> >
> > -       sc = lookup_swap_cgroup(ent, &ctrl);
> > +       ctrl = &swap_cgroup_ctrl[swp_type(ent)];
> > +       map = ctrl->map;
> >
> >         spin_lock_irqsave(&ctrl->lock, flags);
> > -       old = sc->id;
> > -       for (; offset < end; offset++, sc++) {
> > -               VM_BUG_ON(sc->id != old);
> > -               sc->id = id;
> > -       }
> > +       old = map[offset];
> > +       do {
> > +               VM_BUG_ON(map[offset] != old);
> > +               map[offset] = id;
> > +       } while (++offset != end);
>
> Why did you change the for loop here?
>
> >         spin_unlock_irqrestore(&ctrl->lock, flags);
> >
> >         return old;
> > @@ -85,20 +72,26 @@ unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
> >   */
> >  unsigned short lookup_swap_cgroup_id(swp_entry_t ent)
> >  {
> > +       struct swap_cgroup_ctrl *ctrl;
> > +
> >         if (mem_cgroup_disabled())
> >                 return 0;
> > -       return lookup_swap_cgroup(ent, NULL)->id;
> > +
> > +       ctrl = &swap_cgroup_ctrl[swp_type(ent)];
> > +       pgoff_t offset = swp_offset(ent);
> > +
> > +       return READ_ONCE(ctrl->map[offset]);
>
> The READ_ONCE() does not exist today in lookup_swap_cgroup(). Why is it needed?
>
> >  }
> >
> >  int swap_cgroup_swapon(int type, unsigned long max_pages)
> >  {
> > -       struct swap_cgroup *map;
> > +       void *map;
> >         struct swap_cgroup_ctrl *ctrl;
> >
> >         if (mem_cgroup_disabled())
> >                 return 0;
> >
> > -       map = vcalloc(max_pages, sizeof(struct swap_cgroup));
> > +       map = vzalloc(max_pages * sizeof(unsigned short));
> >         if (!map)
> >                 goto nomem;
> >
> > @@ -117,7 +110,7 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
> >
> >  void swap_cgroup_swapoff(int type)
> >  {
> > -       struct swap_cgroup *map;
> > +       void *map;
>
> Why void?
>
> >         struct swap_cgroup_ctrl *ctrl;
> >
> >         if (mem_cgroup_disabled())
> > --
> > 2.47.0
> >
>