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

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

[Juan Quintela]

In this [v2]
- create qqemu_target_pages_to_MiB() and use it
- fix meson.build

Please review, Juan.

[v1]
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 (5):
  softmmu: Create qemu_target_pages_to_MiB()
  Use new created qemu_target_pages_to_MiB()
  migration: Teach dirtyrate about qemu_target_page_size()
  migration: Teach dirtyrate about qemu_target_page_bits()
  migration: Make dirtyrate.c target independent

 include/exec/target_page.h |  1 +
 migration/dirtyrate.c      | 21 +++++++++------------
 softmmu/dirtylimit.c       | 11 +++--------
 softmmu/physmem.c          | 11 +++++++++++
 migration/meson.build      |  4 ++--
 5 files changed, 26 insertions(+), 22 deletions(-)

-- 
2.40.1

[PATCH v2 1/5] softmmu: Create qemu_target_pages_to_MiB()

[Juan Quintela]

Function that convert a number of target_pages into its size in MiB.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/exec/target_page.h |  1 +
 softmmu/physmem.c          | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/exec/target_page.h b/include/exec/target_page.h
index 96726c36a4..bbf37aea17 100644
--- a/include/exec/target_page.h
+++ b/include/exec/target_page.h
@@ -18,4 +18,5 @@ size_t qemu_target_page_size(void);
 int qemu_target_page_bits(void);
 int qemu_target_page_bits_min(void);
 
+size_t qemu_target_pages_to_MiB(size_t pages);
 #endif
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 0e0182d9f2..efaed36773 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3357,6 +3357,17 @@ int qemu_target_page_bits_min(void)
     return TARGET_PAGE_BITS_MIN;
 }
 
+/* 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);
+}
+
 bool cpu_physical_memory_is_io(hwaddr phys_addr)
 {
     MemoryRegion*mr;
-- 
2.40.1

[PATCH v2 2/5] Use new created qemu_target_pages_to_MiB()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/dirtyrate.c | 11 +++++------
 softmmu/dirtylimit.c  | 11 +++--------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 180ba38c7a..38ea95af59 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -16,6 +16,7 @@
 #include "qapi/error.h"
 #include "cpu.h"
 #include "exec/ramblock.h"
+#include "exec/target_page.h"
 #include "exec/ram_addr.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/main-loop.h"
@@ -74,13 +75,11 @@ static inline void record_dirtypages(DirtyPageRecord *dirty_pages,
 static int64_t do_calculate_dirtyrate(DirtyPageRecord dirty_pages,
                                       int64_t calc_time_ms)
 {
-    uint64_t memory_size_MB;
     uint64_t increased_dirty_pages =
         dirty_pages.end_pages - dirty_pages.start_pages;
+    uint64_t memory_size_MiB = qemu_target_pages_to_MiB(increased_dirty_pages);
 
-    memory_size_MB = (increased_dirty_pages * TARGET_PAGE_SIZE) >> 20;
-
-    return memory_size_MB * 1000 / calc_time_ms;
+    return memory_size_MiB * 1000 / calc_time_ms;
 }
 
 void global_dirty_log_change(unsigned int flag, bool start)
@@ -291,8 +290,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 +=
+        qemu_target_pages_to_MiB(info->ramblock_pages);
 }
 
 static void update_dirtyrate(uint64_t msec)
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 71bf6dc7a4..015a9038d1 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -235,20 +235,15 @@ bool dirtylimit_vcpu_index_valid(int cpu_index)
 static uint64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
 {
     static uint64_t max_dirtyrate;
-    unsigned target_page_bits = qemu_target_page_bits();
-    uint64_t dirty_ring_size_MB;
+    uint64_t dirty_ring_size_MiB;
 
-    /* So far, the largest (non-huge) page size is 64k, i.e. 16 bits. */
-    assert(target_page_bits < 20);
-
-    /* Convert ring size (pages) to MiB (2**20). */
-    dirty_ring_size_MB = kvm_dirty_ring_size() >> (20 - target_page_bits);
+    dirty_ring_size_MiB = qemu_target_pages_to_MiB(kvm_dirty_ring_size());
 
     if (max_dirtyrate < dirtyrate) {
         max_dirtyrate = dirtyrate;
     }
 
-    return dirty_ring_size_MB * 1000000 / max_dirtyrate;
+    return dirty_ring_size_MiB * 1000000 / max_dirtyrate;
 }
 
 static inline bool dirtylimit_done(uint64_t quota,
-- 
2.40.1

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

[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 38ea95af59..6706e3fe66 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -314,10 +314,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

[PATCH v2 4/5] migration: Teach dirtyrate about qemu_target_page_bits()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 migration/dirtyrate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 6706e3fe66..2fd089e24a 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -372,7 +372,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));
 }
@@ -483,7 +483,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

[PATCH v2 5/5] 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---

- Remove check for CONFIG_SOFTMMU for dirtyrate.c, not needed (thanks
  Richard)
---
 migration/dirtyrate.c | 2 --
 migration/meson.build | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 2fd089e24a..f29c1c03ea 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -14,10 +14,8 @@
 #include "qemu/error-report.h"
 #include <zlib.h>
 #include "qapi/error.h"
-#include "cpu.h"
 #include "exec/ramblock.h"
 #include "exec/target_page.h"
-#include "exec/ram_addr.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/main-loop.h"
 #include "qapi/qapi-commands-migration.h"
diff --git a/migration/meson.build b/migration/meson.build
index eb41b77db9..dc8b1daef5 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -13,6 +13,7 @@ softmmu_ss.add(files(
   'block-dirty-bitmap.c',
   'channel.c',
   'channel-block.c',
+  'dirtyrate.c',
   'exec.c',
   'fd.c',
   'global_state.c',
@@ -42,6 +43,5 @@ endif
 softmmu_ss.add(when: zstd, if_true: files('multifd-zstd.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 v2 0/5] migration: Make dirtyrate.c target independent

[Richard Henderson]

On 5/11/23 15:12, Juan Quintela wrote:
> Juan Quintela (5):
>    softmmu: Create qemu_target_pages_to_MiB()
>    Use new created qemu_target_pages_to_MiB()
>    migration: Teach dirtyrate about qemu_target_page_size()
>    migration: Teach dirtyrate about qemu_target_page_bits()
>    migration: Make dirtyrate.c target independent

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

r~

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

[Philippe Mathieu-Daudé]

On 11/5/23 16:12, Juan Quintela wrote:
> 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 38ea95af59..6706e3fe66 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -314,10 +314,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();

size_t? Otherwise,

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

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

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

[Philippe Mathieu-Daudé]

On 11/5/23 16:12, Juan Quintela wrote:
> After the previous two patches, there is nothing else that is target
> specific.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> ---
> 
> - Remove check for CONFIG_SOFTMMU for dirtyrate.c, not needed (thanks
>    Richard)
> ---
>   migration/dirtyrate.c | 2 --
>   migration/meson.build | 4 ++--
>   2 files changed, 2 insertions(+), 4 deletions(-)

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

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

[Juan Quintela]

Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 11/5/23 16:12, Juan Quintela wrote:
>> 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 38ea95af59..6706e3fe66 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -314,10 +314,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();
>
> size_t? Otherwise,

Nice catch.  Will change it.

Not that it matters in real life. The bigger value that
qemu_target_page_size() can give with current hardware is 64K, so
anything bigger than a short should work O:-)

>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>>       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;