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

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

[Kairui Song]

From: Kairui Song <kasong@tencent.com>

This series removes the global swap cgroup lock. The critical section of
this lock is minimal but it's still a bottle neck for mass parallel
swap workloads.

Improvement of this series is more significant after the si lock rework:
https://lore.kernel.org/linux-mm/20241022192451.38138-1-ryncsn@gmail.com/
But this series works very well on its own.

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 series:
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 series:
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)

Kairui Song (4):
  mm, memcontrol: avoid duplicated memcg enable check
  mm/swap_cgroup: remove swap_cgroup_cmpxchg
  mm/swap_cgroup: simplify swap cgroup definitions
  mm, swap_cgroup: remove global swap cgroup lock

 include/linux/swap_cgroup.h |   2 -
 mm/memcontrol.c             |   2 +-
 mm/swap_cgroup.c            | 110 ++++++++++++++++--------------------
 3 files changed, 51 insertions(+), 63 deletions(-)

-- 
2.47.0

[PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[Kairui Song]

From: Kairui Song <kasong@tencent.com>

mem_cgroup_uncharge_swap() implies a mem_cgroup_disabled() check,
which is already checked by the caller here. Skip it by calling
__mem_cgroup_uncharge_swap() directly.

Signed-off-by: Kairui Song <kasong@tencent.com>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7b3503d12aaf..d3d1eb506eee 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4615,7 +4615,7 @@ void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
 		 * let's not wait for it.  The page already received a
 		 * memory+swap charge, drop the swap entry duplicate.
 		 */
-		mem_cgroup_uncharge_swap(entry, nr_pages);
+		__mem_cgroup_uncharge_swap(entry, nr_pages);
 	}
 }
 
-- 
2.47.0

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[Yosry Ahmed]

On Mon, Dec 2, 2024 at 10:42 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> mem_cgroup_uncharge_swap() implies a mem_cgroup_disabled() check,
> which is already checked by the caller here. Skip it by calling
> __mem_cgroup_uncharge_swap() directly.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  mm/memcontrol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 7b3503d12aaf..d3d1eb506eee 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4615,7 +4615,7 @@ void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
>                  * let's not wait for it.  The page already received a
>                  * memory+swap charge, drop the swap entry duplicate.
>                  */
> -               mem_cgroup_uncharge_swap(entry, nr_pages);
> +               __mem_cgroup_uncharge_swap(entry, nr_pages);

Would it be better to instead remove the mem_cgroup_disabled() check
here and have a single check in this path?

Anyway, FWIW:

Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

>         }
>  }
>
> --
> 2.47.0
>

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[Kairui Song]

On Tue, Dec 3, 2024 at 3:11 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>
> >
> > mem_cgroup_uncharge_swap() implies a mem_cgroup_disabled() check,
> > which is already checked by the caller here. Skip it by calling
> > __mem_cgroup_uncharge_swap() directly.
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > ---
> >  mm/memcontrol.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 7b3503d12aaf..d3d1eb506eee 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -4615,7 +4615,7 @@ void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
> >                  * let's not wait for it.  The page already received a
> >                  * memory+swap charge, drop the swap entry duplicate.
> >                  */
> > -               mem_cgroup_uncharge_swap(entry, nr_pages);
> > +               __mem_cgroup_uncharge_swap(entry, nr_pages);
>
> Would it be better to instead remove the mem_cgroup_disabled() check
> here and have a single check in this path?

Good suggestion, and the kernel test bot just reported
__mem_cgroup_uncharge_swap is undefined with !CONFIG_SWAP, so better
to fix it by removing the check instead.

>
> Anyway, FWIW:
>
> Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
>
> >         }
> >  }
> >
> > --
> > 2.47.0
> >
>

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[Chris Li]

Hi Kairui,

On Tue, Dec 3, 2024 at 12:26 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Tue, Dec 3, 2024 at 3:11 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>
> > >
> > > mem_cgroup_uncharge_swap() implies a mem_cgroup_disabled() check,
> > > which is already checked by the caller here. Skip it by calling
> > > __mem_cgroup_uncharge_swap() directly.
> > >
> > > Signed-off-by: Kairui Song <kasong@tencent.com>
> > > ---
> > >  mm/memcontrol.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 7b3503d12aaf..d3d1eb506eee 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -4615,7 +4615,7 @@ void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
> > >                  * let's not wait for it.  The page already received a
> > >                  * memory+swap charge, drop the swap entry duplicate.
> > >                  */
> > > -               mem_cgroup_uncharge_swap(entry, nr_pages);
> > > +               __mem_cgroup_uncharge_swap(entry, nr_pages);
> >
> > Would it be better to instead remove the mem_cgroup_disabled() check
> > here and have a single check in this path?
>
> Good suggestion, and the kernel test bot just reported
> __mem_cgroup_uncharge_swap is undefined with !CONFIG_SWAP, so better
> to fix it by removing the check instead.

Agree with Yosry on the suggestion of calling
mem_cgroup_uncharge_swap() instead. With that.

