[Qemu-devel] [PATCH RFC 0/5] kvm/vhost: enable durty logging during memory registration

[Qemu-devel] [PATCH RFC 0/5] kvm/vhost: enable durty logging during memory registration

[Michael S. Tsirkin]

Currently, vga cards that allocate vga ram, register it as regular ram,
and then request dirty logging from kvm (which is required for this hack
to function correctly).  Both these operations involve memory slot
update and flush in kvm and in vhost which is a slow operation.

This was observed to slow down windows guests with a huge amount of
memory and cpu and with cirrus vga.

As a solution, this adds an explicit flag that
will enable dirty logging directly when registering
the ram. kvm then needs a single system call
to update tables for vga ram, vhost-net can simply ignore it.

This patchset only updates the cirrus vga lfb vram mapping.
Follow-up patchsets will update all the rest of the cards
and then remove vga_dirty_log_xxx completely.

This replaces the RFC patchset 'vga: flag vga ram for notifiers'.
Warning: compile-tested only.


Michael S. Tsirkin (5):
  cpu: add set_memory flag to request dirty logging
  kvm: halve number of set memory calls for vga
  vhost: skip memory which needs dirty logging
  vhost: optimize out no-change assignment
  cirrus_vga: flag on-device ram for dirty logging

 cpu-common.h    |   22 +++++++++++++++----
 exec.c          |   14 +++++++-----
 hw/cirrus_vga.c |   16 +++++++++----
 hw/vhost.c      |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 kvm-all.c       |   62 +++++++++++++++++++++++++++++++++---------------------
 5 files changed, 134 insertions(+), 41 deletions(-)

-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH RFC 1/5] cpu: add set_memory flag to request dirty logging

[Michael S. Tsirkin]

Pass the flag to all cpu notifiers, doing
nothing at this point. Will be used by
follow-up patches.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 cpu-common.h |   22 +++++++++++++++++-----
 exec.c       |   14 ++++++++------
 hw/vhost.c   |    3 ++-
 kvm-all.c    |    3 ++-
 4 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index ef4e8da..c239cc0 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -34,10 +34,21 @@ typedef unsigned long ram_addr_t;
 typedef void CPUWriteMemoryFunc(void *opaque, target_phys_addr_t addr, uint32_t value);
 typedef uint32_t CPUReadMemoryFunc(void *opaque, target_phys_addr_t addr);
 
-void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,
-                                         ram_addr_t size,
-                                         ram_addr_t phys_offset,
-                                         ram_addr_t region_offset);
+void cpu_register_physical_memory_log(target_phys_addr_t start_addr,
+                                      ram_addr_t size,
+                                      ram_addr_t phys_offset,
+                                      ram_addr_t region_offset,
+                                      bool log_dirty);
+
+static inline void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,
+                                                       ram_addr_t size,
+                                                       ram_addr_t phys_offset,
+                                                       ram_addr_t region_offset)
+{
+    cpu_register_physical_memory_log(start_addr, size, phys_offset,
+                                     region_offset, false);
+}
+
 static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
                                                 ram_addr_t size,
                                                 ram_addr_t phys_offset)
@@ -91,7 +102,8 @@ struct CPUPhysMemoryClient {
     void (*set_memory)(struct CPUPhysMemoryClient *client,
                        target_phys_addr_t start_addr,
                        ram_addr_t size,
-                       ram_addr_t phys_offset);
+                       ram_addr_t phys_offset,
+                       bool log_dirty);
     int (*sync_dirty_bitmap)(struct CPUPhysMemoryClient *client,
                              target_phys_addr_t start_addr,
                              target_phys_addr_t end_addr);
diff --git a/exec.c b/exec.c
index 964ce31..d1a066c 100644
--- a/exec.c
+++ b/exec.c
@@ -1711,11 +1711,12 @@ static QLIST_HEAD(memory_client_list, CPUPhysMemoryClient) memory_client_list
 
 static void cpu_notify_set_memory(target_phys_addr_t start_addr,
                                   ram_addr_t size,
-                                  ram_addr_t phys_offset)
+                                  ram_addr_t phys_offset,
+                                  bool log_dirty)
 {
     CPUPhysMemoryClient *client;
     QLIST_FOREACH(client, &memory_client_list, list) {
-        client->set_memory(client, start_addr, size, phys_offset);
+        client->set_memory(client, start_addr, size, phys_offset, log_dirty);
     }
 }
 
