[PATCH v2 0/5] virtio-mem: block size and address-assignment optimizations

[PATCH v2 0/5] virtio-mem: block size and address-assignment optimizations

[David Hildenbrand]

Let's try to detect the actual THP size and use it as default block size
(unless the page size of the backend indicates that THP don't apply).
Always allow to set a block size of 1 MiB, but warn if the configured block
size is smaller than the default. Handle large block sizes better, avoiding
a virtio-spec violation and optimizing address auto-detection.

For existing setups (x86-64), the default block size won't change (was, and
will be 2 MiB on anonymous memory). For existing x86-64 setups, the address
auto-detection won't change in relevant setups (esp., anonymous memory
and hugetlbfs with 2 MiB pages and no manual configuration of the block
size). I don't see the need for compatibility handling (especially, as
virtio-mem is still not considered production-ready).

Most of this is a preparation for future architectures, using hugetlbfs
to full extend, and using manually configured, larger block sizes
(relevant for vfio in the future).

v1 -> v2:
- Tweak some patch descriptions
- "virtio-mem: Probe THP size to determine default block size"
-- Beautify THP detection a bit.
-- Assume THP might only get used if the memory backend page size corresponds
   to the real hostpage size.
-- Use virtio_mem_default_block_size(RAMBlock *rb) to handle selection
   of the default block size for a RAMBlock.
-- Implement virtio_mem_get_block_size() as preparation for patch #5
- "memory-device: Add get_min_alignment() callback"
-- Simplify documentation.
- "virito-mem: Implement get_min_alignment()"
-- Simplify due to changes in patch #1.

David Hildenbrand (5):
  virtio-mem: Probe THP size to determine default block size
  virtio-mem: Check that "memaddr" is multiples of the block size
  memory-device: Support big alignment requirements
  memory-device: Add get_min_alignment() callback
  virito-mem: Implement get_min_alignment()

 hw/mem/memory-device.c         |  20 ++++--
 hw/virtio/virtio-mem-pci.c     |   7 +++
 hw/virtio/virtio-mem.c         | 110 +++++++++++++++++++++++++++++++--
 include/hw/mem/memory-device.h |  10 +++
 4 files changed, 137 insertions(+), 10 deletions(-)

-- 
2.26.2

[PATCH v2 1/5] virtio-mem: Probe THP size to determine default block size

[David Hildenbrand]

Let's allow a minimum block size of 1 MiB in all configurations. Select
the default block size based on
- The page size of the memory backend.
- The THP size if the memory backend size corresponds to the real hsot
  page size.
- The global minimum of 1 MiB.
and warn if something smaller is configured by the user.

VIRTIO_MEM only supports Linux (depends on LINUX), so we can probe the
THP size unconditionally.

For now we only support virtio-mem on x86-64 - there isn't a user-visiable
change (x86-64 only supports 2 MiB THP on the PMD level) - the default
was, and will be 2 MiB.

If we ever have THP on the PUD level (e.g., 1 GiB THP on x86-64), we
expect to have a trigger to explicitly opt-in for the new THP granularity.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Wei Yang <richardw.yang@linux.intel.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-mem.c | 105 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 8fbec77ccc..9b1461cf9d 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -33,10 +33,83 @@
 #include "trace.h"
 
 /*
- * Use QEMU_VMALLOC_ALIGN, so no THP will have to be split when unplugging
- * memory (e.g., 2MB on x86_64).
+ * Let's not allow blocks smaller than 1 MiB, for example, to keep the tracking
+ * bitmap small.
  */