Acked-by: Chris Li <chrisl@kernel.org>

Chris

>
> >
> > Anyway, FWIW:
> >
> > Reviewed-by: Yosry Ahmed <yosryahmed@google.com>
> >
> > >         }
> > >  }
> > >
> > > --
> > > 2.47.0
> > >
> >

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[Shakeel Butt]

On Tue, Dec 03, 2024 at 04:25:57PM +0800, Kairui Song wrote:
> On Tue, Dec 3, 2024 at 3:11 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>
> > >
> > > mem_cgroup_uncharge_swap() implies a mem_cgroup_disabled() check,
> > > which is already checked by the caller here. Skip it by calling
> > > __mem_cgroup_uncharge_swap() directly.
> > >
> > > Signed-off-by: Kairui Song <kasong@tencent.com>
> > > ---
> > >  mm/memcontrol.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 7b3503d12aaf..d3d1eb506eee 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -4615,7 +4615,7 @@ void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
> > >                  * let's not wait for it.  The page already received a
> > >                  * memory+swap charge, drop the swap entry duplicate.
> > >                  */
> > > -               mem_cgroup_uncharge_swap(entry, nr_pages);
> > > +               __mem_cgroup_uncharge_swap(entry, nr_pages);
> >
> > Would it be better to instead remove the mem_cgroup_disabled() check
> > here and have a single check in this path?
> 
> Good suggestion, and the kernel test bot just reported
> __mem_cgroup_uncharge_swap is undefined with !CONFIG_SWAP, so better
> to fix it by removing the check instead.
> 

This sounds reasonable.

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[Shakeel Butt]

On Tue, Dec 03, 2024 at 02:41:51AM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> mem_cgroup_uncharge_swap() implies a mem_cgroup_disabled() check,
> which is already checked by the caller here. Skip it by calling
> __mem_cgroup_uncharge_swap() directly.
> 
> Signed-off-by: Kairui Song <kasong@tencent.com>

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[Roman Gushchin]

On Tue, Dec 03, 2024 at 02:41:51AM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> mem_cgroup_uncharge_swap() implies a mem_cgroup_disabled() check,
> which is already checked by the caller here. Skip it by calling
> __mem_cgroup_uncharge_swap() directly.
> 
> Signed-off-by: Kairui Song <kasong@tencent.com>

Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[Barry Song]

On Tue, Dec 3, 2024 at 7:42 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> mem_cgroup_uncharge_swap() implies a mem_cgroup_disabled() check,
> which is already checked by the caller here. Skip it by calling
> __mem_cgroup_uncharge_swap() directly.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>

Reviewed-by: Barry Song <baohua@kernel.org>

> ---
>  mm/memcontrol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 7b3503d12aaf..d3d1eb506eee 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4615,7 +4615,7 @@ void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
>                  * let's not wait for it.  The page already received a
>                  * memory+swap charge, drop the swap entry duplicate.
>                  */
> -               mem_cgroup_uncharge_swap(entry, nr_pages);
> +               __mem_cgroup_uncharge_swap(entry, nr_pages);
>         }
>  }
>
> --
> 2.47.0
>

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[kernel test robot]

