[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mas
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mask to optimize dirty tracking |
Date: |
Tue, 26 May 2015 12:58:40 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 |
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
>>
>>