[Qemu-devel] [PATCH] RFC: use logging count for individual regions

[Qemu-devel] [PATCH] RFC: use logging count for individual regions

[Glauber Costa]

qemu-kvm use this scheme of logging count of individual regions,
which is, IMHO, more flexible which the one we have right now.
I'm proposing we use it.

Anthony, please don't apply this patch yet, as I would want it
to receive proper testing, and FYI, current migration broken ;(
- and I don't really have time to go debug it now.

Jan: Please let me know what you think of it.

Thanks!

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 8567ac9..7d942c8 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -46,6 +46,7 @@ typedef struct KVMSlot
     ram_addr_t phys_offset;
     int slot;
     int flags;
+    uint32_t logging_count;
 } KVMSlot;
 
 typedef struct kvm_dirty_log KVMDirtyLog;
@@ -59,7 +60,6 @@ struct KVMState
     int vmfd;
     int coalesced_mmio;
     int broken_set_mem_region;
-    int migration_log;
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
 #endif
@@ -137,9 +137,7 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     mem.memory_size = slot->memory_size;
     mem.userspace_addr = (unsigned long)qemu_get_ram_ptr(slot->phys_offset);
     mem.flags = slot->flags;
-    if (s->migration_log) {
-        mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
-    }
+
     return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
 }
 
@@ -230,15 +228,22 @@ static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
             return -EINVAL;
     }
 
+    if (flags & KVM_MEM_LOG_DIRTY_PAGES) {
+        if (mem->logging_count++) {
+            return 0;
+        }
+    } else {
+        if (--mem->logging_count) {
+            return 0;
+        }
+    }
+
     old_flags = mem->flags;
 
     flags = (mem->flags & ~mask) | flags;
     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;
     }
@@ -266,8 +271,6 @@ int kvm_set_migration_log(int enable)
     KVMSlot *mem;
     int i, err;
 
-    s->migration_log = enable;
-
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
         mem = &s->slots[i];
 
-- 
1.6.2.2

[Qemu-devel] Re: [PATCH] RFC: use logging count for individual regions

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

Glauber Costa wrote:
> qemu-kvm use this scheme of logging count of individual regions,
> which is, IMHO, more flexible which the one we have right now.
> I'm proposing we use it.
> 
> Anthony, please don't apply this patch yet, as I would want it
> to receive proper testing, and FYI, current migration broken ;(
> - and I don't really have time to go debug it now.
> 
> Jan: Please let me know what you think of it.

No principle concerns. But before looking into details: what additional
use cases will it cover (maybe some example from qemu-kvm), or what
existing code can it help to simplify?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] RFC: use logging count for individual regions

[Glauber Costa]

On Sat, Jul 25, 2009 at 11:28:20AM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > qemu-kvm use this scheme of logging count of individual regions,
> > which is, IMHO, more flexible which the one we have right now.
> > I'm proposing we use it.
> > 
> > Anthony, please don't apply this patch yet, as I would want it
> > to receive proper testing, and FYI, current migration broken ;(
> > - and I don't really have time to go debug it now.
> > 
> > Jan: Please let me know what you think of it.
> 
> No principle concerns. But before looking into details: what additional
> use cases will it cover (maybe some example from qemu-kvm), or what
> existing code can it help to simplify?

Maybe avi can provide more input here, but to the very least, I believe this
approach is more proven, since it lived in qemu-kvm for a while now. Although more
cumbersome, the bits in avi's tree usually work better for kvm-related stuff.

I don't see a particular code path it simplifies, but I believe it can help us finding
bugs that will manifest in the form of an unbalanced count. It will also work if we ever
happen to have two entities manipulating dirty bits in the VGA region, like if we some day
implement dual head or something (although one might arguee that we should change it when
the time comes...)

Btw, a side note: in your current scheme, what we do when migration fail? Do we keep migration_log
up ? I can't find any place in the code where we put it down

[Qemu-devel] Re: [PATCH] RFC: use logging count for individual regions

[Avi Kivity]

On 07/28/2009 04:08 AM, Glauber Costa wrote:
> On Sat, Jul 25, 2009 at 11:28:20AM +0200, Jan Kiszka wrote:
>    
>> Glauber Costa wrote:
>>      
>>> qemu-kvm use this scheme of logging count of individual regions,
>>> which is, IMHO, more flexible which the one we have right now.
>>> I'm proposing we use it.
>>>
>>> Anthony, please don't apply this patch yet, as I would want it
>>> to receive proper testing, and FYI, current migration broken ;(
>>> - and I don't really have time to go debug it now.
>>>
>>> Jan: Please let me know what you think of it.
>>>        
>> No principle concerns. But before looking into details: what additional
>> use cases will it cover (maybe some example from qemu-kvm), or what
>> existing code can it help to simplify?
>>      
>
> Maybe avi can provide more input here, but to the very least, I believe this
> approach is more proven, since it lived in qemu-kvm for a while now. Although more
> cumbersome, the bits in avi's tree usually work better for kvm-related stuff.
>
> I don't see a particular code path it simplifies, but I believe it can help us finding
> bugs that will manifest in the form of an unbalanced count. It will also work if we ever
> happen to have two entities manipulating dirty bits in the VGA region, like if we some day
> implement dual head or something (although one might arguee that we should change it when
> the time comes...)
>    

Unlike the qemu dirty byte-map, the kvm dirty log can only service one 
user.  As we have multiple users (vga and migration), we need some way 
to fix this impedance mismatch.  I think refcounts are the easiest.

> Btw, a side note: in your current scheme, what we do when migration fail? Do we keep migration_log
> up ? I can't find any place in the code where we put it down
>    

What about qemu-kvm?  Do we clear logging there?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH] RFC: use logging count for individual regions

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2252 bytes --]