Hi Kairui,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Kairui-Song/mm-memcontrol-avoid-duplicated-memcg-enable-check/20241203-024957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20241202184154.19321-2-ryncsn%40gmail.com
patch subject: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check
config: arm-randconfig-002-20241203 (https://download.01.org/0day-ci/archive/20241203/202412030915.jyKBIDck-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241203/202412030915.jyKBIDck-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412030915.jyKBIDck-lkp@intel.com/

All errors (new ones prefixed by >>):

   mm/memcontrol.c: In function 'mem_cgroup_swapin_uncharge_swap':
>> mm/memcontrol.c:4618:17: error: implicit declaration of function '__mem_cgroup_uncharge_swap'; did you mean 'mem_cgroup_uncharge_swap'? [-Wimplicit-function-declaration]
    4618 |                 __mem_cgroup_uncharge_swap(entry, nr_pages);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |                 mem_cgroup_uncharge_swap


vim +4618 mm/memcontrol.c

  4587	
  4588	/*
  4589	 * mem_cgroup_swapin_uncharge_swap - uncharge swap slot
  4590	 * @entry: the first swap entry for which the pages are charged
  4591	 * @nr_pages: number of pages which will be uncharged
  4592	 *
  4593	 * Call this function after successfully adding the charged page to swapcache.
  4594	 *
  4595	 * Note: This function assumes the page for which swap slot is being uncharged
  4596	 * is order 0 page.
  4597	 */
  4598	void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
  4599	{
  4600		/*
  4601		 * Cgroup1's unified memory+swap counter has been charged with the
  4602		 * new swapcache page, finish the transfer by uncharging the swap
  4603		 * slot. The swap slot would also get uncharged when it dies, but
  4604		 * it can stick around indefinitely and we'd count the page twice
  4605		 * the entire time.
  4606		 *
  4607		 * Cgroup2 has separate resource counters for memory and swap,
  4608		 * so this is a non-issue here. Memory and swap charge lifetimes
  4609		 * correspond 1:1 to page and swap slot lifetimes: we charge the
  4610		 * page to memory here, and uncharge swap when the slot is freed.
  4611		 */
  4612		if (!mem_cgroup_disabled() && do_memsw_account()) {
  4613			/*
  4614			 * The swap entry might not get freed for a long time,
  4615			 * let's not wait for it.  The page already received a
  4616			 * memory+swap charge, drop the swap entry duplicate.
  4617			 */
> 4618			__mem_cgroup_uncharge_swap(entry, nr_pages);
  4619		}
  4620	}
  4621	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check

[kernel test robot]

Hi Kairui,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Kairui-Song/mm-memcontrol-avoid-duplicated-memcg-enable-check/20241203-024957
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20241202184154.19321-2-ryncsn%40gmail.com
patch subject: [PATCH 1/4] mm, memcontrol: avoid duplicated memcg enable check
config: i386-buildonly-randconfig-003-20241203 (https://download.01.org/0day-ci/archive/20241203/202412031318.6qeKIH6u-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241203/202412031318.6qeKIH6u-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412031318.6qeKIH6u-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from mm/memcontrol.c:30:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   In file included from mm/memcontrol.c:56:
   include/linux/mm_inline.h:47:41: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
      47 |         __mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
         |                                    ~~~~~~~~~~~ ^ ~~~
   include/linux/mm_inline.h:49:22: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
      49 |                                 NR_ZONE_LRU_BASE + lru, nr_pages);
         |                                 ~~~~~~~~~~~~~~~~ ^ ~~~
>> mm/memcontrol.c:4618:3: error: call to undeclared function '__mem_cgroup_uncharge_swap'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    4618 |                 __mem_cgroup_uncharge_swap(entry, nr_pages);
         |                 ^
   mm/memcontrol.c:4618:3: note: did you mean 'mem_cgroup_uncharge_swap'?
   include/linux/swap.h:687:20: note: 'mem_cgroup_uncharge_swap' declared here
     687 | static inline void mem_cgroup_uncharge_swap(swp_entry_t entry,
         |                    ^
   3 warnings and 1 error generated.


vim +/__mem_cgroup_uncharge_swap +4618 mm/memcontrol.c

  4587	
  4588	/*
  4589	 * mem_cgroup_swapin_uncharge_swap - uncharge swap slot
  4590	 * @entry: the first swap entry for which the pages are charged
  4591	 * @nr_pages: number of pages which will be uncharged
  4592	 *
  4593	 * Call this function after successfully adding the charged page to swapcache.
  4594	 *
  4595	 * Note: This function assumes the page for which swap slot is being uncharged
  4596	 * is order 0 page.
  4597	 */
  4598	void mem_cgroup_swapin_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
  4599	{
  4600		/*
  4601		 * Cgroup1's unified memory+swap counter has been charged with the
  4602		 * new swapcache page, finish the transfer by uncharging the swap
  4603		 * slot. The swap slot would also get uncharged when it dies, but
  4604		 * it can stick around indefinitely and we'd count the page twice
  4605		 * the entire time.
  4606		 *
  4607		 * Cgroup2 has separate resource counters for memory and swap,
  4608		 * so this is a non-issue here. Memory and swap charge lifetimes
  4609		 * correspond 1:1 to page and swap slot lifetimes: we charge the
  4610		 * page to memory here, and uncharge swap when the slot is freed.
  4611		 */
  4612		if (!mem_cgroup_disabled() && do_memsw_account()) {
  4613			/*
  4614			 * The swap entry might not get freed for a long time,
  4615			 * let's not wait for it.  The page already received a
  4616			 * memory+swap charge, drop the swap entry duplicate.
  4617			 */
> 4618			__mem_cgroup_uncharge_swap(entry, nr_pages);
  4619		}
  4620	}
  4621	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/4] mm/swap_cgroup: remove swap_cgroup_cmpxchg

[Kairui Song]

From: Kairui Song <kasong@tencent.com>

This function is never used after commit 6b611388b626
("memcg-v1: remove charge move code").

Signed-off-by: Kairui Song <kasong@tencent.com>
---
 include/linux/swap_cgroup.h |  2 --
 mm/swap_cgroup.c            | 29 -----------------------------
 2 files changed, 31 deletions(-)

diff --git a/include/linux/swap_cgroup.h b/include/linux/swap_cgroup.h
index ae73a87775b3..d521ad1c4164 100644
--- a/include/linux/swap_cgroup.h
+++ b/include/linux/swap_cgroup.h
@@ -6,8 +6,6 @@
 
 #if defined(CONFIG_MEMCG) && defined(CONFIG_SWAP)
 
-extern unsigned short swap_cgroup_cmpxchg(swp_entry_t ent,
-					unsigned short old, unsigned short new);
 extern unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
 					 unsigned int nr_ents);
 extern unsigned short lookup_swap_cgroup_id(swp_entry_t ent);
diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c
index f63d1aa072a1..1770b076f6b7 100644
--- a/mm/swap_cgroup.c
+++ b/mm/swap_cgroup.c
@@ -45,35 +45,6 @@ static struct swap_cgroup *lookup_swap_cgroup(swp_entry_t ent,
 	return &ctrl->map[offset];
 }
 
-/**
- * swap_cgroup_cmpxchg - cmpxchg mem_cgroup's id for this swp_entry.
- * @ent: swap entry to be cmpxchged
- * @old: old id
- * @new: new id
- *
- * Returns old id at success, 0 at failure.
- * (There is no mem_cgroup using 0 as its id)
- */
-unsigned short swap_cgroup_cmpxchg(swp_entry_t ent,
-					unsigned short old, unsigned short new)
-{
-	struct swap_cgroup_ctrl *ctrl;
-	struct swap_cgroup *sc;
-	unsigned long flags;
-	unsigned short retval;
-
-	sc = lookup_swap_cgroup(ent, &ctrl);
-
-	spin_lock_irqsave(&ctrl->lock, flags);
-	retval = sc->id;
-	if (retval == old)
-		sc->id = new;
-	else
-		retval = 0;
-	spin_unlock_irqrestore(&ctrl->lock, flags);
-	return retval;
-}
-
 /**
  * swap_cgroup_record - record mem_cgroup for a set of swap entries
  * @ent: the first swap entry to be recorded into
-- 
2.47.0

Re: [PATCH 2/4] mm/swap_cgroup: remove swap_cgroup_cmpxchg

[Yosry Ahmed]

On Mon, Dec 2, 2024 at 10:42 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> This function is never used after commit 6b611388b626
> ("memcg-v1: remove charge move code").
>
> Signed-off-by: Kairui Song <kasong@tencent.com>

Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

> ---
>  include/linux/swap_cgroup.h |  2 --
>  mm/swap_cgroup.c            | 29 -----------------------------
>  2 files changed, 31 deletions(-)
>
> diff --git a/include/linux/swap_cgroup.h b/include/linux/swap_cgroup.h
> index ae73a87775b3..d521ad1c4164 100644
> --- a/include/linux/swap_cgroup.h
> +++ b/include/linux/swap_cgroup.h
> @@ -6,8 +6,6 @@
>
>  #if defined(CONFIG_MEMCG) && defined(CONFIG_SWAP)
>
> -extern unsigned short swap_cgroup_cmpxchg(swp_entry_t ent,
> -                                       unsigned short old, unsigned short new);
>  extern unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
>                                          unsigned int nr_ents);
>  extern unsigned short lookup_swap_cgroup_id(swp_entry_t ent);
> diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c
> index f63d1aa072a1..1770b076f6b7 100644
> --- a/mm/swap_cgroup.c
> +++ b/mm/swap_cgroup.c
> @@ -45,35 +45,6 @@ static struct swap_cgroup *lookup_swap_cgroup(swp_entry_t ent,
>         return &ctrl->map[offset];
>  }
>
> -/**
> - * swap_cgroup_cmpxchg - cmpxchg mem_cgroup's id for this swp_entry.
> - * @ent: swap entry to be cmpxchged
> - * @old: old id
> - * @new: new id
> - *
> - * Returns old id at success, 0 at failure.
> - * (There is no mem_cgroup using 0 as its id)
> - */
> -unsigned short swap_cgroup_cmpxchg(swp_entry_t ent,
> -                                       unsigned short old, unsigned short new)
> -{
> -       struct swap_cgroup_ctrl *ctrl;
> -       struct swap_cgroup *sc;
> -       unsigned long flags;
> -       unsigned short retval;
> -
> -       sc = lookup_swap_cgroup(ent, &ctrl);
> -
> -       spin_lock_irqsave(&ctrl->lock, flags);
> -       retval = sc->id;
> -       if (retval == old)
> -               sc->id = new;
> -       else
> -               retval = 0;
> -       spin_unlock_irqrestore(&ctrl->lock, flags);
> -       return retval;
> -}
> -
>  /**
>   * swap_cgroup_record - record mem_cgroup for a set of swap entries
>   * @ent: the first swap entry to be recorded into
> --
> 2.47.0
>

Re: [PATCH 2/4] mm/swap_cgroup: remove swap_cgroup_cmpxchg

[Shakeel Butt]

On Tue, Dec 03, 2024 at 02:41:52AM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> This function is never used after commit 6b611388b626
> ("memcg-v1: remove charge move code").
> 
> Signed-off-by: Kairui Song <kasong@tencent.com>

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Re: [PATCH 2/4] mm/swap_cgroup: remove swap_cgroup_cmpxchg

[Roman Gushchin]

On Tue, Dec 03, 2024 at 02:41:52AM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> This function is never used after commit 6b611388b626
> ("memcg-v1: remove charge move code").
> 
> Signed-off-by: Kairui Song <kasong@tencent.com>

Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

Re: [PATCH 2/4] mm/swap_cgroup: remove swap_cgroup_cmpxchg

[Chris Li]

On Mon, Dec 2, 2024 at 10:42 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> From: Kairui Song <kasong@tencent.com>
>
> This function is never used after commit 6b611388b626
> ("memcg-v1: remove charge move code").
>
> Signed-off-by: Kairui Song <kasong@tencent.com>

Acked-by: Chris Li <chrisl@kernel.org>

Chris

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

[Kairui Song]

From: Kairui Song <kasong@tencent.com>

Remove the intermediate struct swap_cgroup, it just a unsigned short
wrapper, simplify the code.

Also zero the map on initialization to prevent unexpected behaviour as
swap cgroup helpers are suppose to return 0 on error.

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);
 	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]);
 }
 
 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;
 	struct swap_cgroup_ctrl *ctrl;
 
 	if (mem_cgroup_disabled())
-- 
2.47.0

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

[Yosry Ahmed]

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.

>
> 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?

>
> 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
>

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

[Chris Li]

On Mon, Dec 2, 2024 at 11: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.

Right, I did not see the struct get removed either.

Anyway I will wait for the V2 to review again.

Chris

>
> >
> > 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?
>
> >
> > 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
> >

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

[Kairui Song]

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
> >
>

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

[Roman Gushchin]

On Tue, Dec 03, 2024 at 02:41:53AM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> Remove the intermediate struct swap_cgroup, it just a unsigned short
> wrapper, simplify the code.
> 
> Also zero the map on initialization to prevent unexpected behaviour as
> swap cgroup helpers are suppose to return 0 on error.
> 
> Signed-off-by: Kairui Song <kasong@tencent.com>

Please, merge this one into the next one, so it's easier to follow
how the end result looks like.

It seems like v2 is coming for the next patch in any case.

Thanks!

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

[Kairui Song]

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];
+	};
+};
+
 static DEFINE_MUTEX(swap_cgroup_mutex);
 
 struct swap_cgroup {
@@ -12,8 +21,10 @@ struct swap_cgroup {
 };
 
 struct swap_cgroup_ctrl {
-	unsigned short	*map;
-	spinlock_t	lock;
+	union {
+		struct swap_cgroup_unit *units;
+		unsigned short *map;
+	};
 };
 
 static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
@@ -31,6 +42,24 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
  *
  * TODO: we can push these buffers out to HIGHMEM.
  */