@@ -1755,7 +1756,7 @@ static void phys_page_for_each_1(CPUPhysMemoryClient *client,
         for (i = 0; i < L2_SIZE; ++i) {
             if (pd[i].phys_offset != IO_MEM_UNASSIGNED) {
                 client->set_memory(client, pd[i].region_offset,
-                                   TARGET_PAGE_SIZE, pd[i].phys_offset);
+                                   TARGET_PAGE_SIZE, pd[i].phys_offset, false);
             }
         }
     } else {
@@ -2600,10 +2601,11 @@ static subpage_t *subpage_init (target_phys_addr_t base, ram_addr_t *phys,
    start_addr and region_offset are rounded down to a page boundary
    before calculating this offset.  This should not be a problem unless
    the low bits of start_addr and region_offset differ.  */
-void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,
+void cpu_register_physical_memory_log(target_phys_addr_t start_addr,
                                          ram_addr_t size,
                                          ram_addr_t phys_offset,
-                                         ram_addr_t region_offset)
+                                         ram_addr_t region_offset,
+                                         bool log_dirty)
 {
     target_phys_addr_t addr, end_addr;
     PhysPageDesc *p;
@@ -2611,7 +2613,7 @@ void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,
     ram_addr_t orig_size = size;
     subpage_t *subpage;
 
-    cpu_notify_set_memory(start_addr, size, phys_offset);
+    cpu_notify_set_memory(start_addr, size, phys_offset, log_dirty);
 
     if (phys_offset == IO_MEM_UNASSIGNED) {
         region_offset = start_addr;
diff --git a/hw/vhost.c b/hw/vhost.c
index 14b571d..dc3d0e2 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -300,7 +300,8 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
 static void vhost_client_set_memory(CPUPhysMemoryClient *client,
                                     target_phys_addr_t start_addr,
                                     ram_addr_t size,
-                                    ram_addr_t phys_offset)
+                                    ram_addr_t phys_offset,
+                                    bool log_dirty)
 {
     struct vhost_dev *dev = container_of(client, struct vhost_dev, client);
     ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
diff --git a/kvm-all.c b/kvm-all.c
index 1d7e8ea..1647e1a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -625,7 +625,8 @@ static void kvm_set_phys_mem(target_phys_addr_t start_addr, ram_addr_t size,
 
 static void kvm_client_set_memory(struct CPUPhysMemoryClient *client,
                                   target_phys_addr_t start_addr,
-                                  ram_addr_t size, ram_addr_t phys_offset)
+                                  ram_addr_t size, ram_addr_t phys_offset,
+                                  bool log_dirty)
 {
     kvm_set_phys_mem(start_addr, size, phys_offset);
 }
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH RFC 2/5] kvm: halve number of set memory calls for vga

[Michael S. Tsirkin]

use the new api to reduce the number of these (expensive)
system calls.

Note: using this API, we should be able to
get rid of vga_dirty_log_xxx APIs. Using them doesn't
affect the performance though because we detects
the log_dirty flag set and ignores the call.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 kvm-all.c |   59 ++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 1647e1a..7ace9a2 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -245,48 +245,60 @@ err:
 /*
  * dirty pages logging control
  */
-static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
-                                      ram_addr_t size, int flags, int mask)
+
+static int kvm_mem_flags(KVMState *s, bool log_dirty)
+{
+    return log_dirty ? KVM_MEM_LOG_DIRTY_PAGES : 0;
+}
+
+static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, bool log_dirty)
 {
     KVMState *s = kvm_state;
-    KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
+    int flags, mask = KVM_MEM_LOG_DIRTY_PAGES;
     int old_flags;
 
-    if (mem == NULL)  {
-            fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
-                    TARGET_FMT_plx "\n", __func__, phys_addr,
-                    (target_phys_addr_t)(phys_addr + size - 1));
-            return -EINVAL;
-    }
-
     old_flags = mem->flags;
 
