Re: [PATCH 1/3] migration: Teach dirtyrate about qemu_target_page_size()

[gudkov.andrei]

On Thu, May 11, 2023 at 03:12:24PM +0200, Juan Quintela wrote:
> Richard Henderson <richard.henderson@linaro.org> wrote:
> > On 5/11/23 10:22, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>   migration/dirtyrate.c | 11 ++++++-----
> >>   1 file changed, 6 insertions(+), 5 deletions(-)
> >> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> >> index 180ba38c7a..9aa092738c 100644
> >> --- a/migration/dirtyrate.c
> >> +++ b/migration/dirtyrate.c
> >> @@ -17,6 +17,7 @@
> >>   #include "cpu.h"
> >>   #include "exec/ramblock.h"
> >>   #include "exec/ram_addr.h"
> >> +#include "exec/target_page.h"
> >>   #include "qemu/rcu_queue.h"
> >>   #include "qemu/main-loop.h"
> >>   #include "qapi/qapi-commands-migration.h"
> >> @@ -78,7 +79,7 @@ static int64_t do_calculate_dirtyrate(DirtyPageRecord 
> >> dirty_pages,
> >>       uint64_t increased_dirty_pages =
> >>           dirty_pages.end_pages - dirty_pages.start_pages;
> >>   -    memory_size_MB = (increased_dirty_pages * TARGET_PAGE_SIZE)
> >> >> 20;
> >> +    memory_size_MB = (increased_dirty_pages * qemu_target_page_size()) >> 
> >> 20;
> >
> > See the recent cleanups for dirtylimit_dirty_ring_full_time, folding
> > multiply+shift into subtract+shift.
> 
> I reviewed it and I had already forgotten!!
> 
> >>         return memory_size_MB * 1000 / calc_time_ms;
> >>   }
> >> @@ -291,8 +292,8 @@ static void update_dirtyrate_stat(struct 
> >> RamblockDirtyInfo *info)
> >>       DirtyStat.page_sampling.total_dirty_samples += 
> >> info->sample_dirty_count;
> >>       DirtyStat.page_sampling.total_sample_count += 
> >> info->sample_pages_count;
> >>       /* size of total pages in MB */
> >> -    DirtyStat.page_sampling.total_block_mem_MB += (info->ramblock_pages *
> >> -                                                   TARGET_PAGE_SIZE) >> 
> >> 20;
> >> +    DirtyStat.page_sampling.total_block_mem_MB +=
> >> +        (info->ramblock_pages * qemu_target_page_size()) >> 20;
> >
> > And a third copy?
> > Can we abstract this somewhere?
> 
> I ended with this:
> 
> /* Convert target pages to MiB (2**20). */
> size_t qemu_target_pages_to_MiB(size_t pages)
> {
>     int page_bits = TARGET_PAGE_BITS;
> 
>     /* So far, the largest (non-huge) page size is 64k, i.e. 16 bits. */
>     g_assert(page_bits < 20);
> 
>     return pages >> (20 - page_bits);
> }
> 
> But only found 3 users, the one that you did and this two.

In fact, there is no need to convert VM size into MB inside 
update_dirtyrate_stat().
The unit of total_block_mem_MB can be changed to pages (or bytes).
This will leave update_dirtyrate() as the only user of the proposed function.

> 
> Will resend the series on top of this.
> 
> Thanks, Juan.