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

[Paolo Bonzini <pbonzini@redhat.com>]

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
>>
>>