Re: [Qemu-devel] [PATCH 7/8] dirty bitmap: abstract its use

[Orit Wasserman]

On 06/25/2012 04:20 PM, Juan Quintela wrote:
> Orit Wasserman <address@hidden> wrote:
>> On 06/22/2012 04:46 PM, Juan Quintela wrote:
>>> Always use accessors to read/set the dirty bitmap.
>>>
>>> Signed-off-by: Juan Quintela <address@hidden>
>>> -static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
>>> +/* read dirty bit (return 0 or 1) */
>>> +static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
>>>  {
>>> -    return ram_list.phys_dirty[addr >> TARGET_PAGE_BITS];
>>> +    return cpu_physical_memory_get_dirty_flags(addr) == 0xff;
>>>  }
>> Juan,
>> you changed the order of the functions , can your restore the order as it 
>> was.
> 
> There is reason after madness.  Now cpu_physical_memory_is_dirty calls
> cpu_physical_memory_get_dirty_flags().  After this patch, only 4
> functions touch the bitmap directly.
ok.
>>> index a68b65c..dd4833d 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -2565,8 +2565,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, 
>>> void *host,
>>>
>>>      ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
>>>                                         last_ram_offset() >> 
>>> TARGET_PAGE_BITS);
>>> -    memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
>>> -           0xff, size >> TARGET_PAGE_BITS);
>>> +    cpu_physical_memory_set_dirty_range(new_block->offset, size, 0xff);
>>
>> This will be much slower than memset , do we use it much ?
> 
> No, we only use it each time that we add another region of RAM (i.e. a
> very rare event).
if it is rare , than it is fine
> 
> Next step is to change the bitmap to three bitmaps of 1 bit per page.
> That way, we only need to look at that 4 functions for changing it.
> 
Great.
> On an un-related note, I can't see how migration + TCG can work at the
> moment.  TCG uses "*is_dirty()" to see if the page has changed, but if
> we have migrated that page, is_dirty is going to return _not-dirty_.
> AKA as bad as it can be :-(
> 
> Later, Juan.)

Reviewed-by: Orit Wasserman <address@hidden>