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

["Zach O'Keefe" <zokeefe@google.com>]

On Tue, Jun 7, 2022 at 6:00 PM Yang Shi <shy828301@gmail.com> wrote:
>
> On Tue, Jun 7, 2022 at 5:43 PM Zach O'Keefe <zokeefe@google.com> wrote:
> >
> > On Mon, Jun 6, 2022 at 4:54 PM Yang Shi <shy828301@gmail.com> wrote:
> > >
> > > On Mon, Jun 6, 2022 at 3:23 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > >
> > > > On Mon, 6 Jun 2022 09:40:20 -0700 "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.
> > > >
> > > > Adding 4k to the stack isn't a good thing to do.  It's trivial to
> > > > kmalloc the thing, so why not do that?
> > >
> > > Thanks, Andrew. Yeah, I just suggested that too.
> >
> > Thanks Yang / Andrew for taking the time to voice your suggestions.
> >
> > I'll go ahead and just kmalloc() the thing and fail if we can't.
> >
> > Yang, is there a reason to kmalloc() the entire struct
> > collapse_control with trailing flex array vs stack allocating the
> > struct collapse_control + kmalloc()'ing the node_load array?
>
> I don't think those two have too much difference. I don't have a
> strong preference personally. However you could choose:
>
> Define collapse_control as:
> struct collapse_control {
>     xxx;
>     ...
>     int node_load[MAX_NUMANODES];
> }
> Then you could kmalloc the whole struct.
>
> Or it could be defined as:
> struct collapse_control {
>     xxx;
>     ...
>     int *node_load[];
> }
> In this way you could allocate collapse_control on stack or by
> kmalloc, then kmalloc node_load for all possible nodes instead of
> MAX_NUMANODES. This may have a better success rate since you do
> kmalloc much less memory (typically the number of possible nodes is
> much less than MAX_NUMANODES), but it may be not worth it since the
> error handling path is more complicated and it may not make too much
> difference.
>
> The first choice is definitely much simpler, you may want to try that first.

Thanks for the suggestion. First approach also has the benefit of
being able to statically allocate one for khugepaged and simplifies
error paths there. I'll try that.

Again, thanks for taking the time to review and help out / suggest
improvements :)

Best,
Zach

> >
> >
> > > >
> > > > I'll await some reviewer input (hopefully positive ;)) before merging
> > > > this series.