[PATCH 0/3] migration: Make dirtyrate.c target independent

[PATCH 0/3] migration: Make dirtyrate.c target independent

[Juan Quintela]

Hi

The only features that it used that are target dependent are page_size
and page_bits, both of what have an accessor for target independent
code.

Please review, Juan.

Juan Quintela (3):
  migration: Teach dirtyrate about qemu_target_page_size()
  migration: Teach dirtyrate about qemu_target_page_bits()
  migration: Make dirtyrate.c target independent

 migration/dirtyrate.c | 17 ++++++++---------
 migration/meson.build |  5 ++---
 2 files changed, 10 insertions(+), 12 deletions(-)

-- 
2.40.1

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

[Juan Quintela]

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;
 
     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;
 }
 
 static void update_dirtyrate(uint64_t msec)
@@ -315,10 +316,10 @@ static void update_dirtyrate(uint64_t msec)
 static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
                                       uint64_t vfn)
 {
+    int page_size = qemu_target_page_size();
     uint32_t crc;
 
-    crc = crc32(0, (info->ramblock_addr +
-                vfn * TARGET_PAGE_SIZE), TARGET_PAGE_SIZE);
+    crc = crc32(0, info->ramblock_addr + vfn * page_size, page_size);
 
     trace_get_ramblock_vfn_hash(info->idstr, vfn, crc);
     return crc;
-- 
2.40.1

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

[Richard Henderson]

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.

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


r~

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

[Juan Quintela]

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

This is for dirtyrate folks.  But I agree that having to do this so many
times look wrong.  I will wait to see if they answer.

Later, Juan.

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

[Juan Quintela]

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.

Will resend the series on top of this.

Thanks, Juan.

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

[gudkov.andrei--- via]

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.

[PATCH 2/3] migration: Teach dirtyrate about qemu_target_page_bits()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/dirtyrate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 9aa092738c..76c8b96ab4 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -374,7 +374,7 @@ static void get_ramblock_dirty_info(RAMBlock *block,
                                 sample_pages_per_gigabytes) >> 30;
     /* Right shift TARGET_PAGE_BITS to calc page count */
     info->ramblock_pages = qemu_ram_get_used_length(block) >>
-                           TARGET_PAGE_BITS;
+                           qemu_target_page_bits();
     info->ramblock_addr = qemu_ram_get_host_addr(block);
     strcpy(info->idstr, qemu_ram_get_idstr(block));
 }
@@ -485,7 +485,7 @@ find_block_matched(RAMBlock *block, int count,
 
     if (infos[i].ramblock_addr != qemu_ram_get_host_addr(block) ||
         infos[i].ramblock_pages !=
-            (qemu_ram_get_used_length(block) >> TARGET_PAGE_BITS)) {
+            (qemu_ram_get_used_length(block) >> qemu_target_page_bits())) {
         trace_find_page_matched(block->idstr);
         return NULL;
     }
-- 
2.40.1

Re: [PATCH 2/3] migration: Teach dirtyrate about qemu_target_page_bits()

[Richard Henderson]

On 5/11/23 10:22, Juan Quintela wrote:
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   migration/dirtyrate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/3] migration: Teach dirtyrate about qemu_target_page_bits()

[Philippe Mathieu-Daudé]

On 11/5/23 11:22, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/dirtyrate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/3] migration: Make dirtyrate.c target independent

[Juan Quintela]

After the previous two patches, there is nothing else that is target
specific.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/dirtyrate.c | 2 --
 migration/meson.build | 5 ++---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 76c8b96ab4..f814745bf7 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -14,9 +14,7 @@
 #include "qemu/error-report.h"
 #include <zlib.h>
 #include "qapi/error.h"
-#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"
diff --git a/migration/meson.build b/migration/meson.build
index eb41b77db9..da437b4a27 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -40,8 +40,7 @@ if get_option('live_block_migration').allowed()
   softmmu_ss.add(files('block.c'))
 endif
 softmmu_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
-
+softmmu_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c'))
 specific_ss.add(when: 'CONFIG_SOFTMMU',
-                if_true: files('dirtyrate.c',
-                               'ram.c',
+                if_true: files('ram.c',
                                'target.c'))
-- 
2.40.1

Re: [PATCH 3/3] migration: Make dirtyrate.c target independent

[Richard Henderson]

On 5/11/23 10:22, Juan Quintela wrote:
> After the previous two patches, there is nothing else that is target
> specific.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   migration/dirtyrate.c | 2 --
>   migration/meson.build | 5 ++---
>   2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index 76c8b96ab4..f814745bf7 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -14,9 +14,7 @@
>   #include "qemu/error-report.h"
>   #include <zlib.h>
>   #include "qapi/error.h"
> -#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"
> diff --git a/migration/meson.build b/migration/meson.build
> index eb41b77db9..da437b4a27 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -40,8 +40,7 @@ if get_option('live_block_migration').allowed()
>     softmmu_ss.add(files('block.c'))
>   endif
>   softmmu_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
> -
> +softmmu_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c'))

This becomes

softmmu_ss.add(files('dirtyrate.c'))

the when: is redundant with 'softmmu_ss'.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/3] migration: Make dirtyrate.c target independent

[Juan Quintela]

Richard Henderson <richard.henderson@linaro.org> wrote:
> On 5/11/23 10:22, Juan Quintela wrote:
>> After the previous two patches, there is nothing else that is target
>> specific.
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   migration/dirtyrate.c | 2 --
>>   migration/meson.build | 5 ++---
>>   2 files changed, 2 insertions(+), 5 deletions(-)
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 76c8b96ab4..f814745bf7 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -14,9 +14,7 @@
>>   #include "qemu/error-report.h"
>>   #include <zlib.h>
>>   #include "qapi/error.h"
>> -#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"
>> diff --git a/migration/meson.build b/migration/meson.build
>> index eb41b77db9..da437b4a27 100644
>> --- a/migration/meson.build
>> +++ b/migration/meson.build
>> @@ -40,8 +40,7 @@ if get_option('live_block_migration').allowed()
>>     softmmu_ss.add(files('block.c'))
>>   endif
>>   softmmu_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
>> -
>> +softmmu_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c'))
>
> This becomes
>
> softmmu_ss.add(files('dirtyrate.c'))
>
> the when: is redundant with 'softmmu_ss'.

It looked weird, but I didn't understand this stuff well enough, so I
just copyed what was there O:-)

Is there any documentation about this?  Because what I usually do is
search for a file that does (more or less) the same that I need and just
add it to that list.

> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks.

Re: [PATCH 3/3] migration: Make dirtyrate.c target independent

[Richard Henderson]

On 5/11/23 12:46, Juan Quintela wrote:
>>> +softmmu_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c'))
>>
>> This becomes
>>
>> softmmu_ss.add(files('dirtyrate.c'))
>>
>> the when: is redundant with 'softmmu_ss'.
> 
> It looked weird, but I didn't understand this stuff well enough, so I
> just copyed what was there O:-)
> 
> Is there any documentation about this?  Because what I usually do is
> search for a file that does (more or less) the same that I need and just
> add it to that list.

It's just top-level make^H^H^H^Hmeson.build related.

We have

common_ss.add_all(when: 'CONFIG_SOFTMMU', if_true: [softmmu_ss])

so everything in softmmu_ss is already protected by CONFIG_SOFTMMU.


r~