Re: [PATCH 07/19] Add memtool module with memory allocation stress-test

[Daniel Kiper]

On Tue, Nov 09, 2021 at 01:54:04PM +0100, Daniel Kiper wrote:
> On Tue, Oct 12, 2021 at 06:29:56PM +1100, Daniel Axtens wrote:
> > When working on memory, it's nice to be able to test your work.
> >
> > Add a memtest module. When compiled with --enable-mm-debug, it exposes
> > 3 commands:
> >
> >  * lsmem - print all allocations and free space in all regions
> >  * lsfreemem - print free space in all regions
> >
> >  * stress_big_allocs - stress test large allocations:
> >   - how much memory can we allocate in one chunk?
> >   - how many 1MB chunks can we allocate?
> >   - check that gap-filling works with a 1MB aligned 900kB alloc + a
> >      100kB alloc.
> >
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> >
> > ---
> >
> > I've put this as copyright IBM for now - hopefully we can conclude on
> > whether we're still doing FSF copyright assignments?
>
> It seems to me we should do it. In the worst case, I think, we can put
> IBM and FSF copyright into the file.
>
> > ---
> >  grub-core/Makefile.core.def   |   5 ++
> >  grub-core/commands/memtools.c | 157 ++++++++++++++++++++++++++++++++++
> >  grub-core/kern/mm.c           |   4 +
> >  include/grub/mm.h             |   4 +-
> >  4 files changed, 168 insertions(+), 2 deletions(-)
> >  create mode 100644 grub-core/commands/memtools.c
> >
> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index 8022e1c0a794..0cc3a4a500ec 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -2527,3 +2527,8 @@ module = {
> >    common = commands/i386/wrmsr.c;
> >    enable = x86;
> >  };
> > +
> > +module = {
> > +  name = memtools;
> > +  common = commands/memtools.c;
> > +};
> > diff --git a/grub-core/commands/memtools.c b/grub-core/commands/memtools.c
> > new file mode 100644
> > index 000000000000..6d5778f4a1b0
> > --- /dev/null
> > +++ b/grub-core/commands/memtools.c
> > @@ -0,0 +1,157 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2021  IBM Corporation
> > + *
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <config.h>
> > +#include <grub/dl.h>
> > +#include <grub/misc.h>
> > +#include <grub/command.h>
> > +#include <grub/i18n.h>
> > +#include <grub/memory.h>
> > +#include <grub/mm.h>
> > +
> > +GRUB_MOD_LICENSE ("GPLv3+");
> > +
> > +#ifdef MM_DEBUG
>
> Could we avoid this ifdefery and do everything in Makefile? Then
> non-debug builds will no contain even memtools stub module.
>
> s/memtools/memdebug/?
>
> > +static grub_err_t
> > +grub_cmd_lsmem (grub_command_t cmd __attribute__ ((unused)),
> > +            int argc __attribute__ ((unused)),
> > +            char **args __attribute__ ((unused)))
> > +
> > +{
> > +#ifndef GRUB_MACHINE_EMU
> > +  grub_mm_dump(0);
>
> Missing space between function name and "(". Please fix the same
> mistakes below.
>
> > +#endif
> > +
> > +  return 0;
> > +}
> > +
> > +static grub_err_t
> > +grub_cmd_lsfreemem (grub_command_t cmd __attribute__ ((unused)),
> > +               int argc __attribute__ ((unused)),
> > +               char **args __attribute__ ((unused)))
> > +
> > +{
> > +#ifndef GRUB_MACHINE_EMU
> > +  grub_mm_dump_free();
>
> Ditto...
>
> > +#endif
> > +
> > +  return 0;
>
> return GRUB_ERR_NONE
>
> > +}
> > +
> > +
> > +#define BIG_ALLOC (64 * 1024 * 1024)
>
> s/BIG_ALLOC/STRESS_BIG_ALLOC/?
>
> > +#define SMALL_ALLOC 32
>
> s/SMALL_ALLOC/STRESS_SMALL_ALLOC/?
>
> And could you define these constants immediately behind GRUB_MOD_LICENSE()?
>
> > +static grub_err_t
> > +grub_cmd_stress_big_allocs (grub_command_t cmd __attribute__ ((unused)),
> > +                       int argc __attribute__ ((unused)),
> > +                       char **args __attribute__ ((unused)))
> > +{
> > +  int i, max_mb, blocks_alloced;
> > +  void *mem;
> > +  void **blocklist;
> > +
> > +  grub_printf ("Test 1: increasingly sized allocs to 1GB block\n");
> > +  for (i = 1; i < 1024; i++) {
> > +    grub_printf ("%d MB . ", i);
> > +    mem = grub_malloc (i * 1024 * 1024);
> > +    if (mem == NULL)
> > +      {
> > +   grub_printf ("failed\n");
> > +   break;
> > +      }
> > +    else
> > +      grub_free (mem);
> > +
> > +    if (i % 10 == 0)
> > +      grub_printf ("\n");
> > +  }
> > +
> > +  max_mb = i - 1;
>
> I think i is not incremented when break is executed. So, max_mb value will be 
> wrong.
>
> > +  grub_printf ("Max sized allocation we did was %d MB\n", max_mb);
> > +
> > +  grub_printf ("Test 2: 1MB at a time, max 4GB\n");
> > +  blocklist = grub_calloc (4096, sizeof (void *));
>
> The blocklist can be NULL here.
>
> > +  for (i = 0; i < 4096; i++)
> > +    {
> > +      blocklist[i] = grub_malloc (1024 * 1024);
> > +      if (!blocklist[i])
>
> if (blocklist[i] == NULL)
>
> And please fix similar things below...
>
> > +   {
> > +     grub_printf ("Ran out of memory at iteration %d\n", i);
> > +     break;
>
> Should not we print a dot after every 32 or 64 iterations?
>
> > +   }
> > +    }
> > +  blocks_alloced = i;
>
> Again, i value can be wrong after break.
>
> > +  for (i = 0; i < blocks_alloced; i++)
> > +    grub_free (blocklist[i]);
>
> I think each of these tests should have separate command assigned.

...and then everyone should get some parameters instead of being them
hard coded. This should ease testing with various memory allocation sizes
and number of iterations.

Daniel