+static unsigned short __swap_cgroup_xchg(void *map,
+					 pgoff_t offset,
+					 unsigned int new_id)
+{
+	unsigned int old_id;
+	struct swap_cgroup_unit *units = map;
+	struct swap_cgroup_unit *unit = &units[offset / ID_PER_UNIT];
+	struct swap_cgroup_unit new, old = { .raw = atomic_read(&unit->val) };
+
+	do {
+		new.raw = old.raw;
+		old_id = old.__id[offset % ID_PER_UNIT];
+		new.__id[offset % ID_PER_UNIT] = new_id;
+	} while (!atomic_try_cmpxchg(&unit->val, &old.raw, new.raw));
+
+	return old_id;
+}
+
 /**
  * swap_cgroup_record - record mem_cgroup for a set of swap entries
  * @ent: the first swap entry to be recorded into
@@ -44,22 +73,19 @@ unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
 				  unsigned int nr_ents)
 {
 	struct swap_cgroup_ctrl *ctrl;
-	unsigned short *map;
-	unsigned short old;
-	unsigned long flags;
 	pgoff_t offset = swp_offset(ent);
 	pgoff_t end = offset + nr_ents;
+	unsigned short old, iter;
+	unsigned short *map;
 
 	ctrl = &swap_cgroup_ctrl[swp_type(ent)];
 	map = ctrl->map;
 
-	spin_lock_irqsave(&ctrl->lock, flags);
-	old = map[offset];
+	old = READ_ONCE(map[offset]);
 	do {
-		VM_BUG_ON(map[offset] != old);
-		map[offset] = id;
+		iter = __swap_cgroup_xchg(map, offset, id);
+		VM_BUG_ON(iter != old);
 	} while (++offset != end);
-	spin_unlock_irqrestore(&ctrl->lock, flags);
 
 	return old;
 }
@@ -85,20 +111,20 @@ unsigned short lookup_swap_cgroup_id(swp_entry_t ent)
 
 int swap_cgroup_swapon(int type, unsigned long max_pages)
 {
-	void *map;
+	struct swap_cgroup_unit *units;
 	struct swap_cgroup_ctrl *ctrl;
 
 	if (mem_cgroup_disabled())
 		return 0;
 
-	map = vzalloc(max_pages * sizeof(unsigned short));
-	if (!map)
+	units = vzalloc(DIV_ROUND_UP(max_pages, ID_PER_UNIT) *
+			sizeof(struct swap_cgroup_unit));
+	if (!units)
 		goto nomem;
 
 	ctrl = &swap_cgroup_ctrl[type];
 	mutex_lock(&swap_cgroup_mutex);
-	ctrl->map = map;
-	spin_lock_init(&ctrl->lock);
+	ctrl->units = units;
 	mutex_unlock(&swap_cgroup_mutex);
 
 	return 0;
-- 
2.47.0

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

[Yosry Ahmed]

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?

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

> +
>  static DEFINE_MUTEX(swap_cgroup_mutex);
>
>  struct swap_cgroup {
> @@ -12,8 +21,10 @@ struct swap_cgroup {
>  };
>
>  struct swap_cgroup_ctrl {
> -       unsigned short  *map;
> -       spinlock_t      lock;
> +       union {
> +               struct swap_cgroup_unit *units;
> +               unsigned short *map;
> +       };
>  };
>
>  static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
> @@ -31,6 +42,24 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
>   *
>   * TODO: we can push these buffers out to HIGHMEM.
>   */
> +static unsigned short __swap_cgroup_xchg(void *map,
> +                                        pgoff_t offset,
> +                                        unsigned int new_id)
> +{
> +       unsigned int old_id;
> +       struct swap_cgroup_unit *units = map;
> +       struct swap_cgroup_unit *unit = &units[offset / ID_PER_UNIT];
> +       struct swap_cgroup_unit new, old = { .raw = atomic_read(&unit->val) };
> +
> +       do {
> +               new.raw = old.raw;
> +               old_id = old.__id[offset % ID_PER_UNIT];
> +               new.__id[offset % ID_PER_UNIT] = new_id;
> +       } while (!atomic_try_cmpxchg(&unit->val, &old.raw, new.raw));
> +
> +       return old_id;
> +}
> +
>  /**
>   * swap_cgroup_record - record mem_cgroup for a set of swap entries
>   * @ent: the first swap entry to be recorded into
> @@ -44,22 +73,19 @@ unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
>                                   unsigned int nr_ents)
>  {
>         struct swap_cgroup_ctrl *ctrl;
> -       unsigned short *map;
> -       unsigned short old;
> -       unsigned long flags;
>         pgoff_t offset = swp_offset(ent);
>         pgoff_t end = offset + nr_ents;
> +       unsigned short old, iter;
> +       unsigned short *map;
>
>         ctrl = &swap_cgroup_ctrl[swp_type(ent)];
>         map = ctrl->map;
>
> -       spin_lock_irqsave(&ctrl->lock, flags);
> -       old = map[offset];
> +       old = READ_ONCE(map[offset]);
>         do {
> -               VM_BUG_ON(map[offset] != old);
> -               map[offset] = id;
> +               iter = __swap_cgroup_xchg(map, offset, id);
> +               VM_BUG_ON(iter != old);
>         } while (++offset != end);
> -       spin_unlock_irqrestore(&ctrl->lock, flags);
>
>         return old;
>  }
> @@ -85,20 +111,20 @@ unsigned short lookup_swap_cgroup_id(swp_entry_t ent)
>
>  int swap_cgroup_swapon(int type, unsigned long max_pages)
>  {
> -       void *map;
> +       struct swap_cgroup_unit *units;
>         struct swap_cgroup_ctrl *ctrl;
>
>         if (mem_cgroup_disabled())
>                 return 0;
>
> -       map = vzalloc(max_pages * sizeof(unsigned short));
> -       if (!map)
> +       units = vzalloc(DIV_ROUND_UP(max_pages, ID_PER_UNIT) *
> +                       sizeof(struct swap_cgroup_unit));
> +       if (!units)
>                 goto nomem;
>
>         ctrl = &swap_cgroup_ctrl[type];
>         mutex_lock(&swap_cgroup_mutex);
> -       ctrl->map = map;
> -       spin_lock_init(&ctrl->lock);
> +       ctrl->units = units;
>         mutex_unlock(&swap_cgroup_mutex);
>
>         return 0;
> --
> 2.47.0
>

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

[Yosry Ahmed]

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?

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

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

[Kairui Song]

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?

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

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

[Yosry Ahmed]

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.
> >

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

[Kairui Song]

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?

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

[Yosry Ahmed]

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?

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

[Yosry Ahmed]

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];
> +       };
> +};
> +
>  static DEFINE_MUTEX(swap_cgroup_mutex);
>
>  struct swap_cgroup {
> @@ -12,8 +21,10 @@ struct swap_cgroup {
>  };
>
>  struct swap_cgroup_ctrl {
> -       unsigned short  *map;
> -       spinlock_t      lock;
> +       union {
> +               struct swap_cgroup_unit *units;
> +               unsigned short *map;
> +       };
>  };
>
>  static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
> @@ -31,6 +42,24 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
>   *
>   * TODO: we can push these buffers out to HIGHMEM.
>   */

