| Message ID | 20241202184154.19321-2-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/swap_cgroup: remove global swap cgroup lock | expand |
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 >
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>
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!
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 >
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
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
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 > > >
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 > > > > >
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.
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); } }