Re: [PATCH v2 2/2] virtio-mem: Correct default THP size for ARM64

[Gavin Shan]

On 12/4/21 5:16 AM, David Hildenbrand wrote:
> On 03.12.21 04:35, Gavin Shan wrote:
> > The default block size is same as to the THP size, which is either
> > retrieved from "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"
> > or hardcoded to 2MB. There are flaws in both mechanisms and this
> > intends to fix them up.
> >
> >    * When "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size" is
> >      used to getting the THP size, 32MB and 512MB are valid values
> >      when we have 16KB and 64KB page size on ARM64.
>
> Ah, right, there is 16KB as well :)

Yep, even though it's rarely used :)

> >    * When the hardcoded THP size is used, 2MB, 32MB and 512MB are
> >      valid values when we have 4KB, 16KB and 64KB page sizes on
> >      ARM64.
> >
> > Co-developed-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   hw/virtio/virtio-mem.c | 32 ++++++++++++++++++++------------
> >   1 file changed, 20 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> > index ac7a40f514..8f3c95300f 100644
> > --- a/hw/virtio/virtio-mem.c
> > +++ b/hw/virtio/virtio-mem.c
> > @@ -38,14 +38,25 @@
> >    */
> >   #define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)(1 * MiB))
> >   
> > -#if defined(__x86_64__) || defined(__arm__) || defined(__aarch64__) || \
> > -    defined(__powerpc64__)
> > -#define VIRTIO_MEM_DEFAULT_THP_SIZE ((uint32_t)(2 * MiB))
> > -#else
> > -        /* fallback to 1 MiB (e.g., the THP size on s390x) */
> > -#define VIRTIO_MEM_DEFAULT_THP_SIZE VIRTIO_MEM_MIN_BLOCK_SIZE
> > +static uint32_t virtio_mem_default_thp_size(void)
> > +{
> > +    uint32_t default_thp_size = VIRTIO_MEM_MIN_BLOCK_SIZE;
> > +
> > +#if defined(__x86_64__) || defined(__arm__) || defined(__powerpc64__)
> > +    default_thp_size = (uint32_t)(2 * MiB);
> > +#elif defined(__aarch64__)
> > +    if (qemu_real_host_page_size == (4 * KiB)) {
>
> you can drop the superfluous (), also in the cases below.

It will be included in v3.

> > +        default_thp_size = (uint32_t)(2 * MiB);
>
> The explicit cast shouldn't be required.

It's not required, but inherited from the definition
of VIRTIO_MEM_MIN_BLOCK_SIZE. However, it's safe to
drop the cast and it will be included in v3.

> > +    } else if (qemu_real_host_page_size == (16 * KiB)) {
> > +        default_thp_size = (uint32_t)(32 * MiB);
> > +    } else if (qemu_real_host_page_size == (64 * KiB)) {
> > +        default_thp_size = (uint32_t)(512 * MiB);
> > +    }
> >   #endif
> >   
> > +    return default_thp_size;
> > +}
> > +
> >   /*
> >    * We want to have a reasonable default block size such that
> >    * 1. We avoid splitting THPs when unplugging memory, which degrades
> > @@ -78,11 +89,8 @@ static uint32_t virtio_mem_thp_size(void)
> >       if (g_file_get_contents(HPAGE_PMD_SIZE_PATH, &content, NULL, NULL) &&
> >           !qemu_strtou64(content, &endptr, 0, &tmp) &&
> >           (!endptr || *endptr == '\n')) {
> > -        /*
> > -         * Sanity-check the value, if it's too big (e.g., aarch64 with 64k base
> > -         * pages) or weird, fallback to something smaller.
> > -         */
> > -        if (!tmp || !is_power_of_2(tmp) || tmp > 16 * MiB) {
> > +        /* Sanity-check the value and fallback to something reasonable. */
> > +        if (!tmp || !is_power_of_2(tmp)) {
> >               warn_report("Read unsupported THP size: %" PRIx64, tmp);
> >           } else {
> >               thp_size = tmp;
> > @@ -90,7 +98,7 @@ static uint32_t virtio_mem_thp_size(void)
> >       }
> >   
> >       if (!thp_size) {
> > -        thp_size = VIRTIO_MEM_DEFAULT_THP_SIZE;
> > +        thp_size = virtio_mem_default_thp_size();
> >           warn_report("Could not detect THP size, falling back to %" PRIx64
> >                       "  MiB.", thp_size / MiB);
> >       }
>
> Apart from that,
>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks for your review, David!

Thanks,
Gavin