Re: [PATCH v6 03/15] mm/khugepaged: add struct collapse_control

[Yang Shi <shy828301@gmail.com>]

On Mon, Jun 6, 2022 at 1:20 PM Yang Shi <shy828301@gmail.com> wrote:
>
> On Mon, Jun 6, 2022 at 9:40 AM Zach O'Keefe <zokeefe@google.com> wrote:
> >
> > On Sun, Jun 5, 2022 at 7:42 PM kernel test robot <lkp@intel.com> wrote:
> > >
> > > Hi Zach,
> > >
> > > Thank you for the patch! Perhaps something to improve:
> > >
> > > [auto build test WARNING on akpm-mm/mm-everything]
> > >
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> > > config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220606/202206060911.I8rRqGwC-lkp@intel.com/config)
> > > compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
> > > reproduce (this is a W=1 build):
> > >         # https://github.com/intel-lab-lkp/linux/commit/d87b6065d6050b89930cca0814921aca7c269286
> > >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> > >         git fetch --no-tags linux-review Zach-O-Keefe/mm-userspace-hugepage-collapse/20220606-012953
> > >         git checkout d87b6065d6050b89930cca0814921aca7c269286
> > >         # save the config file
> > >         mkdir build_dir && cp config build_dir/.config
> > >         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>
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > >    mm/khugepaged.c: In function 'khugepaged':
> > > >> mm/khugepaged.c:2284:1: warning: the frame size of 4160 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> > >     2284 | }
> > >          | ^
> >
> > Thanks lkp@intel.com.
> >
> > This is due to config with:
> >
> > CONFIG_FRAME_WARN=2048
> > CONFIG_NODES_SHIFT=10
> >
> > Where struct collapse_control has a member int
> > node_load[MAX_NUMNODES], and we stack allocate one.
> >
> > Is this a configuration that needs to be supported? 1024 nodes seems
> > like a lot and I'm not sure if these configs are randomly generated or
> > are reminiscent of real systems.
>
> I don't have a better idea other than moving it out of the
> collapse_control struct. You may consider changing node_load to two
> dimensions, for example:
>
> node_load[2][MAX_NUMNODES], then define:
> enum {
>     /* khugepaged */
>     COLLAPSE_ASYNC,
>     /* MADV_COLLAPSE */
>     COLLAPSE_SYNC
> }
>
> Then khugepaged and MADV_COLLAPSE get their dedicated node_load respectively.

Sorry, I just realized this won't work for MADV_COLLAPSE since
multiple processes may call it at the same time. We may consider
allocating it dynamically. Have node_load the last element of
collapse_control struct, then do:
for_each_node(node)
    kmalloc(sizeof(int), GFP_KERNEL);

MADV_COLLAPSE or khugepaged could just fail if it fails since THP
allocation is unlikely to succeed in this case. But not sure if it is
worth the complexity rather than just killing it.

>
> The more aggressive approach may be just killing node_load, but I'm
> not sure what impact it may incur.
>
> >
> > Thanks,
> > Zach
> >
> > >
> > > vim +2284 mm/khugepaged.c
> > >
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2261
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2262  static int khugepaged(void *none)
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2263  {
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2264      struct mm_slot *mm_slot;
> > > d87b6065d6050b Zach O'Keefe       2022-06-03  2265      struct collapse_control cc = {
> > > d87b6065d6050b Zach O'Keefe       2022-06-03  2266              .last_target_node = NUMA_NO_NODE,
> > > d87b6065d6050b Zach O'Keefe       2022-06-03  2267      };
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2268
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2269      set_freezable();
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2270      set_user_nice(current, MAX_NICE);
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2271
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2272      while (!kthread_should_stop()) {
> > > d87b6065d6050b Zach O'Keefe       2022-06-03  2273              khugepaged_do_scan(&cc);
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2274              khugepaged_wait_work();
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2275      }
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2276
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2277      spin_lock(&khugepaged_mm_lock);
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2278      mm_slot = khugepaged_scan.mm_slot;
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2279      khugepaged_scan.mm_slot = NULL;
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2280      if (mm_slot)
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2281              collect_mm_slot(mm_slot);
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2282      spin_unlock(&khugepaged_mm_lock);
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2283      return 0;
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26 @2284  }
> > > b46e756f5e4703 Kirill A. Shutemov 2016-07-26  2285
> > >
> > > --
> > > 0-DAY CI Kernel Test Service
> > > https://01.org/lkp
> > >