Patchwork [uq/master,2/5] kvm: add logging count to slots

login
register
mail settings
Submitter Marcelo Tosatti
Date April 23, 2010, 5:04 p.m.
Message ID <20100423170645.675040544@amt.cnet>
Download mbox | patch
Permalink /patch/50850/
State New
Headers show

Comments

Marcelo Tosatti - April 23, 2010, 5:04 p.m.
Otherwise there is no way to differentiate between global and slot 
specific logging, so for example 

vga dirty log start
migration start
migration fail

Disables dirty logging for the vga slot.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Jan Kiszka - April 24, 2010, 7:34 a.m.
Marcelo Tosatti wrote:
> Otherwise there is no way to differentiate between global and slot 
> specific logging, so for example 
> 
> vga dirty log start
> migration start
> migration fail
> 
> Disables dirty logging for the vga slot.

This is not true (unless there is a bug): Migration logging is tracked
via KVMState::migration_log and vga logging via KVMSlot::flags. Both are
merged in kvm_set_user_memory_region. Thus no such change is required
for upstream.

Jan

> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: qemu-kvm/kvm-all.c
> ===================================================================
> --- qemu-kvm.orig/kvm-all.c
> +++ qemu-kvm/kvm-all.c
> @@ -47,6 +47,7 @@ typedef struct KVMSlot
>      ram_addr_t phys_offset;
>      int slot;
>      int flags;
> +    int logging_count;
>  } KVMSlot;
>  
>  typedef struct kvm_dirty_log KVMDirtyLog;
> @@ -218,20 +219,11 @@ 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_dirty_pages_log_change(KVMSlot *mem, int flags, int mask)
>  {
>      KVMState *s = kvm_state;
> -    KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
>      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;
> @@ -250,16 +242,42 @@ static int kvm_dirty_pages_log_change(ta
>  
>  int kvm_log_start(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);
> +    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;
> +    }
> +
> +    if (mem->logging_count++)
> +        return 0;
> +
> +    return kvm_dirty_pages_log_change(mem,
> +                                      KVM_MEM_LOG_DIRTY_PAGES,
> +                                      KVM_MEM_LOG_DIRTY_PAGES);
>  }
>  
>  int kvm_log_stop(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);
> +    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;
> +    }
> +
> +    if (--mem->logging_count)
> +        return 0;
> +
> +    return kvm_dirty_pages_log_change(mem,
> +                                      0,
> +                                      KVM_MEM_LOG_DIRTY_PAGES);
>  }
>  
>  static int kvm_set_migration_log(int enable)
> @@ -273,12 +291,15 @@ static int kvm_set_migration_log(int ena
>      for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
>          mem = &s->slots[i];
>  
> -        if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) {
> -            continue;
> -        }
> -        err = kvm_set_user_memory_region(s, mem);
> -        if (err) {
> -            return err;
> +        if (mem->memory_size) {
> +            if (enable) {
> +                err = kvm_log_start(mem->start_addr, mem->memory_size);
> +            } else {
> +                err = kvm_log_stop(mem->start_addr, mem->memory_size);
> +            }
> +            if (err) {
> +                return err;
> +            }
>          }
>      }
>      return 0;
> @@ -442,6 +463,7 @@ static void kvm_set_phys_mem(target_phys
>  
>          /* unregister the overlapping slot */
>          mem->memory_size = 0;
> +        mem->logging_count = 0;
>          err = kvm_set_user_memory_region(s, mem);
>          if (err) {
>              fprintf(stderr, "%s: error unregistering overlapping slot: %s\n",
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Avi Kivity - April 25, 2010, 12:33 p.m.
On 04/24/2010 10:34 AM, Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>    
>> Otherwise there is no way to differentiate between global and slot
>> specific logging, so for example
>>
>> vga dirty log start
>> migration start
>> migration fail
>>
>> Disables dirty logging for the vga slot.
>>      
> This is not true (unless there is a bug): Migration logging is tracked
> via KVMState::migration_log and vga logging via KVMSlot::flags. Both are
> merged in kvm_set_user_memory_region. Thus no such change is required
> for upstream.
>    

It's still a good idea.  The current API assumes that there will be only 
one slot-based client (or that multiple clients will keep the refcount 
themselves).

After the bytemap -> multiple bitmaps conversion this can be extended to 
each client getting its own bitmap (and therefore, s/refcount/list of 
bitmaps/ and s/!refcount/list_empty()/).
Jan Kiszka - April 25, 2010, 1:57 p.m.
Avi Kivity wrote:
> On 04/24/2010 10:34 AM, Jan Kiszka wrote:
>> Marcelo Tosatti wrote:
>>   
>>> Otherwise there is no way to differentiate between global and slot
>>> specific logging, so for example
>>>
>>> vga dirty log start
>>> migration start
>>> migration fail
>>>
>>> Disables dirty logging for the vga slot.
>>>      
>> This is not true (unless there is a bug): Migration logging is tracked
>> via KVMState::migration_log and vga logging via KVMSlot::flags. Both are
>> merged in kvm_set_user_memory_region. Thus no such change is required
>> for upstream.
>>    
> 
> It's still a good idea.  The current API assumes that there will be only
> one slot-based client (or that multiple clients will keep the refcount
> themselves).
> 
> After the bytemap -> multiple bitmaps conversion this can be extended to
> each client getting its own bitmap (and therefore, s/refcount/list of
> bitmaps/ and s/!refcount/list_empty()/).
> 

No concerns if
 - there is an existing use case for multiple clients, at least in
   qemu-kvm
 - the logging API is consistently converted, not just extended
   (IOW, migration_log is converted to logging_count)
 - someone signs he checked that current use of start/stop in qemu is
   completely symmetrical (I think to remember this used to be not the
   case, but I might be wrong)

Jan
Avi Kivity - April 25, 2010, 2:17 p.m.
On 04/25/2010 04:57 PM, Jan Kiszka wrote:
>
>> It's still a good idea.  The current API assumes that there will be only
>> one slot-based client (or that multiple clients will keep the refcount
>> themselves).
>>
>> After the bytemap ->  multiple bitmaps conversion this can be extended to
>> each client getting its own bitmap (and therefore, s/refcount/list of
>> bitmaps/ and s/!refcount/list_empty()/).
>>
>>      
> No concerns if
>   - there is an existing use case for multiple clients, at least in
>     qemu-kvm
>    

There isn't.  But I don't like hidden breakage.

>   - the logging API is consistently converted, not just extended
>     (IOW, migration_log is converted to logging_count)
>    

migration_log needs to remain global, since we want hotplug memory to 
autostart logging.

>   - someone signs he checked that current use of start/stop in qemu is
>     completely symmetrical (I think to remember this used to be not the
>     case, but I might be wrong)
>    

I remember this too.  Marcelo?
Jan Kiszka - April 25, 2010, 2:29 p.m.
Avi Kivity wrote:
> On 04/25/2010 04:57 PM, Jan Kiszka wrote:
>>
>>> It's still a good idea.  The current API assumes that there will be only
>>> one slot-based client (or that multiple clients will keep the refcount
>>> themselves).
>>>
>>> After the bytemap ->  multiple bitmaps conversion this can be
>>> extended to
>>> each client getting its own bitmap (and therefore, s/refcount/list of
>>> bitmaps/ and s/!refcount/list_empty()/).
>>>
>>>      
>> No concerns if
>>   - there is an existing use case for multiple clients, at least in
>>     qemu-kvm
>>    
> 
> There isn't.  But I don't like hidden breakage.

It's (so far) an unproblematic API property we can document. I don't
like changing APIs just for "there might be the case that...".

> 
>>   - the logging API is consistently converted, not just extended
>>     (IOW, migration_log is converted to logging_count)
>>    
> 
> migration_log needs to remain global, since we want hotplug memory to
> autostart logging.

Can't follow yet, what will be the usage pattern of
kvm_set_migration_log? Or would the hotplug code require a separate
interface? Is it already the multi-client use case I'm looking for?

> 
>>   - someone signs he checked that current use of start/stop in qemu is
>>     completely symmetrical (I think to remember this used to be not the
>>     case, but I might be wrong)
>>    
> 
> I remember this too.  Marcelo?
> 

Jan
Avi Kivity - April 25, 2010, 2:41 p.m.
On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>
>> There isn't.  But I don't like hidden breakage.
>>      
> It's (so far) an unproblematic API property we can document. I don't
> like changing APIs just for "there might be the case that...".
>    

I guess it's one of those agree to disagree things.  I dislike known 
broken APIs even if their none of their users are affected.

>>>    - the logging API is consistently converted, not just extended
>>>      (IOW, migration_log is converted to logging_count)
>>>
>>>        
>> migration_log needs to remain global, since we want hotplug memory to
>> autostart logging.
>>      
> Can't follow yet, what will be the usage pattern of
> kvm_set_migration_log? Or would the hotplug code require a separate
> interface? Is it already the multi-client use case I'm looking for?
>    

kvm_set_migration_log() means, start logging now for all current and 
future memory, until disabled.

It could be implemented in terms of kvm_log_start() (which would provide 
a multi-client use case), but it isn't now.

I guess it is a logical example of how two clients can exist, even 
though they don't step on each others toes in practice since their 
enable flags are kept separate by the implementation.
Jan Kiszka - April 25, 2010, 2:51 p.m.
Avi Kivity wrote:
> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>
>>> There isn't.  But I don't like hidden breakage.
>>>      
>> It's (so far) an unproblematic API property we can document. I don't
>> like changing APIs just for "there might be the case that...".
>>    
> 
> I guess it's one of those agree to disagree things.  I dislike known
> broken APIs even if their none of their users are affected.

The API is not broken. I intentionally designed it for the single user
as I saw no need for more. If I oversaw something, I would really like
to learn about these cases.

> 
>>>>    - the logging API is consistently converted, not just extended
>>>>      (IOW, migration_log is converted to logging_count)
>>>>
>>>>        
>>> migration_log needs to remain global, since we want hotplug memory to
>>> autostart logging.
>>>      
>> Can't follow yet, what will be the usage pattern of
>> kvm_set_migration_log? Or would the hotplug code require a separate
>> interface? Is it already the multi-client use case I'm looking for?
>>    
> 
> kvm_set_migration_log() means, start logging now for all current and
> future memory, until disabled.

Hmm, you mean plugging memory during ongoing migration is valid and can
be handled? I'm a bit skeptical. What makes this different from, say,
PCI hotplugging which should be a no-go during migration as well?

> 
> It could be implemented in terms of kvm_log_start() (which would provide
> a multi-client use case), but it isn't now.
> 
> I guess it is a logical example of how two clients can exist, even
> though they don't step on each others toes in practice since their
> enable flags are kept separate by the implementation.
> 

Jan
Avi Kivity - April 25, 2010, 2:58 p.m.
On 04/25/2010 05:51 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>      
>>>        
>>>> There isn't.  But I don't like hidden breakage.
>>>>
>>>>          
>>> It's (so far) an unproblematic API property we can document. I don't
>>> like changing APIs just for "there might be the case that...".
>>>
>>>        
>> I guess it's one of those agree to disagree things.  I dislike known
>> broken APIs even if their none of their users are affected.
>>      
> The API is not broken. I intentionally designed it for the single user
> as I saw no need for more. If I oversaw something, I would really like
> to learn about these cases.
>    

The fact that the API assumes a single user is what's broken IMO.

If the API were to take a memory slot as parameter you could say it is 
the responsibility of the slot's owner to multiplex (and since vga has a 
single owner, no need to multiplex).  But it takes a range.

>> kvm_set_migration_log() means, start logging now for all current and
>> future memory, until disabled.
>>      
> Hmm, you mean plugging memory during ongoing migration is valid and can
> be handled?

Sure (except that we don't have memory hotplug).

> I'm a bit skeptical. What makes this different from, say,
> PCI hotplugging which should be a no-go during migration as well?
>
>    

PCI hotplugging should be handled in migration as well.  Introducing 
dependencies among unrelated features and expecting upper layers to 
apply the correct constraints is unreasonable.

Currently we don't handle this, but we should.  One way to do it is to 
forward the hotplug/hotunplug along the migration channel.
Jan Kiszka - April 25, 2010, 3:07 p.m.
Avi Kivity wrote:
> On 04/25/2010 05:51 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 04/25/2010 05:29 PM, Jan Kiszka wrote:
>>>     
>>>>       
>>>>> There isn't.  But I don't like hidden breakage.
>>>>>
>>>>>          
>>>> It's (so far) an unproblematic API property we can document. I don't
>>>> like changing APIs just for "there might be the case that...".
>>>>
>>>>        
>>> I guess it's one of those agree to disagree things.  I dislike known
>>> broken APIs even if their none of their users are affected.
>>>      
>> The API is not broken. I intentionally designed it for the single user
>> as I saw no need for more. If I oversaw something, I would really like
>> to learn about these cases.
>>    
> 
> The fact that the API assumes a single user is what's broken IMO.
> 
> If the API were to take a memory slot as parameter you could say it is
> the responsibility of the slot's owner to multiplex (and since vga has a
> single owner, no need to multiplex).  But it takes a range.

No, the API accepts only a single slot. If you try passing bogus ranges
that span multiple or incomplete slots, you get what you deserve - a bug
message.

Jan
Avi Kivity - April 25, 2010, 3:22 p.m.
On 04/25/2010 06:07 PM, Jan Kiszka wrote:
>
>> The fact that the API assumes a single user is what's broken IMO.
>>
>> If the API were to take a memory slot as parameter you could say it is
>> the responsibility of the slot's owner to multiplex (and since vga has a
>> single owner, no need to multiplex).  But it takes a range.
>>      
> No, the API accepts only a single slot. If you try passing bogus ranges
> that span multiple or incomplete slots, you get what you deserve - a bug
> message.
>    

I see.  In its qemu-kvm iteration, it would iterate over slots and 
accept incomplete slots (it's okay to log more than requested).  If the 
API is for a slot, it should accept a slot, not a range (when we move to 
a slots representation in qemu).

Unrelated:

         cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa0000, 0xa8000);
         cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa8000, 0xb0000);

