Re: [PATCH v2 1/1] mm: Better handling of adding new regions

[jim945]

06.10.2022 16:07, Daniel Kiper пишет:

> On Sun, Sep 25, 2022 at 03:59:50PM +0200, Patrick Steinhardt wrote:
> > On Tue, Sep 13, 2022 at 01:49:52AM +0800, Zhang Boyang wrote:
> > > The code of dynamically adding new regions has two problems. First, it
> > > always invalidate disk caches, which decreases performance severely.
> > > Second, it request exactly "size" bytes for new region, ignoring region
> > > management overheads.
> > >
> > > This patch makes adding new regions more priority than disk cache
> > > invalidating. This patch also use "size + align + GRUB_MM_HEAP_GROW" as
> > > the size of new region. GRUB_MM_HEAP_GROW is set to 1MiB. This value can
> > > address the region overheads, and it can also improve the performance of
> > > small allocations when default heap is full.
> > >
> > > Fixes: 887f98f0db43 (mm: Allow dynamically requesting additional memory regions)
> > >         
> > It might be sensible to split this up into two patches, one to change
> > when we drop caches and one to round requested sizes more intelligently.
> >       
> Yes, I agree with Patrick.
>
> > > Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com>
> > > ---
> > >  grub-core/kern/mm.c | 27 +++++++++++++++------------
> > >  include/grub/mm.h   |  2 ++
> > >  2 files changed, 17 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> > > index 75f6eacbe..0836b9538 100644
> > > --- a/grub-core/kern/mm.c
> > > +++ b/grub-core/kern/mm.c
> > > @@ -410,18 +410,21 @@ grub_memalign (grub_size_t align, grub_size_t size)
> > >  {
> > >    grub_mm_region_t r;
> > >    grub_size_t n = ((size + GRUB_MM_ALIGN - 1) >> GRUB_MM_ALIGN_LOG2) + 1;
> > > +  grub_size_t grow;
> > >    int count = 0;
> > >
> > >    if (!grub_mm_base)
> > >      goto fail;
> > >
> > > -  if (size > ~(grub_size_t) align)
> > > +  if (size > ~(grub_size_t) align ||
> > > +      (size + align) > ~(grub_size_t) GRUB_MM_HEAP_GROW)
> > >      goto fail;
> > >
> > >    /* We currently assume at least a 32-bit grub_size_t,
> > > -     so limiting allocations to <adress space size> - 1MiB
> > > +     so limiting heap growth to <adress space size> - 1MiB
> > >       in name of sanity is beneficial. */
> > > -  if ((size + align) > ~(grub_size_t) 0x100000)
> > > +  grow = size + align + GRUB_MM_HEAP_GROW;
> > > +  if (grow > ~(grub_size_t) 0x100000)
> > >      goto fail;
> > >         
> > I wonder whether we want to be a bit more intelligent. It feels like the
> > wrong thing to do to always add 1MB to the request regardless of the
> > requested size. It is probably sensible for small requests, but when you
> > request hundreds of megabytes adding a single megabyte feels rather
> > worthless to me.
> >
> > Maybe we could use some kind of buckets instead, e.g.:
> >
> >     - Up to 256kB: allocate 1MB.
> >     - Up to 2048kB: allocate 8MB.
> >     - Up to 16MB: allocate 64MB.
> >
> > I just make up these numbers, but they should help demonstrate what I
> > mean.
> >       
> Adding more than 1 MiB may lead to situation when we are not able to
> boot machine which still has e.g. 64 MiB essentially free but allocated
> for GRUB heap. So, I would stick with 1 MiB even if it is not very
> efficient for larger sizes. Additionally, assuming large memory
> allocations follow large allocations is not always (often?) true.
> Though I would improve algorithm a bit:
>
> grow = ALIGN_UP (size + GRUB_MM_HEAP_GROW, GRUB_MM_HEAP_GROW);
>
> And more importantly this calculations should happen in switch below.
>
> > >    align = (align >> GRUB_MM_ALIGN_LOG2);
> > > @@ -443,22 +446,16 @@ grub_memalign (grub_size_t align, grub_size_t size)
> > >    switch (count)
> > >      {
> > >      case 0:
> > > -      /* Invalidate disk caches.  */
> > > -      grub_disk_cache_invalidate_all ();
> > > -      count++;
> > > -      goto again;
> > > -
> > > -    case 1:
> > >         
> > It feels sensible to reverse the order here so that we end up trying to
> > satisfy allocations by requesting new pages first. So only when we get
> > into the situation where we really cannot satisfy the request we try to
> > reclaim memory as a last-effort strategy.
> >       
> Yes, I agree.
>
> Daniel
>

/* Invalidate disk caches. */ in case 0 causes a long loading time.

 Script to assemble grab.efi and prefix in SFS. For testing this.

Does not appear in qemu.

The TEST_GRUB directory will contain files that need to be copied to the flash drive.

pwd="$PWD"
workdir=`dirname "$(readlink -e "$0")"`
tmp=$(mktemp -d /tmp/grub-memdisk-XXXXXXXXX)

modules="/lib/grub/x86_64-efi"

if [ -d "${tmp}" ] ; then rm -rf "${tmp}" ; fi
mkdir -p "${tmp}"/settings
cat << EOF >> "${tmp}"/settings/config.cfg
set root=memdisk
normal /boot/grub/memdisk.cfg
EOF

cat << EOF >> "${tmp}"/settings/memdisk.cfg
search -s -f /gp.loop
loopback memdiskgp /gp.loop
set prefix=(memdiskgp)/boot/grub
set root=memdiskgp
configfile /boot/grub/grub.cfg
EOF

cat << EOF >> "${tmp}"/settings/grub.cfg
menuentry "===TEST===" {
echo "===TEST==="
sleep 1
}
EOF

# Make memdisk 1
mkdir -p "${tmp}"/memdisk
mkdir -p "${tmp}"/memdisk/boot/grub
cp "${tmp}"/settings/memdisk.cfg "${tmp}"/memdisk/boot/grub/memdisk.cfg

cd "$tmp"/memdisk
find ./boot | cpio -o -H newc > "${tmp}"/memdisk.loop #2> /dev/null
cd "$pwd"
rm -r "${tmp}"/memdisk
# Make memdisk 1

# Make memdisk 2
mkdir "${tmp}"/prefix_img

mkdir -p "${tmp}"/prefix_img/boot/grub/x86_64-efi
echo "Copying x86_64-efi modules"
cp "${modules}"/*.mod "${tmp}"/prefix_img/boot/grub/x86_64-efi/
cp "${modules}"/*.lst "${tmp}"/prefix_img/boot/grub/x86_64-efi/
cp "${tmp}"/settings/grub.cfg "${tmp}"/prefix_img/boot/grub/grub.cfg
echo "Generate gp.loop"
mksquashfs "$tmp"/prefix_img/ "${tmp}"/gp.loop -all-root &> /dev/null
# Make memdisk 2

echo "Generate grubx64.efi"
builtin_pl="cpio exfat ext2 fat gzio iso9660 loopback lzopio newc normal ntfs part_gpt part_msdos probe read search tar test configfile echo xzio squash4 sfs memdisk"

grub-mkimage -d "${modules}" -m "${tmp}"/memdisk.loop -p "/boot/grub" -c "${tmp}"/settings/config.cfg -o "${tmp}"/grubx64.efi -O x86_64-efi $builtin_pl

echo "Copying files in ${workdir}/TEST_GRUB"
mkdir -p "${workdir}"/TEST_GRUB/EFI/BOOT
cp "${tmp}"/grubx64.efi "${workdir}"/TEST_GRUB/EFI/BOOT/BOOTX64.EFI
cp "${tmp}"/gp.loop "${workdir}"/TEST_GRUB/gp.loop

rm -rf "${tmp}"