diff mbox series

[v1,2/2] mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim

Message ID 20230228085002.2592473-3-yosryahmed@google.com (mailing list archive)
State Superseded
Headers show
Series Ignore non-LRU-based reclaim in memcg reclaim | expand

Commit Message

Yosry Ahmed Feb. 28, 2023, 8:50 a.m. UTC 
We keep track of different types of reclaimed pages through
reclaim_state->reclaimed, and we add them to the reported number of
reclaimed pages. For non-memcg reclaim, this makes sense. For memcg
reclaim, we have no clue if those pages are charged to the memcg under
reclaim.

Slab pages are shared by different memcgs, so a freed slab page may have
only been partially charged to the memcg under reclaim. The same goes
for clean file pages from pruned inodes (on highmem systems) or xfs
buffer pages, there is no way to link them to the memcg under reclaim.

Stop reporting those freed pages as reclaimed pages during memcg
reclaim. This should make the return value of writing to memory.reclaim,
and may help reduce unnecessary reclaim retries during memcg charging.

Generally, this should make the return value of
try_to_free_mem_cgroup_pages() more accurate. In some limited cases (e.g.
freed a slab page that was mostly charged to the memcg under reclaim),
the return value of try_to_free_mem_cgroup_pages() can be
underestimated, but this should be fine. The freed pages will be
uncharged anyway, and we can charge the memcg the next time around as we
usually do memcg reclaim in a retry loop.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
---
 mm/vmscan.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 11 deletions(-)

Comments

