| Message ID | 20100423170645.675040544@amt.cnet |
|---|---|
| State | New |
| Headers | show |
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 >
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()/).
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
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?
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
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.
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
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.
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
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)?
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
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.
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.
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",
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>