Re: [Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mask to optimize dirty tracking

[Fam Zheng]

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 <address@hidden>