While you're at it, I think the comment above is quite outdated :)

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

[Chris Li]

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))

You might want to have some compile time assert that (sizeof(atomic_t)
% sizeof(unsigned short)) is zero. Could not hurt.

> +struct swap_cgroup_unit {
> +       union {
> +               int raw;
> +               atomic_t val;
> +               unsigned short __id[ID_PER_UNIT];
> +       };
> +};

I suggest just getting rid of this complicated struct/union and using
bit shift and mask to get the u16 out from the atomic_t.

> +
>  static DEFINE_MUTEX(swap_cgroup_mutex);
>
>  struct swap_cgroup {
> @@ -12,8 +21,10 @@ struct swap_cgroup {
>  };
>
>  struct swap_cgroup_ctrl {
> -       unsigned short  *map;
> -       spinlock_t      lock;
> +       union {
> +               struct swap_cgroup_unit *units;
> +               unsigned short *map;

You really shouldn't access the map as an "unsigned short" array,
therefore, I suggest changing the array pointer to "atomic_t".

> +       };
>  };
>
>  static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
> @@ -31,6 +42,24 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
>   *
>   * TODO: we can push these buffers out to HIGHMEM.
>   */
> +static unsigned short __swap_cgroup_xchg(void *map,
> +                                        pgoff_t offset,
> +                                        unsigned int new_id)
> +{
> +       unsigned int old_id;
> +       struct swap_cgroup_unit *units = map;
> +       struct swap_cgroup_unit *unit = &units[offset / ID_PER_UNIT];
> +       struct swap_cgroup_unit new, old = { .raw = atomic_read(&unit->val) };
> +
> +       do {
> +               new.raw = old.raw;
> +               old_id = old.__id[offset % ID_PER_UNIT];
> +               new.__id[offset % ID_PER_UNIT] = new_id;
> +       } while (!atomic_try_cmpxchg(&unit->val, &old.raw, new.raw));

I suggest just calculating the atomic_t offset  (offset /
ID_PER_UNIT) and getting the address of the atomic_t.
Then use the mask and shift to construct the new atomic_t value. It is
likely to generate better code.
You don't want the compiler to generate memory load and store for
constructing the temporary new value.
I haven't checked the machine generated code, I suspect the compiler
is not smart enough to convert those into register shift here. Which
is what you really want.

> +
> +       return old_id;
> +}
> +
>  /**
>   * swap_cgroup_record - record mem_cgroup for a set of swap entries
>   * @ent: the first swap entry to be recorded into
> @@ -44,22 +73,19 @@ unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
>                                   unsigned int nr_ents)
>  {
>         struct swap_cgroup_ctrl *ctrl;
> -       unsigned short *map;
> -       unsigned short old;
> -       unsigned long flags;
>         pgoff_t offset = swp_offset(ent);
>         pgoff_t end = offset + nr_ents;
> +       unsigned short old, iter;
> +       unsigned short *map;

Make it an atomic_t pointer here as well.

>
>         ctrl = &swap_cgroup_ctrl[swp_type(ent)];
>         map = ctrl->map;
>
> -       spin_lock_irqsave(&ctrl->lock, flags);
> -       old = map[offset];
> +       old = READ_ONCE(map[offset]);

Ah, you shouldn't perform u16 reading directly. That will get into the
endian problem of how the u16 is arranged into atomic_t. You should do
atomic reading then shift the bits out so you don't have the endian
problem. It is a bad idea mixing atomic updates and reading the middle
of the atomic address location.

Chris

>         do {
> -               VM_BUG_ON(map[offset] != old);
> -               map[offset] = id;
> +               iter = __swap_cgroup_xchg(map, offset, id);
> +               VM_BUG_ON(iter != old);
>         } while (++offset != end);
> -       spin_unlock_irqrestore(&ctrl->lock, flags);
>
>         return old;
>  }
> @@ -85,20 +111,20 @@ unsigned short lookup_swap_cgroup_id(swp_entry_t ent)
>
>  int swap_cgroup_swapon(int type, unsigned long max_pages)
>  {
> -       void *map;
> +       struct swap_cgroup_unit *units;
>         struct swap_cgroup_ctrl *ctrl;
>
>         if (mem_cgroup_disabled())
>                 return 0;
>
> -       map = vzalloc(max_pages * sizeof(unsigned short));
> -       if (!map)
> +       units = vzalloc(DIV_ROUND_UP(max_pages, ID_PER_UNIT) *
> +                       sizeof(struct swap_cgroup_unit));
> +       if (!units)
>                 goto nomem;
>
>         ctrl = &swap_cgroup_ctrl[type];
>         mutex_lock(&swap_cgroup_mutex);
> -       ctrl->map = map;
> -       spin_lock_init(&ctrl->lock);
> +       ctrl->units = units;
>         mutex_unlock(&swap_cgroup_mutex);
>
>         return 0;
> --
> 2.47.0
>

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

[Kairui Song]

On Thu, Dec 5, 2024 at 3:40 AM Chris Li <chrisl@kernel.org> 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))
>
> You might want to have some compile time assert that (sizeof(atomic_t)
> % sizeof(unsigned short)) is zero. Could not hurt.
>
> > +struct swap_cgroup_unit {
> > +       union {
> > +               int raw;
> > +               atomic_t val;
> > +               unsigned short __id[ID_PER_UNIT];
> > +       };
> > +};
>
> I suggest just getting rid of this complicated struct/union and using
> bit shift and mask to get the u16 out from the atomic_t.

Good suggestion.

>
> > +
> >  static DEFINE_MUTEX(swap_cgroup_mutex);
> >
> >  struct swap_cgroup {
> > @@ -12,8 +21,10 @@ struct swap_cgroup {
> >  };
> >
> >  struct swap_cgroup_ctrl {
> > -       unsigned short  *map;
> > -       spinlock_t      lock;
> > +       union {
> > +               struct swap_cgroup_unit *units;
> > +               unsigned short *map;
>
> You really shouldn't access the map as an "unsigned short" array,
> therefore, I suggest changing the array pointer to "atomic_t".
>
> > +       };
> >  };
> >
> >  static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
> > @@ -31,6 +42,24 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
> >   *
> >   * TODO: we can push these buffers out to HIGHMEM.
> >   */
> > +static unsigned short __swap_cgroup_xchg(void *map,
> > +                                        pgoff_t offset,
> > +                                        unsigned int new_id)
> > +{
> > +       unsigned int old_id;
> > +       struct swap_cgroup_unit *units = map;
> > +       struct swap_cgroup_unit *unit = &units[offset / ID_PER_UNIT];
> > +       struct swap_cgroup_unit new, old = { .raw = atomic_read(&unit->val) };
> > +
> > +       do {
> > +               new.raw = old.raw;
> > +               old_id = old.__id[offset % ID_PER_UNIT];
> > +               new.__id[offset % ID_PER_UNIT] = new_id;
> > +       } while (!atomic_try_cmpxchg(&unit->val, &old.raw, new.raw));
>
> I suggest just calculating the atomic_t offset  (offset /
> ID_PER_UNIT) and getting the address of the atomic_t.
> Then use the mask and shift to construct the new atomic_t value. It is
> likely to generate better code.
> You don't want the compiler to generate memory load and store for
> constructing the temporary new value.
> I haven't checked the machine generated code, I suspect the compiler
> is not smart enough to convert those into register shift here. Which
> is what you really want.
>
> > +
> > +       return old_id;
> > +}
> > +
> >  /**
> >   * swap_cgroup_record - record mem_cgroup for a set of swap entries
> >   * @ent: the first swap entry to be recorded into
> > @@ -44,22 +73,19 @@ unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
> >                                   unsigned int nr_ents)
> >  {
> >         struct swap_cgroup_ctrl *ctrl;
> > -       unsigned short *map;
> > -       unsigned short old;
> > -       unsigned long flags;
> >         pgoff_t offset = swp_offset(ent);
> >         pgoff_t end = offset + nr_ents;
> > +       unsigned short old, iter;
> > +       unsigned short *map;
>
> Make it an atomic_t pointer here as well.
>
> >
> >         ctrl = &swap_cgroup_ctrl[swp_type(ent)];
> >         map = ctrl->map;
> >
> > -       spin_lock_irqsave(&ctrl->lock, flags);
> > -       old = map[offset];
> > +       old = READ_ONCE(map[offset]);
>
> Ah, you shouldn't perform u16 reading directly. That will get into the
> endian problem of how the u16 is arranged into atomic_t. You should do
> atomic reading then shift the bits out so you don't have the endian
> problem. It is a bad idea mixing atomic updates and reading the middle
> of the atomic address location.

Good suggestion, convert the whole map into atomic_t and access / xchg
with bit shifts is also OK, mixing atomic with other types may lead to
misuse indeed.

>
> Chris
>
> >         do {
> > -               VM_BUG_ON(map[offset] != old);
> > -               map[offset] = id;
> > +               iter = __swap_cgroup_xchg(map, offset, id);
> > +               VM_BUG_ON(iter != old);
> >         } while (++offset != end);
> > -       spin_unlock_irqrestore(&ctrl->lock, flags);
> >
> >         return old;
> >  }
> > @@ -85,20 +111,20 @@ unsigned short lookup_swap_cgroup_id(swp_entry_t ent)
> >
> >  int swap_cgroup_swapon(int type, unsigned long max_pages)
> >  {
> > -       void *map;
> > +       struct swap_cgroup_unit *units;
> >         struct swap_cgroup_ctrl *ctrl;
> >
> >         if (mem_cgroup_disabled())
> >                 return 0;
> >
> > -       map = vzalloc(max_pages * sizeof(unsigned short));
> > -       if (!map)
> > +       units = vzalloc(DIV_ROUND_UP(max_pages, ID_PER_UNIT) *
> > +                       sizeof(struct swap_cgroup_unit));
> > +       if (!units)
> >                 goto nomem;
> >
> >         ctrl = &swap_cgroup_ctrl[type];
> >         mutex_lock(&swap_cgroup_mutex);
> > -       ctrl->map = map;
> > -       spin_lock_init(&ctrl->lock);
> > +       ctrl->units = units;
> >         mutex_unlock(&swap_cgroup_mutex);
> >
> >         return 0;
> > --
> > 2.47.0
> >
>