Will this sync to the right place (whatever those windows alias to)?
Jan Kiszka - April 25, 2010, 4:42 p.m.
Avi Kivity wrote:
> On 04/25/2010 06:07 PM, Jan Kiszka wrote:
>>
>>> The fact that the API assumes a single user is what's broken IMO.
>>>
>>> If the API were to take a memory slot as parameter you could say it is
>>> the responsibility of the slot's owner to multiplex (and since vga has a
>>> single owner, no need to multiplex).  But it takes a range.
>>>      
>> No, the API accepts only a single slot. If you try passing bogus ranges
>> that span multiple or incomplete slots, you get what you deserve - a bug
>> message.
>>    
> 
> I see.  In its qemu-kvm iteration, it would iterate over slots and
> accept incomplete slots (it's okay to log more than requested).  If the
> API is for a slot, it should accept a slot, not a range (when we move to
> a slots representation in qemu).

Yes, an explicit slot reference in the API would be clearer.

> 
> Unrelated:
> 
>         cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa0000, 0xa8000);
>         cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa8000, 0xb0000);
> 
> Will this sync to the right place (whatever those windows alias to)?
> 

It should. Or where do your worries come from?

Jan
Avi Kivity - April 26, 2010, 5:37 a.m.
On 04/25/2010 07:42 PM, Jan Kiszka wrote:
>
>> Unrelated:
>>
>>          cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa0000, 0xa8000);
>>          cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa8000, 0xb0000);
>>
>> Will this sync to the right place (whatever those windows alias to)?
>>
>>      
> It should. Or where do your worries come from?
>
>    