Avi Kivity wrote:
> On 07/28/2009 04:08 AM, Glauber Costa wrote:
>> On Sat, Jul 25, 2009 at 11:28:20AM +0200, Jan Kiszka wrote:
>>   
>>> Glauber Costa wrote:
>>>     
>>>> qemu-kvm use this scheme of logging count of individual regions,
>>>> which is, IMHO, more flexible which the one we have right now.
>>>> I'm proposing we use it.
>>>>
>>>> Anthony, please don't apply this patch yet, as I would want it
>>>> to receive proper testing, and FYI, current migration broken ;(
>>>> - and I don't really have time to go debug it now.
>>>>
>>>> Jan: Please let me know what you think of it.
>>>>        
>>> No principle concerns. But before looking into details: what additional
>>> use cases will it cover (maybe some example from qemu-kvm), or what
>>> existing code can it help to simplify?
>>>      
>>
>> Maybe avi can provide more input here, but to the very least, I
>> believe this
>> approach is more proven, since it lived in qemu-kvm for a while now.
>> Although more
>> cumbersome, the bits in avi's tree usually work better for kvm-related
>> stuff.
>>
>> I don't see a particular code path it simplifies, but I believe it can
>> help us finding
>> bugs that will manifest in the form of an unbalanced count. It will
>> also work if we ever
>> happen to have two entities manipulating dirty bits in the VGA region,
>> like if we some day
>> implement dual head or something (although one might arguee that we
>> should change it when
>> the time comes...)
>>    
> 
> Unlike the qemu dirty byte-map, the kvm dirty log can only service one
> user.  As we have multiple users (vga and migration), we need some way
> to fix this impedance mismatch.  I think refcounts are the easiest.

This case is already handled upstream. If we had more users, we would
need refcounts.

> 
>> Btw, a side note: in your current scheme, what we do when migration
>> fail? Do we keep migration_log
>> up ? I can't find any place in the code where we put it down
>>    
> 
> What about qemu-kvm?  Do we clear logging there?
> 

I don't think so. In the end, someone has to call
kvm_set_migration_log(0), and so far this only happens in stage 3 which
should not be reached in case of an error.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] RFC: use logging count for individual regions

[Avi Kivity]

On 07/28/2009 09:50 AM, Jan Kiszka wrote:
>
>    
>> Unlike the qemu dirty byte-map, the kvm dirty log can only service one
>> user.  As we have multiple users (vga and migration), we need some way
>> to fix this impedance mismatch.  I think refcounts are the easiest.
>>      
>
> This case is already handled upstream. If we had more users, we would
> need refcounts.
>    

If we turn off logging on migration failure, then we'd introduce a bug 
in vga logging, no?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[Qemu-devel] Re: [PATCH] RFC: use logging count for individual regions

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

Avi Kivity wrote:
> On 07/28/2009 09:50 AM, Jan Kiszka wrote:
>>
>>   
>>> Unlike the qemu dirty byte-map, the kvm dirty log can only service one
>>> user.  As we have multiple users (vga and migration), we need some way
>>> to fix this impedance mismatch.  I think refcounts are the easiest.
>>>      
>>
>> This case is already handled upstream. If we had more users, we would
>> need refcounts.
>>    
> 
> If we turn off logging on migration failure, then we'd introduce a bug
> in vga logging, no?
> 

Nope.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] RFC: use logging count for individual regions

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

Glauber Costa wrote:
> On Sat, Jul 25, 2009 at 11:28:20AM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> qemu-kvm use this scheme of logging count of individual regions,
>>> which is, IMHO, more flexible which the one we have right now.
>>> I'm proposing we use it.
>>>
>>> Anthony, please don't apply this patch yet, as I would want it
>>> to receive proper testing, and FYI, current migration broken ;(
>>> - and I don't really have time to go debug it now.
>>>
>>> Jan: Please let me know what you think of it.
>> No principle concerns. But before looking into details: what additional
>> use cases will it cover (maybe some example from qemu-kvm), or what
>> existing code can it help to simplify?
> 
> Maybe avi can provide more input here, but to the very least, I believe this
> approach is more proven, since it lived in qemu-kvm for a while now. Although more
> cumbersome, the bits in avi's tree usually work better for kvm-related stuff.

As qemu-kvm uses different code here and has different instrumentation
in the devices, it's a bit tricky to asses what it buys upstream. I'm
not against the counting, but I would like to see proper reasoning in
the changelog.

> 
> I don't see a particular code path it simplifies, but I believe it can help us finding
> bugs that will manifest in the form of an unbalanced count. It will also work if we ever
> happen to have two entities manipulating dirty bits in the VGA region, like if we some day
> implement dual head or something (although one might arguee that we should change it when
> the time comes...)
> 
> Btw, a side note: in your current scheme, what we do when migration fail? Do we keep migration_log
> up ? I can't find any place in the code where we put it down

Good question. From a first glance I would say the logging continues...
What does qemu-kvm do on this? Can't find a cleanup there either.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] RFC: use logging count for individual regions

[Anthony Liguori]

Glauber Costa wrote:
> qemu-kvm use this scheme of logging count of individual regions,
> which is, IMHO, more flexible which the one we have right now.
> I'm proposing we use it.
>
> Anthony, please don't apply this patch yet, as I would want it
> to receive proper testing, and FYI, current migration broken ;(
> - and I don't really have time to go debug it now.
>   

FYI, if you use [RFC] in your subject, it will guarantee the patch 
doesn't make it into staging.

But I'll make sure not to apply this.

-- 
Regards,

Anthony Liguori