-#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
+#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
+#endif
+
+/*
+ * We want to have a reasonable default block size such that
+ * 1. We avoid splitting THPs when unplugging memory, which degrades
+ *    performance.
+ * 2. We avoid placing THPs for plugged blocks that also cover unplugged
+ *    blocks.
+ *
+ * The actual THP size might differ between Linux kernels, so we try to probe
+ * it. In the future (if we ever run into issues regarding 2.), we might want
+ * to disable THP in case we fail to properly probe the THP size, or if the
+ * block size is configured smaller than the THP size.
+ */
+static uint32_t thp_size;
+
+#define HPAGE_PMD_SIZE_PATH "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"
+static uint32_t virtio_mem_thp_size(void)
+{
+    gchar *content = NULL;
+    const char *endptr;
+    uint64_t tmp;
+
+    if (thp_size) {
+        return thp_size;
+    }
+
+    /*
+     * Try to probe the actual THP size, fallback to (sane but eventually
+     * incorrect) default sizes.
+     */
+    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) {
+            warn_report("Read unsupported THP size: %" PRIx64, tmp);
+        } else {
+            thp_size = tmp;
+        }
+    }
+
+    if (!thp_size) {
+        thp_size = VIRTIO_MEM_DEFAULT_THP_SIZE;
+        warn_report("Could not detect THP size, falling back to %" PRIx64
+                    "  MiB.", thp_size / MiB);
+    }
+
+    g_free(content);
+    return thp_size;
+}
+
+static uint64_t virtio_mem_default_block_size(RAMBlock *rb)
+{
+    const uint64_t page_size = qemu_ram_pagesize(rb);
+
+    /* We can have hugetlbfs with a page size smaller than the THP size. */
+    if (page_size == qemu_real_host_page_size) {
+        return MAX(page_size, virtio_mem_thp_size());
+    }
+    return MAX(page_size, VIRTIO_MEM_MIN_BLOCK_SIZE);
+}
+
 /*
  * Size the usable region bigger than the requested size if possible. Esp.
  * Linux guests will only add (aligned) memory blocks in case they fully
@@ -437,10 +510,23 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
     rb = vmem->memdev->mr.ram_block;
     page_size = qemu_ram_pagesize(rb);
 
+    /*
+     * If the block size wasn't configured by the user, use a sane default. This
+     * allows using hugetlbfs backends of any page size without manual
+     * intervention.
+     */
+    if (!vmem->block_size) {
+        vmem->block_size = virtio_mem_default_block_size(rb);
+    }
+
     if (vmem->block_size < page_size) {
         error_setg(errp, "'%s' property has to be at least the page size (0x%"
                    PRIx64 ")", VIRTIO_MEM_BLOCK_SIZE_PROP, page_size);
         return;
+    } else if (vmem->block_size < virtio_mem_default_block_size(rb)) {
+        warn_report("'%s' property is smaller than the default block size (%"
+                    PRIx64 " MiB)", VIRTIO_MEM_BLOCK_SIZE_PROP,
+                    virtio_mem_default_block_size(rb) / MiB);
     } else if (!QEMU_IS_ALIGNED(vmem->requested_size, vmem->block_size)) {
         error_setg(errp, "'%s' property has to be multiples of '%s' (0x%" PRIx64
                    ")", VIRTIO_MEM_REQUESTED_SIZE_PROP,
@@ -731,6 +817,18 @@ static void virtio_mem_get_block_size(Object *obj, Visitor *v, const char *name,
     const VirtIOMEM *vmem = VIRTIO_MEM(obj);
     uint64_t value = vmem->block_size;
 
+    /*
+     * If not configured by the user (and we're not realized yet), use the
+     * default block size we would use with the current memory backend.
+     */
+    if (!value) {
+        if (vmem->memdev && memory_region_is_ram(&vmem->memdev->mr)) {
+            value = virtio_mem_default_block_size(vmem->memdev->mr.ram_block);
+        } else {
+            value = virtio_mem_thp_size();
+        }
+    }
+
     visit_type_size(v, name, &value, errp);
 }
 
@@ -810,7 +908,6 @@ static void virtio_mem_instance_init(Object *obj)
 {
     VirtIOMEM *vmem = VIRTIO_MEM(obj);
 
-    vmem->block_size = VIRTIO_MEM_MIN_BLOCK_SIZE;
     notifier_list_init(&vmem->size_change_notifiers);
     vmem->precopy_notifier.notify = virtio_mem_precopy_notify;
 
-- 
2.26.2

[PATCH v2 2/5] virtio-mem: Check that "memaddr" is multiples of the block size

[David Hildenbrand]

The spec requires us to set the "addr" in guest physical address space to
multiples of the block size. In some cases, this is not the case right
now: For example, when starting a VM with 4 GiB boot memory and a
virtio-mem device with a block size of 2 GiB, "memaddr" will be
auto-assigned to 0x140000000 / 5 GiB.

We'll try to improve auto-assignment for memory devices next, to avoid
bailing out in case memory device code selects a bad address.

Note: The Linux driver doesn't support such big block sizes yet.

Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Wei Yang <richardw.yang@linux.intel.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-mem.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 9b1461cf9d..878c0b4f21 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -532,6 +532,11 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
                    ")", VIRTIO_MEM_REQUESTED_SIZE_PROP,
                    VIRTIO_MEM_BLOCK_SIZE_PROP, vmem->block_size);
         return;
+    } else if (!QEMU_IS_ALIGNED(vmem->addr, vmem->block_size)) {
+        error_setg(errp, "'%s' property has to be multiples of '%s' (0x%" PRIx64
+                   ")", VIRTIO_MEM_ADDR_PROP, VIRTIO_MEM_BLOCK_SIZE_PROP,
+                   vmem->block_size);
+        return;
     } else if (!QEMU_IS_ALIGNED(memory_region_size(&vmem->memdev->mr),
                                 vmem->block_size)) {
         error_setg(errp, "'%s' property memdev size has to be multiples of"
-- 
2.26.2

[PATCH v2 3/5] memory-device: Support big alignment requirements

[David Hildenbrand]

Let's warn instead of bailing out - the worst thing that can happen is
that we'll fail hot/coldplug later. The user got warned, and this should
be rare.

This will be necessary for memory devices with rather big (user-defined)
alignment requirements - say a virtio-mem device with a 2G block size -
which will become important, for example, when supporting vfio in the
future.

Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Wei Yang <richardw.yang@linux.intel.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 4bc9cf0917..8a736f1a26 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -119,9 +119,10 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
 
     /* start of address space indicates the maximum alignment we expect */
     if (!QEMU_IS_ALIGNED(range_lob(&as), align)) {
-        error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported",
-                   align);
-        return 0;
+        warn_report("the alignment (0x%" PRIx64 ") exceeds the expected"
+                    " maximum alignment, memory will get fragmented and not"
+                    " all 'maxmem' might be usable for memory devices.",
+                    align);
     }
 
     memory_device_check_addable(ms, size, &err);
@@ -151,7 +152,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
             return 0;
         }
     } else {
-        if (range_init(&new, range_lob(&as), size)) {
+        if (range_init(&new, QEMU_ALIGN_UP(range_lob(&as), align), size)) {
             error_setg(errp, "can't add memory device, device too big");
             return 0;
         }
-- 
2.26.2

[PATCH v2 4/5] memory-device: Add get_min_alignment() callback

[David Hildenbrand]

Add a callback that can be used to express additional alignment
requirements (exceeding the ones from the memory region).

Will be used by virtio-mem to express special alignment requirements due
to manually configured, big block sizes (e.g., 1GB with an ordinary
memory-backend-ram). This avoids failing later when realizing, because
auto-detection wasn't able to assign a properly aligned address.

Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Wei Yang <richardw.yang@linux.intel.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/memory-device.c         | 11 +++++++++--
 include/hw/mem/memory-device.h | 10 ++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 8a736f1a26..cf0627fd01 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -259,7 +259,7 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
 {
     const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
     Error *local_err = NULL;
-    uint64_t addr, align;
+    uint64_t addr, align = 0;
     MemoryRegion *mr;
 
     mr = mdc->get_memory_region(md, &local_err);
@@ -267,7 +267,14 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
         goto out;
     }
 
-    align = legacy_align ? *legacy_align : memory_region_get_alignment(mr);
+    if (legacy_align) {
+        align = *legacy_align;
+    } else {
+        if (mdc->get_min_alignment) {
+            align = mdc->get_min_alignment(md);
+        }
+        align = MAX(align, memory_region_get_alignment(mr));
+    }
     addr = mdc->get_addr(md);
     addr = memory_device_get_free_addr(ms, !addr ? NULL : &addr, align,
                                        memory_region_size(mr), &local_err);
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index cde52e83c9..dcef8d65f7 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -88,6 +88,16 @@ struct MemoryDeviceClass {
      */
     MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
 
+    /*
+     * Optional: Return the desired minimum alignment of the device in guest
+     * physical address space. The final alignment is computed based on this
+     * alignment and the alignment requirements of the memory region.
+     *
+     * Called when plugging the memory device to detect the required alignment
+     * during address assignment.
+     */
+    uint64_t (*get_min_alignment)(const MemoryDeviceState *md);
+
     /*
      * Translate the memory device into #MemoryDeviceInfo.
      */
-- 
2.26.2

[PATCH v2 5/5] virito-mem: Implement get_min_alignment()

[David Hildenbrand]

The block size determines the alignment requirements. Implement
get_min_alignment() of the TYPE_MEMORY_DEVICE interface.

This allows auto-assignment of a properly aligned address in guest
physical address space. For example, when specifying a 2GB block size
for a virtio-mem device with 10GB with a memory setup "-m 4G, 20G",
we'll no longer fail when realizing.

Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Wei Yang <richardw.yang@linux.intel.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-mem-pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/virtio/virtio-mem-pci.c b/hw/virtio/virtio-mem-pci.c
index 590cec041b..aac975a17c 100644
--- a/hw/virtio/virtio-mem-pci.c
+++ b/hw/virtio/virtio-mem-pci.c
@@ -75,6 +75,12 @@ static void virtio_mem_pci_fill_device_info(const MemoryDeviceState *md,
     info->type = MEMORY_DEVICE_INFO_KIND_VIRTIO_MEM;
 }
 
+static uint64_t virtio_mem_pci_get_min_alignment(const MemoryDeviceState *md)
+{
+    return object_property_get_uint(OBJECT(md), VIRTIO_MEM_BLOCK_SIZE_PROP,
+                                    &error_abort);
+}
+
 static void virtio_mem_pci_size_change_notify(Notifier *notifier, void *data)
 {
     VirtIOMEMPCI *pci_mem = container_of(notifier, VirtIOMEMPCI,
@@ -109,6 +115,7 @@ static void virtio_mem_pci_class_init(ObjectClass *klass, void *data)
     mdc->get_plugged_size = virtio_mem_pci_get_plugged_size;
     mdc->get_memory_region = virtio_mem_pci_get_memory_region;
     mdc->fill_device_info = virtio_mem_pci_fill_device_info;
+    mdc->get_min_alignment = virtio_mem_pci_get_min_alignment;
 }
 
 static void virtio_mem_pci_instance_init(Object *obj)
-- 
2.26.2

Re: [PATCH v2 1/5] virtio-mem: Probe THP size to determine default block size

[Pankaj Gupta]

> Let's allow a minimum block size of 1 MiB in all configurations. Select
> the default block size based on
> - The page size of the memory backend.
> - The THP size if the memory backend size corresponds to the real hsot

s/hsot/host
>   page size.
> - The global minimum of 1 MiB.
> and warn if something smaller is configured by the user.
>
> VIRTIO_MEM only supports Linux (depends on LINUX), so we can probe the
> THP size unconditionally.
>
> For now we only support virtio-mem on x86-64 - there isn't a user-visiable

s/visiable/visible
> change (x86-64 only supports 2 MiB THP on the PMD level) - the default
> was, and will be 2 MiB.
>
> If we ever have THP on the PUD level (e.g., 1 GiB THP on x86-64), we
> expect to have a trigger to explicitly opt-in for the new THP granularity.
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Wei Yang <richardw.yang@linux.intel.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/virtio/virtio-mem.c | 105 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 4 deletions(-)
>
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index 8fbec77ccc..9b1461cf9d 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -33,10 +33,83 @@
>  #include "trace.h"
>
>  /*
> - * Use QEMU_VMALLOC_ALIGN, so no THP will have to be split when unplugging
> - * memory (e.g., 2MB on x86_64).
> + * Let's not allow blocks smaller than 1 MiB, for example, to keep the tracking
> + * bitmap small.
>   */
> -#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
> +#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
> +#endif
> +
> +/*
> + * We want to have a reasonable default block size such that
> + * 1. We avoid splitting THPs when unplugging memory, which degrades
> + *    performance.
> + * 2. We avoid placing THPs for plugged blocks that also cover unplugged
> + *    blocks.
> + *
> + * The actual THP size might differ between Linux kernels, so we try to probe
> + * it. In the future (if we ever run into issues regarding 2.), we might want
> + * to disable THP in case we fail to properly probe the THP size, or if the
> + * block size is configured smaller than the THP size.
> + */
> +static uint32_t thp_size;
> +
> +#define HPAGE_PMD_SIZE_PATH "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"
> +static uint32_t virtio_mem_thp_size(void)
> +{
> +    gchar *content = NULL;
> +    const char *endptr;
> +    uint64_t tmp;
> +
> +    if (thp_size) {
> +        return thp_size;
> +    }
> +
> +    /*
> +     * Try to probe the actual THP size, fallback to (sane but eventually
> +     * incorrect) default sizes.
> +     */
> +    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) {
> +            warn_report("Read unsupported THP size: %" PRIx64, tmp);
> +        } else {
> +            thp_size = tmp;
> +        }
> +    }
> +
> +    if (!thp_size) {
> +        thp_size = VIRTIO_MEM_DEFAULT_THP_SIZE;
> +        warn_report("Could not detect THP size, falling back to %" PRIx64
> +                    "  MiB.", thp_size / MiB);
> +    }
> +
> +    g_free(content);
> +    return thp_size;
> +}
> +
> +static uint64_t virtio_mem_default_block_size(RAMBlock *rb)
> +{
> +    const uint64_t page_size = qemu_ram_pagesize(rb);
> +
> +    /* We can have hugetlbfs with a page size smaller than the THP size. */
> +    if (page_size == qemu_real_host_page_size) {
> +        return MAX(page_size, virtio_mem_thp_size());
> +    }

This condition is special, can think of hugetlbfs smaller in size than THP size
configured.
> +    return MAX(page_size, VIRTIO_MEM_MIN_BLOCK_SIZE);

Do we still need this? Or we can have only one return for both the cases?
Probably, I am missing something here.
> +}
> +
>  /*
>   * Size the usable region bigger than the requested size if possible. Esp.
>   * Linux guests will only add (aligned) memory blocks in case they fully
> @@ -437,10 +510,23 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
>      rb = vmem->memdev->mr.ram_block;
>      page_size = qemu_ram_pagesize(rb);
>
> +    /*
> +     * If the block size wasn't configured by the user, use a sane default. This
> +     * allows using hugetlbfs backends of any page size without manual
> +     * intervention.
> +     */
> +    if (!vmem->block_size) {
> +        vmem->block_size = virtio_mem_default_block_size(rb);
> +    }
> +
>      if (vmem->block_size < page_size) {
>          error_setg(errp, "'%s' property has to be at least the page size (0x%"
>                     PRIx64 ")", VIRTIO_MEM_BLOCK_SIZE_PROP, page_size);
>          return;
> +    } else if (vmem->block_size < virtio_mem_default_block_size(rb)) {
> +        warn_report("'%s' property is smaller than the default block size (%"
> +                    PRIx64 " MiB)", VIRTIO_MEM_BLOCK_SIZE_PROP,
> +                    virtio_mem_default_block_size(rb) / MiB);
>      } else if (!QEMU_IS_ALIGNED(vmem->requested_size, vmem->block_size)) {
>          error_setg(errp, "'%s' property has to be multiples of '%s' (0x%" PRIx64
>                     ")", VIRTIO_MEM_REQUESTED_SIZE_PROP,
> @@ -731,6 +817,18 @@ static void virtio_mem_get_block_size(Object *obj, Visitor *v, const char *name,
>      const VirtIOMEM *vmem = VIRTIO_MEM(obj);
>      uint64_t value = vmem->block_size;
>
> +    /*
> +     * If not configured by the user (and we're not realized yet), use the
> +     * default block size we would use with the current memory backend.
> +     */
> +    if (!value) {
> +        if (vmem->memdev && memory_region_is_ram(&vmem->memdev->mr)) {
> +            value = virtio_mem_default_block_size(vmem->memdev->mr.ram_block);
> +        } else {
> +            value = virtio_mem_thp_size();
> +        }
> +    }
> +
>      visit_type_size(v, name, &value, errp);
>  }
>
> @@ -810,7 +908,6 @@ static void virtio_mem_instance_init(Object *obj)
>  {
>      VirtIOMEM *vmem = VIRTIO_MEM(obj);
>
> -    vmem->block_size = VIRTIO_MEM_MIN_BLOCK_SIZE;
>      notifier_list_init(&vmem->size_change_notifiers);
>      vmem->precopy_notifier.notify = virtio_mem_precopy_notify;

Re: [PATCH v2 1/5] virtio-mem: Probe THP size to determine default block size

[David Hildenbrand]

On 29.09.20 15:54, Pankaj Gupta wrote:
>> Let's allow a minimum block size of 1 MiB in all configurations. Select
>> the default block size based on
>> - The page size of the memory backend.
>> - The THP size if the memory backend size corresponds to the real hsot
> 
> s/hsot/host

thanks!

>>   page size.
>> - The global minimum of 1 MiB.
>> and warn if something smaller is configured by the user.
>>
>> VIRTIO_MEM only supports Linux (depends on LINUX), so we can probe the
>> THP size unconditionally.
>>
>> For now we only support virtio-mem on x86-64 - there isn't a user-visiable
> 
> s/visiable/visible

thanks!

>> change (x86-64 only supports 2 MiB THP on the PMD level) - the default
>> was, and will be 2 MiB.
>>
>> If we ever have THP on the PUD level (e.g., 1 GiB THP on x86-64), we
>> expect to have a trigger to explicitly opt-in for the new THP granularity.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Wei Yang <richardw.yang@linux.intel.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/virtio/virtio-mem.c | 105 +++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 101 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
>> index 8fbec77ccc..9b1461cf9d 100644
>> --- a/hw/virtio/virtio-mem.c
>> +++ b/hw/virtio/virtio-mem.c
>> @@ -33,10 +33,83 @@
>>  #include "trace.h"
>>
>>  /*
>> - * Use QEMU_VMALLOC_ALIGN, so no THP will have to be split when unplugging
>> - * memory (e.g., 2MB on x86_64).
>> + * Let's not allow blocks smaller than 1 MiB, for example, to keep the tracking
>> + * bitmap small.
>>   */
>> -#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
>> +#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
>> +#endif
>> +
>> +/*
>> + * We want to have a reasonable default block size such that
>> + * 1. We avoid splitting THPs when unplugging memory, which degrades
>> + *    performance.
>> + * 2. We avoid placing THPs for plugged blocks that also cover unplugged
>> + *    blocks.
>> + *
>> + * The actual THP size might differ between Linux kernels, so we try to probe
>> + * it. In the future (if we ever run into issues regarding 2.), we might want
>> + * to disable THP in case we fail to properly probe the THP size, or if the
>> + * block size is configured smaller than the THP size.
>> + */
>> +static uint32_t thp_size;
>> +
>> +#define HPAGE_PMD_SIZE_PATH "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"
>> +static uint32_t virtio_mem_thp_size(void)
>> +{
>> +    gchar *content = NULL;
>> +    const char *endptr;
>> +    uint64_t tmp;
>> +
>> +    if (thp_size) {
>> +        return thp_size;
>> +    }
>> +
>> +    /*
>> +     * Try to probe the actual THP size, fallback to (sane but eventually
>> +     * incorrect) default sizes.
>> +     */
>> +    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) {
>> +            warn_report("Read unsupported THP size: %" PRIx64, tmp);
>> +        } else {
>> +            thp_size = tmp;
>> +        }
>> +    }
>> +
>> +    if (!thp_size) {
>> +        thp_size = VIRTIO_MEM_DEFAULT_THP_SIZE;
>> +        warn_report("Could not detect THP size, falling back to %" PRIx64
>> +                    "  MiB.", thp_size / MiB);
>> +    }
>> +
>> +    g_free(content);
>> +    return thp_size;
>> +}
>> +
>> +static uint64_t virtio_mem_default_block_size(RAMBlock *rb)
>> +{
>> +    const uint64_t page_size = qemu_ram_pagesize(rb);
>> +
>> +    /* We can have hugetlbfs with a page size smaller than the THP size. */
>> +    if (page_size == qemu_real_host_page_size) {
>> +        return MAX(page_size, virtio_mem_thp_size());
>> +    }
> 
> This condition is special, can think of hugetlbfs smaller in size than THP size
> configured.

Yeah, there are weird architectures, most prominently arm64:

https://www.kernel.org/doc/html/latest/arm64/hugetlbpage.html

Assume you're on 64K base pages with a probed 512MB THP size
(currently). You could have hugetlbfs with 2MB page size via "CONT PTE"
bits.

>> +    return MAX(page_size, VIRTIO_MEM_MIN_BLOCK_SIZE);
> 
> Do we still need this? Or we can have only one return for both the cases?
> Probably, I am missing something here.

We still need it. Assume somebody would have 64K hugetlbfs on arm64
(with 4k base pages), we want to make sure we use at least 1MB blocks.

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/5] virtio-mem: Probe THP size to determine default block size

[Pankaj Gupta]

> >> Let's allow a minimum block size of 1 MiB in all configurations. Select
> >> the default block size based on
> >> - The page size of the memory backend.
> >> - The THP size if the memory backend size corresponds to the real hsot
> >
> > s/hsot/host
>
> thanks!
>
> >>   page size.
> >> - The global minimum of 1 MiB.
> >> and warn if something smaller is configured by the user.
> >>
> >> VIRTIO_MEM only supports Linux (depends on LINUX), so we can probe the
> >> THP size unconditionally.
> >>
> >> For now we only support virtio-mem on x86-64 - there isn't a user-visiable
> >
> > s/visiable/visible
>
> thanks!
>
> >> change (x86-64 only supports 2 MiB THP on the PMD level) - the default
> >> was, and will be 2 MiB.
> >>
> >> If we ever have THP on the PUD level (e.g., 1 GiB THP on x86-64), we
> >> expect to have a trigger to explicitly opt-in for the new THP granularity.
> >>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Wei Yang <richardw.yang@linux.intel.com>
> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/virtio/virtio-mem.c | 105 +++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 101 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> >> index 8fbec77ccc..9b1461cf9d 100644
> >> --- a/hw/virtio/virtio-mem.c
> >> +++ b/hw/virtio/virtio-mem.c
> >> @@ -33,10 +33,83 @@
> >>  #include "trace.h"
> >>
> >>  /*
> >> - * Use QEMU_VMALLOC_ALIGN, so no THP will have to be split when unplugging
> >> - * memory (e.g., 2MB on x86_64).
> >> + * Let's not allow blocks smaller than 1 MiB, for example, to keep the tracking
> >> + * bitmap small.
> >>   */
> >> -#define VIRTIO_MEM_MIN_BLOCK_SIZE ((uint32_t)QEMU_VMALLOC_ALIGN)
> >> +#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
> >> +#endif
> >> +
> >> +/*
> >> + * We want to have a reasonable default block size such that
> >> + * 1. We avoid splitting THPs when unplugging memory, which degrades
> >> + *    performance.
> >> + * 2. We avoid placing THPs for plugged blocks that also cover unplugged
> >> + *    blocks.
> >> + *
> >> + * The actual THP size might differ between Linux kernels, so we try to probe
> >> + * it. In the future (if we ever run into issues regarding 2.), we might want
> >> + * to disable THP in case we fail to properly probe the THP size, or if the
> >> + * block size is configured smaller than the THP size.
> >> + */
> >> +static uint32_t thp_size;
> >> +
> >> +#define HPAGE_PMD_SIZE_PATH "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"
> >> +static uint32_t virtio_mem_thp_size(void)
> >> +{
> >> +    gchar *content = NULL;
> >> +    const char *endptr;
> >> +    uint64_t tmp;
> >> +
> >> +    if (thp_size) {
> >> +        return thp_size;
> >> +    }
> >> +
> >> +    /*
> >> +     * Try to probe the actual THP size, fallback to (sane but eventually
> >> +     * incorrect) default sizes.
> >> +     */
> >> +    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) {
> >> +            warn_report("Read unsupported THP size: %" PRIx64, tmp);
> >> +        } else {
> >> +            thp_size = tmp;
> >> +        }
> >> +    }
> >> +
> >> +    if (!thp_size) {
> >> +        thp_size = VIRTIO_MEM_DEFAULT_THP_SIZE;
> >> +        warn_report("Could not detect THP size, falling back to %" PRIx64
> >> +                    "  MiB.", thp_size / MiB);
> >> +    }
> >> +
> >> +    g_free(content);
> >> +    return thp_size;
> >> +}
> >> +
> >> +static uint64_t virtio_mem_default_block_size(RAMBlock *rb)
> >> +{
> >> +    const uint64_t page_size = qemu_ram_pagesize(rb);
> >> +
> >> +    /* We can have hugetlbfs with a page size smaller than the THP size. */
> >> +    if (page_size == qemu_real_host_page_size) {
> >> +        return MAX(page_size, virtio_mem_thp_size());
> >> +    }
> >
> > This condition is special, can think of hugetlbfs smaller in size than THP size
> > configured.
>
> Yeah, there are weird architectures, most prominently arm64:
>
> https://www.kernel.org/doc/html/latest/arm64/hugetlbpage.html
>
> Assume you're on 64K base pages with a probed 512MB THP size
> (currently). You could have hugetlbfs with 2MB page size via "CONT PTE"
> bits.

Ok. I understand now. Thanks for explaining!
>
> >> +    return MAX(page_size, VIRTIO_MEM_MIN_BLOCK_SIZE);
> >
> > Do we still need this? Or we can have only one return for both the cases?
> > Probably, I am missing something here.
>
> We still need it. Assume somebody would have 64K hugetlbfs on arm64
> (with 4k base pages), we want to make sure we use at least 1MB blocks.
ok. got it.

Looks good to me.
Reviewed-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>