I was worried the bitmap was indexed by physical addresses, but now I 
remember it is indexed by ram addresses, so it all works out.
Marcelo Tosatti - April 26, 2010, 1:47 p.m.
On Sun, Apr 25, 2010 at 05:17:55PM +0300, Avi Kivity wrote:
> On 04/25/2010 04:57 PM, Jan Kiszka wrote:
> >
> >>It's still a good idea.  The current API assumes that there will be only
> >>one slot-based client (or that multiple clients will keep the refcount
> >>themselves).
> >>
> >>After the bytemap ->  multiple bitmaps conversion this can be extended to
> >>each client getting its own bitmap (and therefore, s/refcount/list of
> >>bitmaps/ and s/!refcount/list_empty()/).
> >>
> >No concerns if
> >  - there is an existing use case for multiple clients, at least in
> >    qemu-kvm
> 
> There isn't.  But I don't like hidden breakage.
> 
> >  - the logging API is consistently converted, not just extended
> >    (IOW, migration_log is converted to logging_count)
> 
> migration_log needs to remain global, since we want hotplug memory
> to autostart logging.
> 
> >  - someone signs he checked that current use of start/stop in qemu is
> >    completely symmetrical (I think to remember this used to be not the
> >    case, but I might be wrong)
> 
> I remember this too.  Marcelo?

