| Message ID | 20211203033522.27580-3-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Support for virtio-mem-pci | expand |
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 :) > > * 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. > + default_thp_size = (uint32_t)(2 * MiB); The explicit cast shouldn't be required. > + } 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!
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
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)) { + default_thp_size = (uint32_t)(2 * MiB); + } 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); }
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. * 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(-)