kernel test robot Feb. 28, 2023, 11:45 a.m. UTC | #1 
Hi Yosry,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]
[cannot apply to vbabka-slab/for-next xfs-linux/for-next v6.2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/mm-vmscan-refactor-updating-reclaimed-pages-in-reclaim_state/20230228-165214
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20230228085002.2592473-3-yosryahmed%40google.com
patch subject: [PATCH v1 2/2] mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim
config: x86_64-randconfig-a014-20230227 (https://download.01.org/0day-ci/archive/20230228/202302281959.EmOJaeae-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/f6d2b849f186a927925a29e289d60895048550f5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yosry-Ahmed/mm-vmscan-refactor-updating-reclaimed-pages-in-reclaim_state/20230228-165214
        git checkout f6d2b849f186a927925a29e289d60895048550f5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302281959.EmOJaeae-lkp@intel.com/

All errors (new ones prefixed by >>):

>> mm/vmscan.c:549:13: error: redefinition of 'cgroup_reclaim'
     549 | static bool cgroup_reclaim(struct scan_control *sc)
         |             ^~~~~~~~~~~~~~
   mm/vmscan.c:191:13: note: previous definition of 'cgroup_reclaim' with type 'bool(struct scan_control *)' {aka '_Bool(struct scan_control *)'}
     191 | static bool cgroup_reclaim(struct scan_control *sc)
         |             ^~~~~~~~~~~~~~
>> mm/vmscan.c:554:13: error: redefinition of 'global_reclaim'
     554 | static bool global_reclaim(struct scan_control *sc)
         |             ^~~~~~~~~~~~~~
   mm/vmscan.c:196:13: note: previous definition of 'global_reclaim' with type 'bool(struct scan_control *)' {aka '_Bool(struct scan_control *)'}
     196 | static bool global_reclaim(struct scan_control *sc)
         |             ^~~~~~~~~~~~~~
   mm/vmscan.c:196:13: warning: 'global_reclaim' defined but not used [-Wunused-function]


vim +/cgroup_reclaim +549 mm/vmscan.c

86750830468506 Yang Shi        2021-05-04  548  
b5ead35e7e1d34 Johannes Weiner 2019-11-30 @549  static bool cgroup_reclaim(struct scan_control *sc)
89b5fae5368f6a Johannes Weiner 2012-01-12  550  {
b5ead35e7e1d34 Johannes Weiner 2019-11-30  551  	return false;
89b5fae5368f6a Johannes Weiner 2012-01-12  552  }
97c9341f727105 Tejun Heo       2015-05-22  553  
a579086c99ed70 Yu Zhao         2022-12-21 @554  static bool global_reclaim(struct scan_control *sc)
a579086c99ed70 Yu Zhao         2022-12-21  555  {
a579086c99ed70 Yu Zhao         2022-12-21  556  	return true;
a579086c99ed70 Yu Zhao         2022-12-21  557  }
a579086c99ed70 Yu Zhao         2022-12-21  558
kernel test robot Feb. 28, 2023, 11:55 a.m. UTC | #2 
Hi Yosry,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on linus/master next-20230228]
[cannot apply to vbabka-slab/for-next xfs-linux/for-next v6.2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/mm-vmscan-refactor-updating-reclaimed-pages-in-reclaim_state/20230228-165214
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20230228085002.2592473-3-yosryahmed%40google.com
patch subject: [PATCH v1 2/2] mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim
config: i386-randconfig-a002-20230227 (https://download.01.org/0day-ci/archive/20230228/202302281933.vU1PHuZr-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f6d2b849f186a927925a29e289d60895048550f5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yosry-Ahmed/mm-vmscan-refactor-updating-reclaimed-pages-in-reclaim_state/20230228-165214
        git checkout f6d2b849f186a927925a29e289d60895048550f5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302281933.vU1PHuZr-lkp@intel.com/

All errors (new ones prefixed by >>):

>> mm/vmscan.c:549:13: error: redefinition of 'cgroup_reclaim'
   static bool cgroup_reclaim(struct scan_control *sc)
               ^
   mm/vmscan.c:191:13: note: previous definition is here
   static bool cgroup_reclaim(struct scan_control *sc)
               ^
>> mm/vmscan.c:554:13: error: redefinition of 'global_reclaim'
   static bool global_reclaim(struct scan_control *sc)
               ^
   mm/vmscan.c:196:13: note: previous definition is here
   static bool global_reclaim(struct scan_control *sc)
               ^
   2 errors generated.


vim +/cgroup_reclaim +549 mm/vmscan.c

86750830468506 Yang Shi        2021-05-04  548  
b5ead35e7e1d34 Johannes Weiner 2019-11-30 @549  static bool cgroup_reclaim(struct scan_control *sc)
89b5fae5368f6a Johannes Weiner 2012-01-12  550  {
b5ead35e7e1d34 Johannes Weiner 2019-11-30  551  	return false;
89b5fae5368f6a Johannes Weiner 2012-01-12  552  }
97c9341f727105 Tejun Heo       2015-05-22  553  
a579086c99ed70 Yu Zhao         2022-12-21 @554  static bool global_reclaim(struct scan_control *sc)
a579086c99ed70 Yu Zhao         2022-12-21  555  {
a579086c99ed70 Yu Zhao         2022-12-21  556  	return true;
a579086c99ed70 Yu Zhao         2022-12-21  557  }
a579086c99ed70 Yu Zhao         2022-12-21  558
Yosry Ahmed Feb. 28, 2023, 5:18 p.m. UTC | #3 
On Tue, Feb 28, 2023 at 3:56 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Yosry,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on akpm-mm/mm-everything]
> [also build test ERROR on linus/master next-20230228]
> [cannot apply to vbabka-slab/for-next xfs-linux/for-next v6.2]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/mm-vmscan-refactor-updating-reclaimed-pages-in-reclaim_state/20230228-165214
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link:    https://lore.kernel.org/r/20230228085002.2592473-3-yosryahmed%40google.com
> patch subject: [PATCH v1 2/2] mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim
> config: i386-randconfig-a002-20230227 (https://download.01.org/0day-ci/archive/20230228/202302281933.vU1PHuZr-lkp@intel.com/config)
> compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/intel-lab-lkp/linux/commit/f6d2b849f186a927925a29e289d60895048550f5
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Yosry-Ahmed/mm-vmscan-refactor-updating-reclaimed-pages-in-reclaim_state/20230228-165214
>         git checkout f6d2b849f186a927925a29e289d60895048550f5
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Link: https://lore.kernel.org/oe-kbuild-all/202302281933.vU1PHuZr-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> >> mm/vmscan.c:549:13: error: redefinition of 'cgroup_reclaim'
>    static bool cgroup_reclaim(struct scan_control *sc)
>                ^
>    mm/vmscan.c:191:13: note: previous definition is here
>    static bool cgroup_reclaim(struct scan_control *sc)
>                ^
> >> mm/vmscan.c:554:13: error: redefinition of 'global_reclaim'
>    static bool global_reclaim(struct scan_control *sc)
>                ^
>    mm/vmscan.c:196:13: note: previous definition is here
>    static bool global_reclaim(struct scan_control *sc)
>                ^
>    2 errors generated.

Ugh yeah I didn't realize I am moving the definitions from within an
#ifdef CONFIG_MEMCG. I will just leave the definitions as-is and add a
forward declaration before the definition of
add_non_vmscan_reclaimed(), should also reduce the churn in the diff.
Will wait for a bit before re-spinning to gather some feedback on the
current version first.

>
>
> vim +/cgroup_reclaim +549 mm/vmscan.c
>
> 86750830468506 Yang Shi        2021-05-04  548
> b5ead35e7e1d34 Johannes Weiner 2019-11-30 @549  static bool cgroup_reclaim(struct scan_control *sc)
> 89b5fae5368f6a Johannes Weiner 2012-01-12  550  {
> b5ead35e7e1d34 Johannes Weiner 2019-11-30  551          return false;
> 89b5fae5368f6a Johannes Weiner 2012-01-12  552  }
> 97c9341f727105 Tejun Heo       2015-05-22  553
> a579086c99ed70 Yu Zhao         2022-12-21 @554  static bool global_reclaim(struct scan_control *sc)
> a579086c99ed70 Yu Zhao         2022-12-21  555  {
> a579086c99ed70 Yu Zhao         2022-12-21  556          return true;
> a579086c99ed70 Yu Zhao         2022-12-21  557  }
> a579086c99ed70 Yu Zhao         2022-12-21  558
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
Yosry Ahmed Feb. 28, 2023, 5:24 p.m. UTC | #4 
On Tue, Feb 28, 2023 at 9:18 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Tue, Feb 28, 2023 at 3:56 AM kernel test robot <lkp@intel.com> wrote:
> >
> > Hi Yosry,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on akpm-mm/mm-everything]
> > [also build test ERROR on linus/master next-20230228]
> > [cannot apply to vbabka-slab/for-next xfs-linux/for-next v6.2]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Yosry-Ahmed/mm-vmscan-refactor-updating-reclaimed-pages-in-reclaim_state/20230228-165214
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> > patch link:    https://lore.kernel.org/r/20230228085002.2592473-3-yosryahmed%40google.com
> > patch subject: [PATCH v1 2/2] mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim
> > config: i386-randconfig-a002-20230227 (https://download.01.org/0day-ci/archive/20230228/202302281933.vU1PHuZr-lkp@intel.com/config)
> > compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://github.com/intel-lab-lkp/linux/commit/f6d2b849f186a927925a29e289d60895048550f5
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review Yosry-Ahmed/mm-vmscan-refactor-updating-reclaimed-pages-in-reclaim_state/20230228-165214
> >         git checkout f6d2b849f186a927925a29e289d60895048550f5
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
> >
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Link: https://lore.kernel.org/oe-kbuild-all/202302281933.vU1PHuZr-lkp@intel.com/
> >
> > All errors (new ones prefixed by >>):
> >
> > >> mm/vmscan.c:549:13: error: redefinition of 'cgroup_reclaim'
> >    static bool cgroup_reclaim(struct scan_control *sc)
> >                ^
> >    mm/vmscan.c:191:13: note: previous definition is here
> >    static bool cgroup_reclaim(struct scan_control *sc)
> >                ^
> > >> mm/vmscan.c:554:13: error: redefinition of 'global_reclaim'
> >    static bool global_reclaim(struct scan_control *sc)
> >                ^
> >    mm/vmscan.c:196:13: note: previous definition is here
> >    static bool global_reclaim(struct scan_control *sc)
> >                ^
> >    2 errors generated.
>
> Ugh yeah I didn't realize I am moving the definitions from within an
> #ifdef CONFIG_MEMCG. I will just leave the definitions as-is and add a
> forward declaration before the definition of
> add_non_vmscan_reclaimed(), should also reduce the churn in the diff.
> Will wait for a bit before re-spinning to gather some feedback on the
> current version first.

I can also just move all the reclaim state functions
(set_task_reclaim_state(), report_freed_pages(),
add_non_vmscan_reclaimed()) below that #ifdef CONFIG_MEMCG.
Might also name them more consistently.


>
> >
> >
> > vim +/cgroup_reclaim +549 mm/vmscan.c
> >
> > 86750830468506 Yang Shi        2021-05-04  548
> > b5ead35e7e1d34 Johannes Weiner 2019-11-30 @549  static bool cgroup_reclaim(struct scan_control *sc)
> > 89b5fae5368f6a Johannes Weiner 2012-01-12  550  {
> > b5ead35e7e1d34 Johannes Weiner 2019-11-30  551          return false;
> > 89b5fae5368f6a Johannes Weiner 2012-01-12  552  }
> > 97c9341f727105 Tejun Heo       2015-05-22  553
> > a579086c99ed70 Yu Zhao         2022-12-21 @554  static bool global_reclaim(struct scan_control *sc)
> > a579086c99ed70 Yu Zhao         2022-12-21  555  {
> > a579086c99ed70 Yu Zhao         2022-12-21  556          return true;
> > a579086c99ed70 Yu Zhao         2022-12-21  557  }
> > a579086c99ed70 Yu Zhao         2022-12-21  558
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests
diff mbox series

Patch

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 8846531e85a4..c53659221965 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -188,6 +188,16 @@  struct scan_control {
  */
 int vm_swappiness = 60;
 
+static bool cgroup_reclaim(struct scan_control *sc)
+{
+	return sc->target_mem_cgroup;
+}
+
+static bool global_reclaim(struct scan_control *sc)
+{
+	return !sc->target_mem_cgroup || mem_cgroup_is_root(sc->target_mem_cgroup);
+}
+
 static void set_task_reclaim_state(struct task_struct *task,
 				   struct reclaim_state *rs)
 {
@@ -217,7 +227,35 @@  EXPORT_SYMBOL(report_freed_pages);
 static void add_non_vmscan_reclaimed(struct scan_control *sc,
 				     struct reclaim_state *rs)
 {
-	if (rs) {
+	/*
+	 * Currently, reclaim_state->reclaimed includes three types of pages
+	 * freed outside of vmscan:
+	 * (1) Slab pages.
+	 * (2) Clean file pages from pruned inodes.
+	 * (3) XFS freed buffer pages.
+	 *
+	 * For all of these cases, we have no way of finding out whether these
+	 * pages were related to the memcg under reclaim. For example, a freed
+	 * slab page could have had only a single object charged to the memcg
+	 * under reclaim. Also, populated inodes are not on shrinker LRUs
+	 * anymore except on highmem systems.
+	 *
+	 * Instead of over-reporting the reclaimed pages in a memcg reclaim,
+	 * only count such pages in system-wide reclaim. This prevents
+	 * unnecessary retries during memcg charging and false positive from
+	 * proactive reclaim (memory.reclaim).
+	 *
+	 * For uncommon cases were the freed pages were actually significantly
+	 * charged to the memcg under reclaim, and we end up under-reporting, it
+	 * should be fine. The freed pages will be uncharged anyway, even if
+	 * they are not reported properly, and we will be able to make forward
+	 * progress in charging (which is usually in a retry loop).
+	 *
+	 * We can go one step further, and report the uncharged objcg pages in
+	 * memcg reclaim, to make reporting more accurate and reduce
+	 * under-reporting, but it's probably not worth the complexity for now.
+	 */
+	if (rs && !cgroup_reclaim(sc)) {
 		sc->nr_reclaimed += rs->reclaimed;
 		rs->reclaimed = 0;
 	}
@@ -463,16 +501,6 @@  void reparent_shrinker_deferred(struct mem_cgroup *memcg)
 	up_read(&shrinker_rwsem);
 }
 
-static bool cgroup_reclaim(struct scan_control *sc)
-{
-	return sc->target_mem_cgroup;
-}
-
-static bool global_reclaim(struct scan_control *sc)
-{
-	return !sc->target_mem_cgroup || mem_cgroup_is_root(sc->target_mem_cgroup);
-}
-
 /**
  * writeback_throttling_sane - is the usual dirty throttling mechanism available?
  * @sc: scan_control in question