-    flags = (mem->flags & ~mask) | flags;
+    flags = (mem->flags & ~mask) | kvm_mem_flags(s, log_dirty);
     mem->flags = flags;
 
     /* If nothing changed effectively, no need to issue ioctl */
     if (s->migration_log) {
         flags |= KVM_MEM_LOG_DIRTY_PAGES;
     }
+
     if (flags == old_flags) {
-            return 0;
+        return 0;
     }
 
     return kvm_set_user_memory_region(s, mem);
 }
 
+static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
+                                      ram_addr_t size, bool log_dirty)
+{
+    KVMState *s = kvm_state;
+    KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
+
+    if (mem == NULL)  {
+        fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
+                TARGET_FMT_plx "\n", __func__, phys_addr,
+                (target_phys_addr_t)(phys_addr + size - 1));
+        return -EINVAL;
+    }
+    return kvm_slot_dirty_pages_log_change(mem, log_dirty);
+}
+
 static int kvm_log_start(CPUPhysMemoryClient *client,
                          target_phys_addr_t phys_addr, ram_addr_t size)
 {
-    return kvm_dirty_pages_log_change(phys_addr, size, KVM_MEM_LOG_DIRTY_PAGES,
-                                      KVM_MEM_LOG_DIRTY_PAGES);
+    return kvm_dirty_pages_log_change(phys_addr, size, true);
 }
 
 static int kvm_log_stop(CPUPhysMemoryClient *client,
                         target_phys_addr_t phys_addr, ram_addr_t size)
 {
-    return kvm_dirty_pages_log_change(phys_addr, size, 0,
-                                      KVM_MEM_LOG_DIRTY_PAGES);
+    return kvm_dirty_pages_log_change(phys_addr, size, false);
 }
 
 static int kvm_set_migration_log(int enable)