Don't see any guarantee that it is symmetrical. Anyway, will drop 
the patch from the series.

Patch

Index: qemu-kvm/kvm-all.c
===================================================================
--- qemu-kvm.orig/kvm-all.c
+++ qemu-kvm/kvm-all.c
@@ -47,6 +47,7 @@  typedef struct KVMSlot
     ram_addr_t phys_offset;
     int slot;
     int flags;
+    int logging_count;
 } KVMSlot;
 
 typedef struct kvm_dirty_log KVMDirtyLog;
@@ -218,20 +219,11 @@  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_dirty_pages_log_change(KVMSlot *mem, int flags, int mask)
 {
     KVMState *s = kvm_state;
-    KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
     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;
@@ -250,16 +242,42 @@  static int kvm_dirty_pages_log_change(ta
 
 int kvm_log_start(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);
+    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;
+    }
+
+    if (mem->logging_count++)
+        return 0;
+
+    return kvm_dirty_pages_log_change(mem,
+                                      KVM_MEM_LOG_DIRTY_PAGES,
+                                      KVM_MEM_LOG_DIRTY_PAGES);
 }
 
 int kvm_log_stop(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);
+    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;
+    }
+
+    if (--mem->logging_count)
+        return 0;
+
+    return kvm_dirty_pages_log_change(mem,
+                                      0,
+                                      KVM_MEM_LOG_DIRTY_PAGES);
 }
 
 static int kvm_set_migration_log(int enable)
@@ -273,12 +291,15 @@  static int kvm_set_migration_log(int ena
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
         mem = &s->slots[i];
 
-        if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) {
-            continue;
-        }
-        err = kvm_set_user_memory_region(s, mem);
-        if (err) {
-            return err;
+        if (mem->memory_size) {
+            if (enable) {
+                err = kvm_log_start(mem->start_addr, mem->memory_size);
+            } else {
+                err = kvm_log_stop(mem->start_addr, mem->memory_size);
+            }
+            if (err) {
+                return err;
+            }
         }
     }
     return 0;
@@ -442,6 +463,7 @@  static void kvm_set_phys_mem(target_phys
 
         /* unregister the overlapping slot */
         mem->memory_size = 0;
+        mem->logging_count = 0;
         err = kvm_set_user_memory_region(s, mem);
         if (err) {
             fprintf(stderr, "%s: error unregistering overlapping slot: %s\n",