| Message ID | 1430152117-100558-15-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 04/27 18:28, Paolo Bonzini wrote: > The memory API can now return the exact set of bitmaps that have to > be tracked. Use it instead of the in_migration variable. > > In the next patches, we will also use it to set only DIRTY_MEMORY_VGA > or DIRTY_MEMORY_MIGRATION if necessary. This can make a difference > for dataplane, especially after the dirty bitmap is changed to use > more expensive atomic operations. > > Of some interest is the change to stl_phys_notdirty. When migration > was introduced, stl_phys_notdirty was changed to effectively behave > as stl_phys during migration. In fact, if one looks at the function as it > was in the beginning (commit 8df1cd0, physical memory access functions, > 2005-01-28), at the time the dirty bitmap was the equivalent of > DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch > the dirty code bits. This patch changes it to do the intended thing. There are three changes in this patch: 1) Removal of core_memory_listener; 2) Test of dirty log mask bits in invalidate_and_set_dirty; 3) Test of dirty log mask bits in stl_phys_notdirty. 1) and 3) are connected by in_migration, so they belong to the same patch. But I'm not sure about 2). Is it required by 1) and 3), or it's changed because it also touches the condition of tb_invalidate_phys_range? > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > exec.c | 59 +++++++++++++++++++---------------------------------------- > 1 file changed, 19 insertions(+), 40 deletions(-) > > diff --git a/exec.c b/exec.c > index 083d65d..6e83161 100644 > --- a/exec.c > +++ b/exec.c > @@ -59,8 +59,6 @@ > //#define DEBUG_SUBPAGE > > #if !defined(CONFIG_USER_ONLY) > -static bool in_migration; > - > /* ram_list is read under rcu_read_lock()/rcu_read_unlock(). Writes > * are protected by the ramlist lock. > */ > @@ -871,11 +869,6 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length, > } > } > > -static void cpu_physical_memory_set_dirty_tracking(bool enable) > -{ > - in_migration = enable; > -} > - > /* Called from RCU critical section */ > hwaddr memory_region_section_get_iotlb(CPUState *cpu, > MemoryRegionSection *section, > @@ -2140,22 +2133,6 @@ static void tcg_commit(MemoryListener *listener) > } > } > > -static void core_log_global_start(MemoryListener *listener) > -{ > - cpu_physical_memory_set_dirty_tracking(true); > -} > - > -static void core_log_global_stop(MemoryListener *listener) > -{ > - cpu_physical_memory_set_dirty_tracking(false); > -} > - > -static MemoryListener core_memory_listener = { > - .log_global_start = core_log_global_start, > - .log_global_stop = core_log_global_stop, > - .priority = 1, > -}; > - > void address_space_init_dispatch(AddressSpace *as) > { > as->dispatch = NULL; > @@ -2195,8 +2172,6 @@ static void memory_map_init(void) > memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io", > 65536); > address_space_init(&address_space_io, system_io, "I/O"); > - > - memory_listener_register(&core_memory_listener, &address_space_memory); > } > > MemoryRegion *get_system_memory(void) > @@ -2254,12 +2229,18 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr, > > #else > > -static void invalidate_and_set_dirty(hwaddr addr, > +static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr, > hwaddr length) > { > if (cpu_physical_memory_range_includes_clean(addr, length)) { > - tb_invalidate_phys_range(addr, addr + length, 0); > - cpu_physical_memory_set_dirty_range_nocode(addr, length); > + uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr); > + if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) { > + tb_invalidate_phys_range(addr, addr + length, 0); > + dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE); > + } > + if (dirty_log_mask) { > + cpu_physical_memory_set_dirty_range_nocode(addr, length); > + } > } else { > xen_modified_memory(addr, length); > } > @@ -2342,7 +2323,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf, > /* RAM case */ > ptr = qemu_get_ram_ptr(addr1); > memcpy(ptr, buf, l); > - invalidate_and_set_dirty(addr1, l); > + invalidate_and_set_dirty(mr, addr1, l); > } > } else { > if (!memory_access_is_direct(mr, is_write)) { > @@ -2431,7 +2412,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as, > switch (type) { > case WRITE_DATA: > memcpy(ptr, buf, l); > - invalidate_and_set_dirty(addr1, l); > + invalidate_and_set_dirty(mr, addr1, l); > break; > case FLUSH_CACHE: > flush_icache_range((uintptr_t)ptr, (uintptr_t)ptr + l); > @@ -2647,7 +2628,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len, > mr = qemu_ram_addr_from_host(buffer, &addr1); > assert(mr != NULL); > if (is_write) { > - invalidate_and_set_dirty(addr1, access_len); > + invalidate_and_set_dirty(mr, addr1, access_len); > } > if (xen_enabled()) { > xen_invalidate_map_cache_entry(buffer); > @@ -2871,6 +2852,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val) > MemoryRegion *mr; > hwaddr l = 4; > hwaddr addr1; > + uint8_t dirty_log_mask; > > mr = address_space_translate(as, addr, &addr1, &l, > true); > @@ -2881,13 +2863,10 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val) > ptr = qemu_get_ram_ptr(addr1); > stl_p(ptr, val); > > - if (unlikely(in_migration)) { > - if (cpu_physical_memory_is_clean(addr1)) { > - /* invalidate code */ > - tb_invalidate_phys_page_range(addr1, addr1 + 4, 0); > - /* set dirty bit */ > - cpu_physical_memory_set_dirty_range_nocode(addr1, 4); > - } > + dirty_log_mask = memory_region_get_dirty_log_mask(mr); > + dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE); > + if (dirty_log_mask) { > + cpu_physical_memory_set_dirty_range_nocode(addr1, 4); Looks OK. A side question, it seems cpu_physical_memory_is_clean returns true if *any* of three bitmaps is clean: static inline bool cpu_physical_memory_is_clean(ram_addr_t addr) { bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA); bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE); bool migration = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRATION); -> return !(vga && code && migration); } It's counter-intuitive. Why is that? Fam > } > } > } > @@ -2930,7 +2909,7 @@ static inline void stl_phys_internal(AddressSpace *as, > stl_p(ptr, val); > break; > } > - invalidate_and_set_dirty(addr1, 4); > + invalidate_and_set_dirty(mr, addr1, 4); > } > } > > @@ -2993,7 +2972,7 @@ static inline void stw_phys_internal(AddressSpace *as, > stw_p(ptr, val); > break; > } > - invalidate_and_set_dirty(addr1, 2); > + invalidate_and_set_dirty(mr, addr1, 2); > } > } > > -- > 1.8.3.1 > >
On 26/05/2015 12:42, Fam Zheng wrote: > On Mon, 04/27 18:28, Paolo Bonzini wrote: >> The memory API can now return the exact set of bitmaps that have to >> be tracked. Use it instead of the in_migration variable. >> >> In the next patches, we will also use it to set only DIRTY_MEMORY_VGA >> or DIRTY_MEMORY_MIGRATION if necessary. This can make a difference >> for dataplane, especially after the dirty bitmap is changed to use >> more expensive atomic operations. >> >> Of some interest is the change to stl_phys_notdirty. When migration >> was introduced, stl_phys_notdirty was changed to effectively behave >> as stl_phys during migration. In fact, if one looks at the function as it >> was in the beginning (commit 8df1cd0, physical memory access functions, >> 2005-01-28), at the time the dirty bitmap was the equivalent of >> DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch >> the dirty code bits. This patch changes it to do the intended thing. > > There are three changes in this patch: > > 1) Removal of core_memory_listener; > 2) Test of dirty log mask bits in invalidate_and_set_dirty; > 3) Test of dirty log mask bits in stl_phys_notdirty. > > 1) and 3) are connected by in_migration, so they belong to the same patch. But > I'm not sure about 2). Is it required by 1) and 3), or it's changed because it > also touches the condition of tb_invalidate_phys_range? The idea was really to put together (2) and (3), which are connected by memory_region_get_dirty_log_mask and cpu_physical_memory_set_dirty_range_nocode. The difference is that (2) calls tb_invalidate_phys_range, while (3) does not (because it is "_notdirty"). (1) is just dead code removal. > Looks OK. > > A side question, it seems cpu_physical_memory_is_clean returns true if *any* of > three bitmaps is clean: > > static inline bool cpu_physical_memory_is_clean(ram_addr_t addr) > { > bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA); > bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE); > bool migration = > cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRATION); > -> return !(vga && code && migration); > } > > It's counter-intuitive. Why is that? If any bit is clean, writes need trapping in order to set those bits. Remember that the code in address_space_stl_notdirty didn't really make sense before this patch, so do not read much into it. At the end of the series, cpu_physical_memory_is_clean is only used from notdirty_mem_write and tlb_set_page_with_attrs, i.e. only from TCG. That is more understandable. Perhaps we can rename it to cpu_physical_memory_needs_notdirty. Paolo > > Fam > >> } >> } >> } >> @@ -2930,7 +2909,7 @@ static inline void stl_phys_internal(AddressSpace *as, >> stl_p(ptr, val); >> break; >> } >> - invalidate_and_set_dirty(addr1, 4); >> + invalidate_and_set_dirty(mr, addr1, 4); >> } >> } >> >> @@ -2993,7 +2972,7 @@ static inline void stw_phys_internal(AddressSpace *as, >> stw_p(ptr, val); >> break; >> } >> - invalidate_and_set_dirty(addr1, 2); >> + invalidate_and_set_dirty(mr, addr1, 2); >> } >> } >> >> -- >> 1.8.3.1 >> >>
On Tue, 05/26 12:58, Paolo Bonzini wrote: > > > On 26/05/2015 12:42, Fam Zheng wrote: > > On Mon, 04/27 18:28, Paolo Bonzini wrote: > >> The memory API can now return the exact set of bitmaps that have to > >> be tracked. Use it instead of the in_migration variable. > >> > >> In the next patches, we will also use it to set only DIRTY_MEMORY_VGA > >> or DIRTY_MEMORY_MIGRATION if necessary. This can make a difference > >> for dataplane, especially after the dirty bitmap is changed to use > >> more expensive atomic operations. > >> > >> Of some interest is the change to stl_phys_notdirty. When migration > >> was introduced, stl_phys_notdirty was changed to effectively behave > >> as stl_phys during migration. In fact, if one looks at the function as it > >> was in the beginning (commit 8df1cd0, physical memory access functions, > >> 2005-01-28), at the time the dirty bitmap was the equivalent of > >> DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch > >> the dirty code bits. This patch changes it to do the intended thing. > > > > There are three changes in this patch: > > > > 1) Removal of core_memory_listener; > > 2) Test of dirty log mask bits in invalidate_and_set_dirty; > > 3) Test of dirty log mask bits in stl_phys_notdirty. > > > > 1) and 3) are connected by in_migration, so they belong to the same patch. But > > I'm not sure about 2). Is it required by 1) and 3), or it's changed because it > > also touches the condition of tb_invalidate_phys_range? > > The idea was really to put together (2) and (3), which are connected by > memory_region_get_dirty_log_mask and > cpu_physical_memory_set_dirty_range_nocode. The difference is that (2) > calls tb_invalidate_phys_range, while (3) does not (because it is > "_notdirty"). > > (1) is just dead code removal. > > > Looks OK. > > > > A side question, it seems cpu_physical_memory_is_clean returns true if *any* of > > three bitmaps is clean: > > > > static inline bool cpu_physical_memory_is_clean(ram_addr_t addr) > > { > > bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA); > > bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE); > > bool migration = > > cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRATION); > > -> return !(vga && code && migration); > > } > > > > It's counter-intuitive. Why is that? > > If any bit is clean, writes need trapping in order to set those bits. Yes. > > Remember that the code in address_space_stl_notdirty didn't really make > sense before this patch, so do not read much into it. At the end of the > series, cpu_physical_memory_is_clean is only used from > notdirty_mem_write and tlb_set_page_with_attrs, i.e. only from TCG. > That is more understandable. Perhaps we can rename it to > cpu_physical_memory_needs_notdirty. Thanks, that complements my reading! Reviewed-by: Fam Zheng <famz@redhat.com>
diff --git a/exec.c b/exec.c index 083d65d..6e83161 100644 --- a/exec.c +++ b/exec.c @@ -59,8 +59,6 @@ //#define DEBUG_SUBPAGE #if !defined(CONFIG_USER_ONLY) -static bool in_migration; - /* ram_list is read under rcu_read_lock()/rcu_read_unlock(). Writes * are protected by the ramlist lock. */ @@ -871,11 +869,6 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length, } } -static void cpu_physical_memory_set_dirty_tracking(bool enable) -{ - in_migration = enable; -} - /* Called from RCU critical section */ hwaddr memory_region_section_get_iotlb(CPUState *cpu, MemoryRegionSection *section, @@ -2140,22 +2133,6 @@ static void tcg_commit(MemoryListener *listener) } } -static void core_log_global_start(MemoryListener *listener) -{ - cpu_physical_memory_set_dirty_tracking(true); -} - -static void core_log_global_stop(MemoryListener *listener) -{ - cpu_physical_memory_set_dirty_tracking(false); -} - -static MemoryListener core_memory_listener = { - .log_global_start = core_log_global_start, - .log_global_stop = core_log_global_stop, - .priority = 1, -}; - void address_space_init_dispatch(AddressSpace *as) { as->dispatch = NULL; @@ -2195,8 +2172,6 @@ static void memory_map_init(void) memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io", 65536); address_space_init(&address_space_io, system_io, "I/O"); - - memory_listener_register(&core_memory_listener, &address_space_memory); } MemoryRegion *get_system_memory(void) @@ -2254,12 +2229,18 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr, #else -static void invalidate_and_set_dirty(hwaddr addr, +static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr, hwaddr length) { if (cpu_physical_memory_range_includes_clean(addr, length)) { - tb_invalidate_phys_range(addr, addr + length, 0); - cpu_physical_memory_set_dirty_range_nocode(addr, length); + uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr); + if (dirty_log_mask & (1 << DIRTY_MEMORY_CODE)) { + tb_invalidate_phys_range(addr, addr + length, 0); + dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE); + } + if (dirty_log_mask) { + cpu_physical_memory_set_dirty_range_nocode(addr, length); + } } else { xen_modified_memory(addr, length); } @@ -2342,7 +2323,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf, /* RAM case */ ptr = qemu_get_ram_ptr(addr1); memcpy(ptr, buf, l); - invalidate_and_set_dirty(addr1, l); + invalidate_and_set_dirty(mr, addr1, l); } } else { if (!memory_access_is_direct(mr, is_write)) { @@ -2431,7 +2412,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as, switch (type) { case WRITE_DATA: memcpy(ptr, buf, l); - invalidate_and_set_dirty(addr1, l); + invalidate_and_set_dirty(mr, addr1, l); break; case FLUSH_CACHE: flush_icache_range((uintptr_t)ptr, (uintptr_t)ptr + l); @@ -2647,7 +2628,7 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len, mr = qemu_ram_addr_from_host(buffer, &addr1); assert(mr != NULL); if (is_write) { - invalidate_and_set_dirty(addr1, access_len); + invalidate_and_set_dirty(mr, addr1, access_len); } if (xen_enabled()) { xen_invalidate_map_cache_entry(buffer); @@ -2871,6 +2852,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val) MemoryRegion *mr; hwaddr l = 4; hwaddr addr1; + uint8_t dirty_log_mask; mr = address_space_translate(as, addr, &addr1, &l, true); @@ -2881,13 +2863,10 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val) ptr = qemu_get_ram_ptr(addr1); stl_p(ptr, val); - if (unlikely(in_migration)) { - if (cpu_physical_memory_is_clean(addr1)) { - /* invalidate code */ - tb_invalidate_phys_page_range(addr1, addr1 + 4, 0); - /* set dirty bit */ - cpu_physical_memory_set_dirty_range_nocode(addr1, 4); - } + dirty_log_mask = memory_region_get_dirty_log_mask(mr); + dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE); + if (dirty_log_mask) { + cpu_physical_memory_set_dirty_range_nocode(addr1, 4); } } } @@ -2930,7 +2909,7 @@ static inline void stl_phys_internal(AddressSpace *as, stl_p(ptr, val); break; } - invalidate_and_set_dirty(addr1, 4); + invalidate_and_set_dirty(mr, addr1, 4); } } @@ -2993,7 +2972,7 @@ static inline void stw_phys_internal(AddressSpace *as, stw_p(ptr, val); break; } - invalidate_and_set_dirty(addr1, 2); + invalidate_and_set_dirty(mr, addr1, 2); } }
The memory API can now return the exact set of bitmaps that have to be tracked. Use it instead of the in_migration variable. In the next patches, we will also use it to set only DIRTY_MEMORY_VGA or DIRTY_MEMORY_MIGRATION if necessary. This can make a difference for dataplane, especially after the dirty bitmap is changed to use more expensive atomic operations. Of some interest is the change to stl_phys_notdirty. When migration was introduced, stl_phys_notdirty was changed to effectively behave as stl_phys during migration. In fact, if one looks at the function as it was in the beginning (commit 8df1cd0, physical memory access functions, 2005-01-28), at the time the dirty bitmap was the equivalent of DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch the dirty code bits. This patch changes it to do the intended thing. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- exec.c | 59 +++++++++++++++++++---------------------------------------- 1 file changed, 19 insertions(+), 40 deletions(-)