@@ -495,7 +507,7 @@ kvm_check_extension_list(KVMState *s, const KVMCapabilityInfo *list)
 }
 
 static void kvm_set_phys_mem(target_phys_addr_t start_addr, ram_addr_t size,
-                             ram_addr_t phys_offset)
+                             ram_addr_t phys_offset, bool log_dirty)
 {
     KVMState *s = kvm_state;
     ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
@@ -520,7 +532,8 @@ static void kvm_set_phys_mem(target_phys_addr_t start_addr, ram_addr_t size,
             (start_addr + size <= mem->start_addr + mem->memory_size) &&
             (phys_offset - start_addr == mem->phys_offset - mem->start_addr)) {
             /* The new slot fits into the existing one and comes with
-             * identical parameters - nothing to be done. */
+             * identical parameters - update flags and done. */
+            kvm_slot_dirty_pages_log_change(mem, log_dirty);
             return;
         }
 
@@ -550,7 +563,7 @@ static void kvm_set_phys_mem(target_phys_addr_t start_addr, ram_addr_t size,
             mem->memory_size = old.memory_size;
             mem->start_addr = old.start_addr;
             mem->phys_offset = old.phys_offset;
-            mem->flags = 0;
+            mem->flags = kvm_mem_flags(s, log_dirty);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -571,7 +584,7 @@ static void kvm_set_phys_mem(target_phys_addr_t start_addr, ram_addr_t size,
             mem->memory_size = start_addr - old.start_addr;
             mem->start_addr = old.start_addr;
             mem->phys_offset = old.phys_offset;
-            mem->flags = 0;
+            mem->flags =  kvm_mem_flags(s, log_dirty);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -590,7 +603,7 @@ static void kvm_set_phys_mem(target_phys_addr_t start_addr, ram_addr_t size,
             size_delta = mem->start_addr - old.start_addr;
             mem->memory_size = old.memory_size - size_delta;
             mem->phys_offset = old.phys_offset + size_delta;
-            mem->flags = 0;
+            mem->flags = kvm_mem_flags(s, log_dirty);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -613,7 +626,7 @@ static void kvm_set_phys_mem(target_phys_addr_t start_addr, ram_addr_t size,
     mem->memory_size = size;
     mem->start_addr = start_addr;
     mem->phys_offset = phys_offset;
-    mem->flags = 0;
+    mem->flags = kvm_mem_flags(s, log_dirty);
 
     err = kvm_set_user_memory_region(s, mem);
     if (err) {
@@ -628,7 +641,7 @@ static void kvm_client_set_memory(struct CPUPhysMemoryClient *client,
                                   ram_addr_t size, ram_addr_t phys_offset,
                                   bool log_dirty)
 {
-    kvm_set_phys_mem(start_addr, size, phys_offset);
+    kvm_set_phys_mem(start_addr, size, phys_offset, log_dirty);
 }
 
 static int kvm_client_sync_dirty_bitmap(struct CPUPhysMemoryClient *client,
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH RFC 3/5] vhost: skip memory which needs dirty logging

[Michael S. Tsirkin]

vhost doesn't support write logging
(except for migration), anyway.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/vhost.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/vhost.c b/hw/vhost.c
index dc3d0e2..257e3dd 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -311,6 +311,10 @@ static void vhost_client_set_memory(CPUPhysMemoryClient *client,
     int r;
     dev->mem = qemu_realloc(dev->mem, s);
 
+    if (log_dirty) {
+        flags = IO_MEM_UNASSIGNED;
+    }
+
     assert(size);
 
     vhost_dev_unassign_memory(dev, start_addr, size);
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH RFC 4/5] vhost: optimize out no-change assignment

[Michael S. Tsirkin]

Cirrus VGA (at least) calls register memory region
with the same values again and again. The
registration in vhost-net slows this a lot,
optimize by checking that the same data is already registered.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/vhost.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/hw/vhost.c b/hw/vhost.c
index 257e3dd..ff8fc1f 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -297,6 +297,45 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
     return 0;
 }
 
+static struct vhost_memory_region *vhost_dev_find_reg(struct vhost_dev *dev,
+						      uint64_t start_addr,
+						      uint64_t size)
+{
+    int i, n = dev->mem->nregions;
+    for (i = 0; i < n; ++i) {
+        struct vhost_memory_region *reg = dev->mem->regions + i;
+        if (ranges_overlap(reg->guest_phys_addr, reg->memory_size,
+                           start_addr, size)) {
+            return reg;
+        }
+    }
+    return NULL;
+}
+
+static bool vhost_dev_cmp_memory(struct vhost_dev *dev,
+                                 uint64_t start_addr,
+                                 uint64_t size,
+                                 uint64_t uaddr)
+{
+    struct vhost_memory_region *reg = vhost_dev_find_reg(dev, start_addr, size);
+    uint64_t reglast;
+    uint64_t memlast;
+
+    if (!reg) {
+        return true;
+    }
+
+    reglast = range_get_last(reg->guest_phys_addr, reg->memory_size);
+    memlast = range_get_last(start_addr, size);
+
+    /* Need to extend region? */
+    if (start_addr < reg->guest_phys_addr || memlast > reglast) {
+        return true;
+    }
+    /* userspace_addr changed? */
+    return uaddr != reg->userspace_addr + start_addr - reg->guest_phys_addr;
+}
+
 static void vhost_client_set_memory(CPUPhysMemoryClient *client,
                                     target_phys_addr_t start_addr,
                                     ram_addr_t size,
@@ -309,6 +348,21 @@ static void vhost_client_set_memory(CPUPhysMemoryClient *client,
         (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
     uint64_t log_size;
     int r;
+
+    /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */
+    if (flags == IO_MEM_RAM) {
+        if (!vhost_dev_cmp_memory(dev, start_addr, size,
+                                  (uintptr_t)qemu_get_ram_ptr(phys_offset))) {
+            /* Region exists with same address. Nothing to do. */
+            return;
+        }
+    } else {
+        if (!vhost_dev_find_reg(dev, start_addr, size)) {
+            /* Removing region that we don't access. Nothing to do. */
+            return;
+        }
+    }
+
     dev->mem = qemu_realloc(dev->mem, s);
 
     if (log_dirty) {
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCH RFC 5/5] cirrus_vga: flag on-device ram for dirty logging

[Michael S. Tsirkin]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/cirrus_vga.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index bdf4c8b..7212849 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2489,7 +2489,9 @@ static void map_linear_vram(CirrusVGAState *s)
     if (!s->vga.map_addr && s->vga.lfb_addr && s->vga.lfb_end) {
         s->vga.map_addr = s->vga.lfb_addr;
         s->vga.map_end = s->vga.lfb_end;
-        cpu_register_physical_memory(s->vga.map_addr, s->vga.map_end - s->vga.map_addr, s->vga.vram_offset);
+        cpu_register_physical_memory_log(s->vga.map_addr,
+					 s->vga.map_end - s->vga.map_addr,
+					 s->vga.vram_offset, 0, true);
     }
 
     if (!s->vga.map_addr)
@@ -2502,10 +2504,14 @@ static void map_linear_vram(CirrusVGAState *s)
         && !((s->vga.gr[0x0B] & 0x14) == 0x14)
         && !(s->vga.gr[0x0B] & 0x02)) {
 
-        cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
-                                    (s->vga.vram_offset + s->cirrus_bank_base[0]) | IO_MEM_RAM);
-        cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
-                                    (s->vga.vram_offset + s->cirrus_bank_base[1]) | IO_MEM_RAM);
+        cpu_register_physical_memory_log(isa_mem_base + 0xa0000, 0x8000,
+					 (s->vga.vram_offset +
+					  s->cirrus_bank_base[0]) |
+					 IO_MEM_RAM, 0, true);
+        cpu_register_physical_memory_log(isa_mem_base + 0xa8000, 0x8000,
+					 (s->vga.vram_offset +
+					  s->cirrus_bank_base[1]) |
+					 IO_MEM_RAM, 0, true);
 
         s->vga.lfb_vram_mapped = 1;
     }
-- 
1.7.3.2.91.g446ac

Re: [Qemu-devel] [PATCH RFC 0/5] kvm/vhost: enable durty logging during memory registration

[Brad Hards]

On Thu, 7 Apr 2011 06:41:35 am Michael S. Tsirkin wrote:
> As a solution, this adds an explicit flag that
> will enable dirty logging directly when registering
> the ram. kvm then needs a single system call
> to update tables for vga ram, vhost-net can simply ignore it.
I'm not very familiar with QEMU coding style and standards (so this could be 
completely bogus), but I think that the code would be easier if you used a 
integer flag / enum instead of a boolean. That would also give you options for 
additional behaviour changes later too. So instead of passing true/false, pass 
MEM_LOG_DIRTY_PAGES (or whatever you decide to call it).

I can't really comment on the rest of the patch - I'm too clueless. 

> This replaces the RFC patchset 'vga: flag vga ram for notifiers'.
> Warning: compile-tested only.
I compiled and booted my Ubuntu guest (on ubuntu host) which uses Cirrus for 
each step in this patchset. Seems OK to me - I didn't notice appreciable 
speedup, but I wasn't really expecting to.

Brad

Re: [Qemu-devel] [PATCH RFC 0/5] kvm/vhost: enable durty logging during memory registration

[Blue Swirl]

On Thu, Apr 7, 2011 at 8:39 AM, Brad Hards <bradh@frogmouth.net> wrote:
> On Thu, 7 Apr 2011 06:41:35 am Michael S. Tsirkin wrote:
>> As a solution, this adds an explicit flag that
>> will enable dirty logging directly when registering
>> the ram. kvm then needs a single system call
>> to update tables for vga ram, vhost-net can simply ignore it.
> I'm not very familiar with QEMU coding style and standards (so this could be
> completely bogus), but I think that the code would be easier if you used a
> integer flag / enum instead of a boolean. That would also give you options for
> additional behaviour changes later too. So instead of passing true/false, pass
> MEM_LOG_DIRTY_PAGES (or whatever you decide to call it).

Good idea, it's more descriptive.

Maybe also calls to qemu_register_coalesced_mmio() could be avoided
with another flag?